Re: [RFC][PATCH] (#7U3) [linux-2.6.12-rc2-mm1] file system auditing

+int audit_list_watches(int pid, int seq) +{ + int ret = 0; + struct audit_wentry *wentry; + struct hlist_node *pos; + + spin_lock(&audit_master_watchlist_lock); + hlist_for_each_entry(wentry, pos, &audit_master_watchlist, w_master) { + ret = audit_send_watch(pid, seq, wentry->w_watch);

+int audit_send_watch(int pid, int seq, struct audit_watch *watch) +{ + int ret = 0; + void *memblk; + unsigned int offset; + unsigned int total; + struct audit_data *data; + struct audit_wentry *wentry; + struct audit_transport req; + struct nameidata nd; + + req.valid = 0; + req.dev_major = MAJOR(watch->dev); + req.dev_minor = MINOR(watch->dev); + req.perms = watch->perms; + req.pathlen = strlen(watch->path) + 1; + if (watch->filterkey) + req.fklen = strlen(watch->filterkey) + 1; + else + req.fklen = 0; + + ret = -ENOMEM; + total = sizeof(req) + req.pathlen + req.fklen; + memblk = kmalloc(total, GFP_KERNEL);

+ if (!memblk) + goto audit_send_watch_exit; + + /* See if path to watch is valid */ + ret = path_lookup(watch->path, LOOKUP_PARENT, &nd);

+ if (!ret) { + data = nd.dentry->d_inode->i_audit; + wentry = audit_wentry_fetch_lock(watch->name, data); + if (wentry && nd.dentry->d_inode->i_sb->s_dev == watch->dev) + req.valid = 1; + audit_wentry_put(wentry); + path_release(&nd); + } + + /* Payload */ + memcpy(memblk, &req, sizeof(req)); + offset = total - req.fklen; + memcpy(memblk + offset, watch->filterkey, req.fklen); + offset = offset - req.pathlen; + memcpy(memblk + offset, watch->path, req.pathlen); + + audit_send_reply(pid, seq, AUDIT_WATCH_LIST, 0, 1, + memblk, total);

+ kfree(memblk); + + ret = 0; + +audit_send_watch_exit: + return ret; +}

Example above is by far the most ridiculous, but the rest [of the locking] is also very bad. Stuff like write_lock(&data->lock); ... write_unlock(&data->lock); kfree(data); is not a good sign and while in this case lock is pure obfuscation (fortunately - if it could be contended here, we would be screwed) the entire thing is on that level. And they are going for "fine-grained" - i.e. drop and reacquire locks at the rate that would create [lots] of bus traffic on SMP box with nothing approaching a good reason. Note that the only comment they did not ignore was about LOOKUP_NOFOLLOW having no effect when combined with LOOKUP_PARENT - everything else is left as-is.