Re: [PATCH] filesystem location based auditing
Several notes:
+/* Remove given krule from its associated watch's rules list and
clean up any
+ * last instances of associated watch and parent.
+ * Caller must hold exit filterlist lock */
+static inline void audit_remove_watch(struct audit_krule *krule,
+ struct list_head *in_list)
+{
+ struct audit_watch *watch = krule->watch;
+ struct audit_parent *parent = watch->parent;
+
+ list_del(&krule->rlist);
+ if (list_empty(&watch->rules)) {
+ list_del(&watch->wlist);
+ audit_put_watch(watch);
+
+ if (list_empty(&parent->watches)) {
+ /* put parent on the inotify un-registration list */
+ list_add(&parent->ilist, in_list);
+ audit_get_parent(parent);
+ }
+ }
}
Umm... What guarantees that parent survives audit_put_watch()?
Another thing that looks rather fishy: audit_inotify_register()
+ if (wd < 0) {
+ audit_remove_parent_watches(p);
+ audit_remove_parent(p);
+ audit_put_parent(p);
+ ret = wd;
+ } else {
followed by _another_ audit_put_parent(), balancing audit_get_parent()
before. Since audit_remove_parent() also drops a reference... Something
doesn't add up.
AFAICS, audit_get_watch() puts new parent on list, with refcount 2
and one watch attached. We bump refcount to 3. Then we have
audit_remove_parent_watches() do audit_put_watch() on the only watch,
which will drop its reference to parent, leaving us with refcount 2.
Then audit_remove_parent() will call audit_put_parent(), getting the
refcount to 1. Then we do audit_put_parent(), releaseing the last
reference and freeing p; *then* we do audit_put_parent(p) again. Oops...
Could you put at least short comments on refcounting in there?
One more: audit_put_nd() blocks, so
+ spin_lock(&flist->lock);
+ if (watch) {
+ err = audit_add_watch(&entry->rule, ndp, ndw, &inotify_list);
+ if (err) {
+ audit_put_nd(ndp, ndw);
deadlocks - audit_add_watch() won't drop the spinlock for us. Forgotten
spin_unlock()?
In audit_list():
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &audit_filter_list[i].head, list) {
struct audit_rule_data *data;
data = audit_krule_to_data(&e->rule);
if (unlikely(!data))
break;
audit_send_reply(pid, seq, AUDIT_LIST_RULES, 0, 1,
- data, sizeof(*data));
+ data, sizeof(*data) + data->buflen);
audit_send_reply() quite obviously blocks. To quote include/linux/rcupdate.h:
/**
* rcu_read_lock - mark the beginning of an RCU read-side critical section.
*
...
*
* It is illegal to block while in an RCU read-side critical section.
*/
#define rcu_read_lock() preempt_disable()
In audit_list_rules(): ditto.