This patch contains fixes for the audit watch implementation in the git tree. - deadlock fix: release mutex on error from audit_add_watch - remove "moved" inotify watches from event handler - use inotify refcounting for audit_parent data - register inotify watches before adding rule instead of after; using inotify refcounting, this ensures our audit_watch->parent is valid - some code consolidation based on above changes Please take a look and make sure I haven't bungled anything. Signed-off-by: Amy Griffis <amy.griffis(a)hp.com&gt; -- audit.c | 8 ++ audit.h | 1 auditfilter.c | 175 ++++++++++++++++++++++------------------------------------ 3 files changed, 75 insertions(+), 109 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index b834b2e..db8bce4 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -680,6 +680,12 @@ static void audit_receive(struct sock *s mutex_unlock(&audit_cmd_mutex); } +#ifdef CONFIG_AUDITSYSCALL +static const struct inotify_operations audit_inotify_ops = { + .handle_event = audit_handle_ievent, + .destroy_watch = audit_free_parent, +}; +#endif /* Initialize audit support at boot time. */ static int __init audit_init(void) @@ -706,7 +712,7 @@ static int __init audit_init(void) audit_log(NULL, GFP_KERNEL, AUDIT_KERNEL, "initialized"); #ifdef CONFIG_AUDITSYSCALL - audit_ih = inotify_init(audit_handle_ievent); + audit_ih = inotify_init(&audit_inotify_ops); if (IS_ERR(audit_ih)) audit_panic("cannot initialize inotify handle"); diff --git a/kernel/audit.h b/kernel/audit.h index 8506287..125aebe 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -122,6 +122,7 @@ struct audit_netlink_list { int audit_send_list(void *); struct inotify_watch; +extern void audit_free_parent(struct inotify_watch *); extern void audit_handle_ievent(struct inotify_watch *, u32, u32, u32, const char *, struct inode *); extern int selinux_audit_rule_update(void); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index e116322..6caa46e 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -58,7 +58,6 @@ #include "audit.h" */ struct audit_parent { - atomic_t count; /* reference count */ struct list_head ilist; /* entry in inotify registration list */ struct list_head watches; /* associated watches */ struct inotify_watch wdata; /* inotify watch data */ @@ -98,19 +97,14 @@ extern struct inotify_handle *audit_ih; /* 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 -static inline void audit_get_parent(struct audit_parent *parent) +void audit_free_parent(struct inotify_watch *i_watch) { - atomic_inc(&parent->count); -} + struct audit_parent *parent; -static inline void audit_put_parent(struct audit_parent *parent) -{ - if (atomic_dec_and_test(&parent->count)) { - WARN_ON(!list_empty(&parent->watches)); - kfree(parent); - } + parent = container_of(i_watch, struct audit_parent, wdata); + WARN_ON(!list_empty(&parent->watches)); + kfree(parent); } static inline void audit_get_watch(struct audit_watch *watch) @@ -124,7 +118,7 @@ static inline void audit_put_watch(struc WARN_ON(!list_empty(&watch->rules)); /* watches that were never added don't have a parent */ if (watch->parent) - audit_put_parent(watch->parent); + put_inotify_watch(&watch->parent->wdata); kfree(watch->path); kfree(watch); } @@ -154,18 +148,28 @@ static inline void audit_free_rule_rcu(s } /* Initialize a parent watch entry. */ -static inline struct audit_parent *audit_init_parent(void) +static inline struct audit_parent *audit_init_parent(struct nameidata *ndp) { struct audit_parent *parent; + s32 wd; parent = kzalloc(sizeof(*parent), GFP_KERNEL); if (unlikely(!parent)) return ERR_PTR(-ENOMEM); INIT_LIST_HEAD(&parent->watches); - atomic_set(&parent->count, 1); parent->flags = 0; + inotify_init_watch(&parent->wdata); + /* grab a ref so inotify watch hangs around until we take audit_filter_mutex */ + get_inotify_watch(&parent->wdata); + wd = inotify_add_watch(audit_ih, &parent->wdata, ndp->dentry->d_inode, + AUDIT_IN_WATCH); + if (wd < 0) { + audit_free_parent(&parent->wdata); + return ERR_PTR(wd); + } + return parent; } @@ -632,7 +636,7 @@ static inline struct audit_watch *audit_ new->dev = old->dev; new->ino = old->ino; - audit_get_parent(old->parent); + get_inotify_watch(&old->parent->wdata); new->parent = old->parent; out: @@ -811,30 +815,6 @@ static inline void audit_remove_parent_w mutex_unlock(&audit_filter_mutex); } -/* Register inotify watches for parents on in_list. */ -static int audit_inotify_register(struct nameidata *nd, - struct list_head *in_list) -{ - struct audit_parent *p; - s32 wd; - int ret = 0; - - list_for_each_entry(p, in_list, ilist) { - wd = inotify_add_watch(audit_ih, &p->wdata, nd->dentry->d_inode, - AUDIT_IN_WATCH); - if (wd < 0) { - audit_remove_parent_watches(p); - /* the put matching the get in audit_init_parent() */ - audit_put_parent(p); - /* save the first error for return value */ - if (!ret) - ret = wd; - } - } - - return ret; -} - /* Unregister inotify watches for parents on in_list. * Generates an IN_IGNORED event. */ static void audit_inotify_unregister(struct list_head *in_list) @@ -844,7 +824,7 @@ static void audit_inotify_unregister(str list_for_each_entry(p, in_list, ilist) { inotify_rm_watch(audit_ih, &p->wdata); /* the put matching the get in audit_remove_watch() */ - audit_put_parent(p); + put_inotify_watch(&p->wdata); } } @@ -897,46 +877,14 @@ static inline void audit_put_nd(struct n } } -/* Add a parent inotify_watch for the given rule. */ -static int audit_add_parent(struct audit_krule *krule, - struct list_head *inotify_list) -{ - struct audit_parent *parent; - struct audit_watch *watch = krule->watch; - - parent = audit_init_parent(); - if (IS_ERR(parent)) - return PTR_ERR(parent); - - audit_get_parent(parent); - watch->parent = parent; - - /* krule, watch and parent have not been added to any global - * lists, so we don't need to take audit_filter_mutex. */ - list_add(&watch->wlist, &parent->watches); - list_add(&krule->rlist, &watch->rules); - - /* add parent to inotify registration list */ - list_add(&parent->ilist, inotify_list); - - return 0; -} - /* Associate the given rule with an existing parent inotify_watch. * Caller must hold audit_filter_mutex. */ -static int audit_add_to_parent(struct audit_krule *krule, - struct inotify_watch *iwatch) +static void audit_add_to_parent(struct audit_krule *krule, + struct audit_parent *parent) { - struct audit_parent *parent; struct audit_watch *w, *watch = krule->watch; int watch_found = 0; - parent = container_of(iwatch, struct audit_parent, wdata); - - /* parent was moved before we took audit_filter_mutex */ - if (parent->flags & AUDIT_PARENT_INVALID) - return -ENOENT; - list_for_each_entry(w, &parent->watches, wlist) { if (strcmp(watch->path, w->path)) continue; @@ -953,26 +901,23 @@ static int audit_add_to_parent(struct au } if (!watch_found) { - audit_get_parent(parent); + get_inotify_watch(&parent->wdata); watch->parent = parent; list_add(&watch->wlist, &parent->watches); } - list_add(&krule->rlist, &watch->rules); - - return 0; } /* Find a matching watch entry, or add this one. * Caller must hold audit_filter_mutex. */ static int audit_add_watch(struct audit_krule *krule, struct nameidata *ndp, - struct nameidata *ndw, - struct list_head *inotify_list) + struct nameidata *ndw) { struct audit_watch *watch = krule->watch; - struct inotify_watch *iwatch; - int ret; + struct inotify_watch *i_watch; + struct audit_parent *parent; + int ret = 0; /* update watch filter fields */ if (ndw) { @@ -981,18 +926,32 @@ static int audit_add_watch(struct audit_ } /* The audit_filter_mutex must not be held during inotify calls because - * we hold it during inotify event callback processing. - * We can trust iwatch to stick around because we hold nameidata (ndp). */ + * we hold it during inotify event callback processing. If an existing + * inotify watch is found, inotify_find_watch() grabs a reference before + * returning. + */ mutex_unlock(&audit_filter_mutex); - if (inotify_find_watch(audit_ih, ndp->dentry->d_inode, &iwatch) < 0) { - ret = audit_add_parent(krule, inotify_list); - mutex_lock(&audit_filter_mutex); - } else { - mutex_lock(&audit_filter_mutex); - ret = audit_add_to_parent(krule, iwatch); - } + if (inotify_find_watch(audit_ih, ndp->dentry->d_inode, &i_watch) < 0) { + parent = audit_init_parent(ndp); + if (IS_ERR(parent)) { + /* caller expects mutex locked */ + mutex_lock(&audit_filter_mutex); + return PTR_ERR(parent); + } + } else + parent = container_of(i_watch, struct audit_parent, wdata); + mutex_lock(&audit_filter_mutex); + + /* parent was moved before we took audit_filter_mutex */ + if (parent->flags & AUDIT_PARENT_INVALID) + ret = -ENOENT; + else + audit_add_to_parent(krule, parent); + + /* put ref grabbed by inotify_find_watch or before inotify_add_watch */ + put_inotify_watch(&parent->wdata); return ret; } @@ -1004,7 +963,6 @@ static inline int audit_add_rule(struct struct audit_field *inode_f = entry->rule.inode_f; struct audit_watch *watch = entry->rule.watch; struct nameidata *ndp, *ndw; - LIST_HEAD(inotify_list); int h, err, putnd_needed = 0; /* Taking audit_filter_mutex protects from stale rule data. */ @@ -1029,9 +987,11 @@ static inline int audit_add_rule(struct mutex_lock(&audit_filter_mutex); if (watch) { /* audit_filter_mutex is dropped and re-taken during this call */ - err = audit_add_watch(&entry->rule, ndp, ndw, &inotify_list); - if (err) + err = audit_add_watch(&entry->rule, ndp, ndw); + if (err) { + mutex_unlock(&audit_filter_mutex); goto error; + } h = audit_hash_ino((u32)watch->ino); list = &audit_inode_hash[h]; } else if (inode_f) { @@ -1046,11 +1006,6 @@ static inline int audit_add_rule(struct } mutex_unlock(&audit_filter_mutex); - if (!list_empty(&inotify_list)) { - err = audit_inotify_register(ndp, &inotify_list); - if (err) - goto error; - } if (putnd_needed) audit_put_nd(ndp, ndw); @@ -1083,7 +1038,7 @@ static inline void audit_remove_watch(st * Grab a reference before releasing audit_filter_mutex, * to be released in audit_inotify_unregister(). */ list_add(&parent->ilist, in_list); - audit_get_parent(parent); + get_inotify_watch(&parent->wdata); } } } @@ -1561,21 +1516,25 @@ int selinux_audit_rule_update(void) } /* Update watch data in audit rules based on inotify events. */ -void audit_handle_ievent(struct inotify_watch *iwatch, u32 wd, u32 mask, +void audit_handle_ievent(struct inotify_watch *i_watch, u32 wd, u32 mask, u32 cookie, const char *dname, struct inode *inode) { - struct audit_parent *parent = container_of(iwatch, struct audit_parent, wdata); + struct audit_parent *parent; + + parent = container_of(i_watch, struct audit_parent, wdata); if (mask & (IN_CREATE|IN_MOVED_TO) && inode) audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino); else if (mask & (IN_DELETE|IN_MOVED_FROM)) audit_update_watch(parent, dname, (dev_t)-1, (unsigned long)-1); - /* Note: Inotify doesn't remove the watch for the IN_MOVE_SELF event. - * Work around this by leaving the parent around with an empty - * watchlist. It will be re-used if new watches are added. */ - else if (mask & (AUDIT_IN_SELF)) + /* inotify automatically removes the watch and sends IN_IGNORED */ + else if (mask & (IN_DELETE_SELF|IN_UNMOUNT)) + audit_remove_parent_watches(parent); + /* inotify does not remove the watch, so remove it manually */ + else if(mask & IN_MOVE_SELF) { audit_remove_parent_watches(parent); - else if (mask & IN_IGNORED) - audit_put_parent(parent); /* match get in audit_init_parent() */ + inotify_remove_watch_locked(audit_ih, i_watch); + } else if (mask & IN_IGNORED) + put_inotify_watch(i_watch); }