5:05 p.m.
On Fri, Feb 24, 2006 at 03:07:30PM -0600, Timothy R. Chavez wrote:
> +/* Inotify events we care about. */
> +#define AUDIT_IN_WATCH IN_MOVE|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF
> +#define AUDIT_IN_SELF IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT
> +
> +/* Flags for stale filterlist data */
> +#define AUDIT_ENTRY_ADD 0x01 /* Rule entry addition in progress. */
> +#define AUDIT_ENTRY_DEL 0x02 /* Rule entry deletion in progress. */
> +#define AUDIT_PARENT_DEL 0x01 /* Parent deletion in progress. */
AUDIT_ENTRY_ADD and AUDIT_PARENT_DEL are the same? I'm assuming since
this is a mask, that it probably makes sense to make 0x04, no?
AUDIT_ENTRY_* and AUDIT_PARENT_* were two different masks in the
audit_entry and audit_parent structs. I've re-written the code
though, so neither of them is needed now.
> +/* Remove all watches & rules associated with a parent that
is going away. */
> +static inline void audit_remove_parent_watches(struct audit_parent *parent)
> +{
> + struct audit_watch *w, *nextw;
> + struct audit_krule *r, *nextr;
> + struct audit_entry *e;
> + struct audit_flist *flist = &audit_filter_list[AUDIT_FILTER_EXIT];
> +
> + spin_lock(&flist->lock);
> + list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
> + list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
> + e = container_of(r, struct audit_entry, rule);
> +
> + /* make sure we have a valid copy */
> + while (e->replacement != NULL)
> + e = e->replacement;
> + if (e->flags & AUDIT_ENTRY_DEL)
> + continue;
> +
> + list_del(&r->rlist);
> + list_del_rcu(&e->list);
> + e->flags |= AUDIT_ENTRY_DEL;
> + call_rcu(&e->rcu, audit_free_rule_rcu);
> + audit_log(NULL, GFP_ATOMIC, AUDIT_CONFIG_CHANGE,
> + "audit implicitly removed rule from list=%d\n",
> + AUDIT_FILTER_EXIT);
> + }
> + list_del(&w->wlist);
> + audit_put_watch(w);
> + }
> + spin_unlock(&flist->lock);
> +}
Why not rcu locking here? You can iterate over the list w/
rcu_read_lock/unlock and then removals from the list via admin
action can be defferred using list_del_rcu? Doesn't the same apply
for additions to the flist as well..
I am using rcu locking, but you need an additional lock to protect
against concurrent list writes.
> +/* Register this parent's inotify watch. */
> +static int audit_inotify_register(void *_data)
> +{
> + void **data = _data;
> + struct audit_parent *parent;
> + char *path;
> + struct nameidata nd;
> + int err;
> + u32 wd;
> +
> + parent = data[0];
> + path = data[1];
> + kfree(data);
> +
> + err = path_lookup(path, LOOKUP_PARENT, &nd);
> + if (err)
> + goto handle_error;
> +
> + wd = inotify_add_watch(audit_idev, nd.dentry->d_inode, AUDIT_IN_WATCH,
> + parent);
> + if (wd < 0)
> + goto handle_error;
> +
> + spin_lock(&master_parents_lock);
> + parent->wd = wd;
> + parent->ino = nd.dentry->d_inode->i_ino;
> + spin_unlock(&master_parents_lock);
> +
> + path_release(&nd);
> + audit_put_parent(parent);
> + return 0;
> +
> +handle_error:
> + path_release(&nd);
> + audit_remove_parent_watches(parent);
> + audit_remove_parent(parent);
> +
> + audit_put_parent(parent);
> + return 0;
Hm... on error you return 0? That's what you return on success too.
Yeah, the function was supposed to contain the error by cleaning up
after itself. It's a little different in the latest iteration...
> +/* Unregister this parent's inotify watch. Generates an
IN_IGNORED event. */
> +static int audit_inotify_unregister(void *data)
> +{
> + struct audit_parent *parent = data;
> + s32 wd;
> +
> + spin_lock(&master_parents_lock);
> + wd = parent->wd;
> + spin_unlock(&master_parents_lock);
> +
> + if (inotify_ignore(audit_idev, wd))
> + printk(KERN_ERR
> + "%s:%d: unable to remove inotify watch for inode %lu\n",
> + __FILE__, __LINE__, parent->ino);
This is benign?
Not really benign, but a way to signal an un-recoverable error. This
situation has been removed by not using kthread_run.
> +/* Initialize a parent watch entry. */
> +static inline struct audit_parent *audit_init_parent(char *path,
> + unsigned long ino)
> +{
> + struct audit_parent *parent;
> + void **data;
> + struct task_struct *task;
> +
> + parent = kmalloc(sizeof(*parent), GFP_ATOMIC);
> + if (unlikely(!parent))
> + return ERR_PTR(-ENOMEM);
> +
> + memset(parent, 0, sizeof(*parent));
> + INIT_LIST_HEAD(&parent->watches);
> + atomic_set(&parent->count, 0);
> + parent->ino = ino;
> + audit_get_parent(parent);
> +
> + /* Spawn a thread to register the parent's inotify watch without
> + * the filterlist spinlock. */
> + data = kmalloc(2 * sizeof(void *), GFP_ATOMIC);
> + if (!data) {
> + audit_put_parent(parent);
> + return ERR_PTR(-ENOMEM);
> + }
> + data[0] = parent;
> + data[1] = path;
> + audit_get_parent(parent);
> + task = kthread_run(audit_inotify_register, data,
> + "audit_inotify_register");
> + if (IS_ERR(task)) {
> + audit_put_parent(parent);
> + return ERR_PTR(PTR_ERR(task));
Redundant? return ERR_PTR(task), right?
The function returns an audit_parent struct, and this is a task
struct. Anyway, it's been removed as a result of other changes. :-)
> +/* Initialize a watch entry. */
> +static inline struct audit_watch *audit_init_watch(char *path)
> +{
> + struct audit_watch *watch;
> +
> + watch = kmalloc(sizeof(*watch), GFP_KERNEL);
> + if (unlikely(!watch))
> + return ERR_PTR(-ENOMEM);
> +
> + memset(watch, 0, sizeof(*watch));
> + INIT_LIST_HEAD(&watch->rules);
> + atomic_set(&watch->count, 0);
> + watch->path = path;
> + audit_get_watch(watch);
> +
Why not just atomic_set(&watch->count, 1), rather than making two
atomic calls?
Done.
> +/* Compare given dentry name with last component in given
path,
> + * return of 0 indicates a match. */
> +int audit_compare_dname_path(const char *dname, const char *path)
> +{
> + int dlen, plen;
> + const char *p;
>
> + if (!dname || !path)
> + return 1;
> +
> + dlen = strlen(dname);
> + plen = strlen(path);
> + if (plen < dlen)
> + return 1;
> +
> + /* disregard trailing slashes */
> + p = path + plen - 1;
> + while ((*p == '/') && (p > path))
> + p--;
> +
> + /* find last path component */
Why not path_lookup with LOOKUP_PARENT, then d_lookup?
I was hoping to avoid doing path_lookup here.
> + p = p - dlen + 1;
> + if (p < path)
> + return 1;
> + else if (p > path) {
> + if (*--p != '/')
> + return 1;
> + else
> + p++;
> + }
> +
> + return strncmp(p, dname, dlen);
> +}