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

We've asked Al to take a another look over the patch and provide a second round of feedback... On Wed, 2005-04-27 at 18:17 -0500, Timothy R. Chavez wrote: > +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); OK, so audit_send_watch() is called with a spinlock held, and hence may not sleep... > +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); BANG. > + if (!memblk) > + goto audit_send_watch_exit; > + > + /* See if path to watch is valid */ > + ret = path_lookup(watch->path, LOOKUP_PARENT, &nd); BANG. > + 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); BANG. > + kfree(memblk); > + > + ret = 0; > + > +audit_send_watch_exit: > + return ret; > +} Other feedback includes... > 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.