audit: Fix various races when tagging and untagging mounts

Currently, audit_tree code uses mark->lock to protect against detaching of mark from an inode. In most places it however also uses mark->group->mark_mutex (as we need to atomically replace attached marks) and this provides protection against mark detaching as well. So just remove protection with mark->lock from audit tree code and replace it with mark->group->mark_mutex protection in all the places. It simplifies the code and gets rid of some ugly catches like calling fsnotify_add_mark_locked() with mark->lock held (which cannot sleep only because we hold a reference to another mark attached to the same inode).

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

Quoted 14 lines · Jan Kara

Currently, audit_tree code uses mark->lock to protect against detaching of mark from an inode. In most places it however also uses mark->group->mark_mutex (as we need to atomically replace attached marks) and this provides protection against mark detaching as well. So just remove protection with mark->lock from audit tree code and replace it with mark->group->mark_mutex protection in all the places. It simplifies the code and gets rid of some ugly catches like calling fsnotify_add_mark_locked() with mark->lock held (which cannot sleep only because we hold a reference to another mark attached to the same inode).

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

...

Quoted 8 lines

kernel/audit_tree.c +0 -0

index 02feef939560..1c82eb6674c4 100644
@@ -360,12 +355,12 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
                return -ENOSPC;
        }
 

kernel/audit_tree.c static int create_chunk(struct inode *inode, struct audit_tree *tree) · line 358

Quoted 2 lines

kernel/audit_tree.c +1 -1

-       spin_lock(&entry->lock);
+       mutex_lock(&entry->group->mark_mutex);

I wonder if we could move the lock up above the fsnotify_add_inode_mark() earlier in create_chunk() and use fsnotify_add_mark_locked()?

When an inode is tagged with a tree, tag_chunk() checks whether there is audit_tree_group mark attached to the inode and adds one if not. However nothing protects another tag_chunk() to add the mark between we've checked and try to add the fsnotify mark thus resulting in an error from fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk().

Fix the problem by holding mark_mutex over the whole check-insert code sequence.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Quoted 13 lines · Jan Kara

When an inode is tagged with a tree, tag_chunk() checks whether there is audit_tree_group mark attached to the inode and adds one if not. However nothing protects another tag_chunk() to add the mark between we've checked and try to add the fsnotify mark thus resulting in an error from fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk().

Fix the problem by holding mark_mutex over the whole check-insert code sequence.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

...

Quoted 8 lines

kernel/audit_tree.c +0 -0

index 1c82eb6674c4..de8d344d91b1 100644
@@ -342,25 +342,29 @@ static void untag_chunk(struct node *p)
        spin_lock(&hash_lock);
 }
 

kernel/audit_tree.c static void untag_chunk(struct node *p) · line 345

Quoted 1 line

kernel/audit_tree.c +1 -0

+/* Call with group->mark_mutex held, releases it */

Stuff like that always makes me nervous.

Could we defer releasing the mutex to the caller, after create_chunk() returns? It looks like fsnotify_destroy_mark() allows a single level of nesting so it should be okay, yes?

Currently chunk hash key (which is in fact pointer to the inode) is derived as chunk->mark.conn->obj. It is tricky to make this dereference reliable for hash table lookups only under RCU as mark can get detached from the connector and connector gets freed independently of the running lookup. Thus there is a possible use after free / NULL ptr dereference issue:

CPU1 CPU2

                                        untag_chunk()
                                          ...
audit_tree_lookup()
  list_for_each_entry_rcu(p, list, hash) {
                                          list_del_rcu(&chunk->hash);
                                          fsnotify_destroy_mark(entry);
                                          fsnotify_put_mark(entry)
    chunk_to_key(p)
      if (!chunk->mark.connector)
                                            ...
                                            hlist_del_init_rcu(&mark->obj_list);
                                            if (hlist_empty(&conn->list)) {
                                              inode = fsnotify_detach_connector_from_object(conn);
                                            mark->connector = NULL;
                                            ...
                                            frees connector from workqueue
      chunk->mark.connector->obj

This race is probably impossible to hit in practice as the race window on CPU1 is very narrow and CPU2 has a lot of code to execute. Still it's better to have this fixed. Since the inode the chunk is attached to is constant during chunk's lifetime it is easy to cache the key in the chunk itself and thus avoid these issues.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 86 +++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 46 deletions(-)

kernel/audit_tree.c +40 -47

index 6a01738d1ac2..f419fdfc25b4 100644
@@ -235,6 +235,38 @@ static struct audit_chunk *find_chunk(struct node *p)
        return container_of(p, struct audit_chunk, owners[0]);
 }
 
+static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
+                         struct node *skip)
+{
+       struct audit_tree *owner;
+       int i, j;
+
+       new->key = old->key;
+       list_replace_init(&old->trees, &new->trees);
+       list_for_each_entry(owner, &new->trees, same_root)
+               owner->root = new;
+       for (i = j = 0; j < old->count; i++, j++) {
+               if (&old->owners[j] == skip) {
+                       i--;
+                       continue;
+               }
+               owner = old->owners[j].owner;
+               new->owners[i].owner = owner;
+               new->owners[i].index = old->owners[j].index - j + i;
+               if (!owner) /* result of earlier fallback */
+                       continue;
+               get_tree(owner);
+               list_replace_init(&old->owners[j].list, &new->owners[i].list);
+       }
+       /*
+        * Make sure chunk is fully initialized before making it visible in the
+        * hash. Pairs with a data dependency barrier in READ_ONCE() in
+        * audit_tree_lookup().
+        */
+       smp_wmb();
+       list_replace_rcu(&old->hash, &new->hash);
+}
+
 static void untag_chunk(struct node *p)
 {
        struct audit_chunk *chunk = find_chunk(p);

@@ -242,7 +274,6 @@ static void untag_chunk(struct node *p)
        struct audit_chunk *new = NULL;
        struct audit_tree *owner;
        int size = chunk->count - 1;
-       int i, j;
 
        fsnotify_get_mark(entry);
 

@@ -291,38 +322,16 @@ static void untag_chunk(struct node *p)
 
        chunk->dead = 1;
        spin_lock(&hash_lock);
-       new->key = chunk->key;
-       list_replace_init(&chunk->trees, &new->trees);
        if (owner->root == chunk) {
                list_del_init(&owner->same_root);
                owner->root = NULL;
        }
-
-       for (i = j = 0; j <= size; i++, j++) {
-               struct audit_tree *s;
-               if (&chunk->owners[j] == p) {
-                       list_del_init(&p->list);
-                       i--;
-                       continue;
-               }
-               s = chunk->owners[j].owner;
-               new->owners[i].owner = s;
-               new->owners[i].index = chunk->owners[j].index - j + i;
-               if (!s) /* result of earlier fallback */
-                       continue;
-               get_tree(s);
-               list_replace_init(&chunk->owners[j].list, &new->owners[i].list);
-       }
-
-       list_for_each_entry(owner, &new->trees, same_root)
-               owner->root = new;
+       list_del_init(&p->list);
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&chunk->hash, &new->hash);
+       replace_chunk(new, chunk, p);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(entry);
        mutex_unlock(&entry->group->mark_mutex);

@@ -399,7 +408,6 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
 static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 {
        struct fsnotify_mark *old_entry, *chunk_entry;
-       struct audit_tree *owner;
        struct audit_chunk *chunk, *old;
        struct node *p;
        int n;

@@ -464,35 +472,21 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
                fsnotify_put_mark(old_entry);
                return 0;
        }
-       chunk->key = old->key;
-       list_replace_init(&old->trees, &chunk->trees);
-       for (n = 0, p = chunk->owners; n < old->count; n++, p++) {
-               struct audit_tree *s = old->owners[n].owner;
-               p->owner = s;
-               p->index = old->owners[n].index;
-               if (!s) /* result of fallback in untag */
-                       continue;
-               get_tree(s);
-               list_replace_init(&old->owners[n].list, &p->list);
-       }
+       p = &chunk->owners[chunk->count - 1];
        p->index = (chunk->count - 1) | (1U<<31);
        p->owner = tree;
        get_tree(tree);
        list_add(&p->list, &tree->chunks);
-       list_for_each_entry(owner, &chunk->trees, same_root)
-               owner->root = chunk;
        old->dead = 1;
        if (!tree->root) {
                tree->root = chunk;
                list_add(&tree->same_root, &chunk->trees);
        }
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&old->hash, &chunk->hash);
+       replace_chunk(chunk, old, NULL);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(old_entry);
        mutex_unlock(&audit_tree_group->mark_mutex);
--

2.16.4

Quoted 4 lines · Jan Kara

Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function.

Signed-off-by Jan Kara <[email protected]>

Ack. Nice re-factoring.

Thanks,
Amir.

Quoted 149 lines

---
 kernel/audit_tree.c | 86 +++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 46 deletions(-)

kernel/audit_tree.c +40 -47

index 6a01738d1ac2..f419fdfc25b4 100644
@@ -235,6 +235,38 @@ static struct audit_chunk *find_chunk(struct node *p)
        return container_of(p, struct audit_chunk, owners[0]);
 }
 
+static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
+                         struct node *skip)
+{
+       struct audit_tree *owner;
+       int i, j;
+
+       new->key = old->key;
+       list_replace_init(&old->trees, &new->trees);
+       list_for_each_entry(owner, &new->trees, same_root)
+               owner->root = new;
+       for (i = j = 0; j < old->count; i++, j++) {
+               if (&old->owners[j] == skip) {
+                       i--;
+                       continue;
+               }
+               owner = old->owners[j].owner;
+               new->owners[i].owner = owner;
+               new->owners[i].index = old->owners[j].index - j + i;
+               if (!owner) /* result of earlier fallback */
+                       continue;
+               get_tree(owner);
+               list_replace_init(&old->owners[j].list, &new->owners[i].list);
+       }
+       /*
+        * Make sure chunk is fully initialized before making it visible in the
+        * hash. Pairs with a data dependency barrier in READ_ONCE() in
+        * audit_tree_lookup().
+        */
+       smp_wmb();
+       list_replace_rcu(&old->hash, &new->hash);
+}
+
 static void untag_chunk(struct node *p)
 {
        struct audit_chunk *chunk = find_chunk(p);

@@ -242,7 +274,6 @@ static void untag_chunk(struct node *p)
        struct audit_chunk *new = NULL;
        struct audit_tree *owner;
        int size = chunk->count - 1;
-       int i, j;
 
        fsnotify_get_mark(entry);
 

@@ -291,38 +322,16 @@ static void untag_chunk(struct node *p)
 
        chunk->dead = 1;
        spin_lock(&hash_lock);
-       new->key = chunk->key;
-       list_replace_init(&chunk->trees, &new->trees);
        if (owner->root == chunk) {
                list_del_init(&owner->same_root);
                owner->root = NULL;
        }
-
-       for (i = j = 0; j <= size; i++, j++) {
-               struct audit_tree *s;
-               if (&chunk->owners[j] == p) {
-                       list_del_init(&p->list);
-                       i--;
-                       continue;
-               }
-               s = chunk->owners[j].owner;
-               new->owners[i].owner = s;
-               new->owners[i].index = chunk->owners[j].index - j + i;
-               if (!s) /* result of earlier fallback */
-                       continue;
-               get_tree(s);
-               list_replace_init(&chunk->owners[j].list, &new->owners[i].list);
-       }
-
-       list_for_each_entry(owner, &new->trees, same_root)
-               owner->root = new;
+       list_del_init(&p->list);
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&chunk->hash, &new->hash);
+       replace_chunk(new, chunk, p);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(entry);
        mutex_unlock(&entry->group->mark_mutex);

@@ -399,7 +408,6 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
 static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 {
        struct fsnotify_mark *old_entry, *chunk_entry;
-       struct audit_tree *owner;
        struct audit_chunk *chunk, *old;
        struct node *p;
        int n;

@@ -464,35 +472,21 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
                fsnotify_put_mark(old_entry);
                return 0;
        }
-       chunk->key = old->key;
-       list_replace_init(&old->trees, &chunk->trees);
-       for (n = 0, p = chunk->owners; n < old->count; n++, p++) {
-               struct audit_tree *s = old->owners[n].owner;
-               p->owner = s;
-               p->index = old->owners[n].index;
-               if (!s) /* result of fallback in untag */
-                       continue;
-               get_tree(s);
-               list_replace_init(&old->owners[n].list, &p->list);
-       }
+       p = &chunk->owners[chunk->count - 1];
        p->index = (chunk->count - 1) | (1U<<31);
        p->owner = tree;
        get_tree(tree);
        list_add(&p->list, &tree->chunks);
-       list_for_each_entry(owner, &chunk->trees, same_root)
-               owner->root = chunk;
        old->dead = 1;
        if (!tree->root) {
                tree->root = chunk;
                list_add(&tree->same_root, &chunk->trees);
        }
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&old->hash, &chunk->hash);
+       replace_chunk(chunk, old, NULL);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(old_entry);
        mutex_unlock(&audit_tree_group->mark_mutex);
--

2.16.4

Quoted 4 lines · Amir Goldstein

Quoted 3 lines · Jan Kara

Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function.

Quoted 3 lines

Quoted 1 line

Signed-off-by Jan Kara <[email protected]>

Ack. Nice re-factoring.

Thanks. BTW, I didn't add your Acked-by tags because it wasn't obvious to me whether you meant them as an official tag-worthy statement. So do want me to add the tags or do you prefer to stay "anonymous reviewer" ;)?

Quoted 5 lines · Jan Kara

Quoted 4 lines · Amir Goldstein

Quoted 3 lines · Jan Kara

Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Jan Kara <[email protected]>

Ack. Nice re-factoring.

Thanks. BTW, I didn't add your Acked-by tags because it wasn't obvious to me whether you meant them as an official tag-worthy statement. So do want me to add the tags or do you prefer to stay "anonymous reviewer" ;)?

Don't feel confident enough with Reviewed-by/Acked-by on this series, just letting you know that I eyeballed your patches FWIW ;-)

I'm down to the last one, which is agonizing to make sure nothing is missed from the diff. At this point [10/10] I'd rather review the final code.

Can you push the work to a branch.

Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 86 +++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 46 deletions(-)

kernel/audit_tree.c +40 -47

index 6a01738d1ac2..f419fdfc25b4 100644
@@ -235,6 +235,38 @@ static struct audit_chunk *find_chunk(struct node *p)
        return container_of(p, struct audit_chunk, owners[0]);
 }
 
+static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
+                         struct node *skip)
+{
+       struct audit_tree *owner;
+       int i, j;
+
+       new->key = old->key;
+       list_replace_init(&old->trees, &new->trees);
+       list_for_each_entry(owner, &new->trees, same_root)
+               owner->root = new;
+       for (i = j = 0; j < old->count; i++, j++) {
+               if (&old->owners[j] == skip) {
+                       i--;
+                       continue;
+               }
+               owner = old->owners[j].owner;
+               new->owners[i].owner = owner;
+               new->owners[i].index = old->owners[j].index - j + i;
+               if (!owner) /* result of earlier fallback */
+                       continue;
+               get_tree(owner);
+               list_replace_init(&old->owners[j].list, &new->owners[i].list);
+       }
+       /*
+        * Make sure chunk is fully initialized before making it visible in the
+        * hash. Pairs with a data dependency barrier in READ_ONCE() in
+        * audit_tree_lookup().
+        */
+       smp_wmb();
+       list_replace_rcu(&old->hash, &new->hash);
+}
+
 static void untag_chunk(struct node *p)
 {
        struct audit_chunk *chunk = find_chunk(p);

@@ -242,7 +274,6 @@ static void untag_chunk(struct node *p)
        struct audit_chunk *new = NULL;
        struct audit_tree *owner;
        int size = chunk->count - 1;
-       int i, j;
 
        fsnotify_get_mark(entry);
 

@@ -291,38 +322,16 @@ static void untag_chunk(struct node *p)
 
        chunk->dead = 1;
        spin_lock(&hash_lock);
-       new->key = chunk->key;
-       list_replace_init(&chunk->trees, &new->trees);
        if (owner->root == chunk) {
                list_del_init(&owner->same_root);
                owner->root = NULL;
        }
-
-       for (i = j = 0; j <= size; i++, j++) {
-               struct audit_tree *s;
-               if (&chunk->owners[j] == p) {
-                       list_del_init(&p->list);
-                       i--;
-                       continue;
-               }
-               s = chunk->owners[j].owner;
-               new->owners[i].owner = s;
-               new->owners[i].index = chunk->owners[j].index - j + i;
-               if (!s) /* result of earlier fallback */
-                       continue;
-               get_tree(s);
-               list_replace_init(&chunk->owners[j].list, &new->owners[i].list);
-       }
-
-       list_for_each_entry(owner, &new->trees, same_root)
-               owner->root = new;
+       list_del_init(&p->list);
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&chunk->hash, &new->hash);
+       replace_chunk(new, chunk, p);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(entry);
        mutex_unlock(&entry->group->mark_mutex);

@@ -399,7 +408,6 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
 static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 {
        struct fsnotify_mark *old_entry, *chunk_entry;
-       struct audit_tree *owner;
        struct audit_chunk *chunk, *old;
        struct node *p;
        int n;

@@ -464,35 +472,21 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
                fsnotify_put_mark(old_entry);
                return 0;
        }
-       chunk->key = old->key;
-       list_replace_init(&old->trees, &chunk->trees);
-       for (n = 0, p = chunk->owners; n < old->count; n++, p++) {
-               struct audit_tree *s = old->owners[n].owner;
-               p->owner = s;
-               p->index = old->owners[n].index;
-               if (!s) /* result of fallback in untag */
-                       continue;
-               get_tree(s);
-               list_replace_init(&old->owners[n].list, &p->list);
-       }
+       p = &chunk->owners[chunk->count - 1];
        p->index = (chunk->count - 1) | (1U<<31);
        p->owner = tree;
        get_tree(tree);
        list_add(&p->list, &tree->chunks);
-       list_for_each_entry(owner, &chunk->trees, same_root)
-               owner->root = chunk;
        old->dead = 1;
        if (!tree->root) {
                tree->root = chunk;
                list_add(&tree->same_root, &chunk->trees);
        }
        /*
-        * Make sure chunk is fully initialized before making it visible in the
-        * hash. Pairs with a data dependency barrier in READ_ONCE() in
-        * audit_tree_lookup().
+        * This has to go last when updating chunk as once replace_chunk() is
+        * called, new RCU readers can see the new chunk.
         */
-       smp_wmb();
-       list_replace_rcu(&old->hash, &chunk->hash);
+       replace_chunk(chunk, old, NULL);
        spin_unlock(&hash_lock);
        fsnotify_detach_mark(old_entry);
        mutex_unlock(&audit_tree_group->mark_mutex);
--

2.16.4

The audit_tree_group->mark_mutex is held all the time while we create the fsnotify mark, add it to the inode, and insert chunk into the hash. Hence mark cannot get detached during this time and so the check whether the mark is attached in insert_hash() is pointless.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 2 --
 1 file changed, 2 deletions(-)

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 14 deletions(-)

kernel/audit_tree.c +45 -15

index bce3b04a365d..aec9b27a20ff 100644
@@ -25,7 +25,7 @@ struct audit_tree {
 struct audit_chunk {
        struct list_head hash;
        unsigned long key;
-       struct fsnotify_mark mark;
+       struct fsnotify_mark *mark;
        struct list_head trees;         /* with root here */
        int dead;
        int count;

@@ -38,6 +38,11 @@ struct audit_chunk {
        } owners[];
 };
 
+struct audit_tree_mark {
+       struct fsnotify_mark fsn_mark;
+       struct audit_chunk *chunk;
+};
+
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
 static struct task_struct *prune_thread;

@@ -73,6 +78,7 @@ static struct task_struct *prune_thread;
  */
 
 static struct fsnotify_group *audit_tree_group;
+static struct kmem_cache *audit_tree_mark_cachep __read_mostly;
 
 static struct audit_tree *alloc_tree(const char *s)
 {

@@ -142,10 +148,28 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
        call_rcu(&chunk->head, __put_chunk);
 }
 
+static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
+{
+       return container_of(entry, struct audit_tree_mark, fsn_mark);
+}
+
 static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
 {
-       struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+       struct audit_chunk *chunk = AUDIT_M(entry)->chunk;
        audit_mark_put_chunk(chunk);
+       kmem_cache_free(audit_tree_mark_cachep, entry);
+}
+
+static struct fsnotify_mark *alloc_fsnotify_mark(void)
+{
+       struct audit_tree_mark *mark;
+
+       mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
+       if (!mark)
+               return NULL;
+       fsnotify_init_mark(&mark->fsn_mark, audit_tree_group);
+       mark->fsn_mark.mask = FS_IN_IGNORED;
+       return &mark->fsn_mark;
 }
 
 static struct audit_chunk *alloc_chunk(int count)

@@ -159,6 +183,13 @@ static struct audit_chunk *alloc_chunk(int count)
        if (!chunk)
                return NULL;
 
+       chunk->mark = alloc_fsnotify_mark();
+       if (!chunk->mark) {
+               kfree(chunk);
+               return NULL;
+       }
+       AUDIT_M(chunk->mark)->chunk = chunk;
+
        INIT_LIST_HEAD(&chunk->hash);
        INIT_LIST_HEAD(&chunk->trees);
        chunk->count = count;

@@ -167,8 +198,6 @@ static struct audit_chunk *alloc_chunk(int count)
                INIT_LIST_HEAD(&chunk->owners[i].list);
                chunk->owners[i].index = i;
        }
-       fsnotify_init_mark(&chunk->mark, audit_tree_group);
-       chunk->mark.mask = FS_IN_IGNORED;
        return chunk;
 }
 

@@ -278,7 +307,7 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
 static void untag_chunk(struct node *p)
 {
        struct audit_chunk *chunk = find_chunk(p);
-       struct fsnotify_mark *entry = &chunk->mark;
+       struct fsnotify_mark *entry = chunk->mark;
        struct audit_chunk *new = NULL;
        struct audit_tree *owner;
        int size = chunk->count - 1;

@@ -298,7 +327,7 @@ static void untag_chunk(struct node *p)
        if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
                mutex_unlock(&entry->group->mark_mutex);
                if (new)
-                       fsnotify_put_mark(&new->mark);
+                       fsnotify_put_mark(new->mark);
                goto out;
        }
 

@@ -322,9 +351,9 @@ static void untag_chunk(struct node *p)
        if (!new)
                goto Fallback;
 
-       if (fsnotify_add_mark_locked(&new->mark, entry->connector->obj,
+       if (fsnotify_add_mark_locked(new->mark, entry->connector->obj,
                                     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
-               fsnotify_put_mark(&new->mark);
+               fsnotify_put_mark(new->mark);
                goto Fallback;
        }
 

@@ -344,7 +373,7 @@ static void untag_chunk(struct node *p)
        fsnotify_detach_mark(entry);
        mutex_unlock(&entry->group->mark_mutex);
        fsnotify_free_mark(entry);
-       fsnotify_put_mark(&new->mark);  /* drop initial reference */
+       fsnotify_put_mark(new->mark);   /* drop initial reference */
        goto out;
 
 Fallback:

@@ -375,7 +404,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
                return -ENOMEM;
        }
 
-       entry = &chunk->mark;
+       entry = chunk->mark;
        if (fsnotify_add_inode_mark_locked(entry, inode, 0)) {
                mutex_unlock(&audit_tree_group->mark_mutex);
                fsnotify_put_mark(entry);

@@ -426,7 +455,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
        if (!old_entry)
                return create_chunk(inode, tree);
 
-       old = container_of(old_entry, struct audit_chunk, mark);
+       old = AUDIT_M(old_entry)->chunk;
 
        /* are we already there? */
        spin_lock(&hash_lock);

@@ -447,7 +476,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
                return -ENOMEM;
        }
 
-       chunk_entry = &chunk->mark;
+       chunk_entry = chunk->mark;
 
        /*
         * mark_mutex protects mark from getting detached and thus also from

@@ -457,7 +486,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
                /* old_entry is being shot, lets just lie */
                mutex_unlock(&audit_tree_group->mark_mutex);
                fsnotify_put_mark(old_entry);
-               fsnotify_put_mark(&chunk->mark);
+               fsnotify_put_mark(chunk->mark);
                return -ENOENT;
        }
 

@@ -1009,7 +1038,7 @@ static int audit_tree_handle_event(struct fsnotify_group *group,
 
 static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group)
 {
-       struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+       struct audit_chunk *chunk = AUDIT_M(entry)->chunk;
 
        evict_chunk(chunk);
 

@@ -1030,6 +1059,8 @@ static int __init audit_tree_init(void)
 {
        int i;
 
+       audit_tree_mark_cachep = KMEM_CACHE(audit_tree_mark, SLAB_PANIC);
+
        audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
        if (IS_ERR(audit_tree_group))
                audit_panic("cannot initialize fsnotify group for rectree watches");
--

2.16.4

Quoted 6 lines · Jan Kara

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

Ack.

kernel/audit_tree.c static void audit_mark_put_chunk(struct audit_chunk *chunk) · line 160

Quoted 11 lines

kernel/audit_tree.c +7 -1

+static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
+{
+       return container_of(entry, struct audit_tree_mark, fsn_mark);
+}
+
 static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
 {
-       struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+       struct audit_chunk *chunk = AUDIT_M(entry)->chunk;
        audit_mark_put_chunk(chunk);
+       kmem_cache_free(audit_tree_mark_cachep, entry);

Quoted 9 lines · Amir Goldstein

Quoted 6 lines · Jan Kara

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

Ack.

Found minor nit.

[...]

Quoted 11 lines

kernel/audit_tree.c static void audit_mark_put_chunk(struct audit_chunk *chunk) · line 160

Quoted 11 lines

kernel/audit_tree.c +7 -1

+static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
+{
+       return container_of(entry, struct audit_tree_mark, fsn_mark);
+}
+
 static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
 {
-       struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+       struct audit_chunk *chunk = AUDIT_M(entry)->chunk;
        audit_mark_put_chunk(chunk);
+       kmem_cache_free(audit_tree_mark_cachep, entry);

Technically, we should be freeing AUDIT_M(entry) although it is the same address with current struct layout.

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 14 deletions(-)

Quoted 9 lines · Jan Kara

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 14 deletions(-)

...

Quoted 8 lines

kernel/audit_tree.c +0 -0

index bce3b04a365d..aec9b27a20ff 100644
@@ -38,6 +38,11 @@ struct audit_chunk {
        } owners[];
 };
 

kernel/audit_tree.c struct audit_chunk { · line 44

Quoted 4 lines

kernel/audit_tree.c +4 -0

+struct audit_tree_mark {
+       struct fsnotify_mark fsn_mark;
+       struct audit_chunk *chunk;
+};

It's probably okay to just call it "mark" considering we call fsnotify_mark fields "mark" elsewhere. If we are going to change it in one spot we should probably change it other places as well for the sake of readability.

2,10 +148,28 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)

Quoted 3 lines

kernel/audit_tree.c +0 -0

        call_rcu(&chunk->head, __put_chunk);
 }
 

kernel/audit_tree.c static void audit_mark_put_chunk(struct audit_chunk *chunk) · line 154

Quoted 4 lines

kernel/audit_tree.c +4 -0

+static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
+{
+       return container_of(entry, struct audit_tree_mark, fsn_mark);
+}

When I see a symbol in all caps I think "macro definition", but this isn't a macro definition. I would suggest a different name, dropping the caps, or converting it into a macro.

Also, unless I missed a call, it seems like after patch 10 all callers of AUDIT_M end up getting the chunk field; maybe it is better if AUDIT_M() ends up returning the audit_chunk pointer instead of the audit_tree_mark pointer (and of course a name change if this is a reasonable change)?

Quoted 6 lines

kernel/audit_tree.c +6 -0

+static struct fsnotify_mark *alloc_fsnotify_mark(void)
+{
+       struct audit_tree_mark *mark;
+
+       mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
+       if (!mark)

kernel/audit_tree.c static void audit_mark_put_chunk(struct audit_chunk *chunk) · line 173

Quoted 5 lines

kernel/audit_tree.c +4 -0

+               return NULL;
+       fsnotify_init_mark(&mark->fsn_mark, audit_tree_group);
+       mark->fsn_mark.mask = FS_IN_IGNORED;
+       return &mark->fsn_mark;
 }

Can't we just call it alloc_mark()? Or did you create the function earlier in the patchset and I'm missing it now?

[SIDE NOTE: I have some rather big disagreements with the current naming in this file, but keeping things consistent seems to be a Good Thing (once again, this is an existing problem not specific to your patches).]

Quoted 21 lines · Paul Moore

Quoted 9 lines · Jan Kara

Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 14 deletions(-)

...

Quoted 8 lines

kernel/audit_tree.c +0 -0

index bce3b04a365d..aec9b27a20ff 100644
@@ -38,6 +38,11 @@ struct audit_chunk {
        } owners[];
 };
 

Quoted 9 lines

kernel/audit_tree.c struct audit_chunk { · line 44

Quoted 4 lines

kernel/audit_tree.c +4 -0

+struct audit_tree_mark {
+       struct fsnotify_mark fsn_mark;
+       struct audit_chunk *chunk;
+};

It's probably okay to just call it "mark" considering we call fsnotify_mark fields "mark" elsewhere. If we are going to change it in one spot we should probably change it other places as well for the sake of readability.

The current notation is that 'fsn_mark' (or sometimes 'entry') is struct fsnotify_mark while plain 'mark' is struct audit_tree_mark (well, except for audit_chunk AFAICS). So just replacing fsn_mark with mark is IMO going to cause more confusion. But if you prefer different naming convention, this is the right moment to bring some consistency into the whole thing. So how do you prefer to differentiate between fsnotify_mark and audit_tree_mark?

Audit tree code currently associates new fsnotify mark with each new chunk. As chunk attached to an inode is replaced when new tag is added / removed, we also need to remove old fsnotify mark and add a new one on such occasion. This is cumbersome and makes locking rules somewhat difficult to follow.

Fix these problems by allocating fsnotify mark independently of chunk and keeping it all the time while there is some chunk attached to an inode. Also add documentation about the locking rules so that things are easier to follow.

Signed-off-by Jan Kara <[email protected]>

---

Audit tree code currently associates new fsnotify mark with each new chunk. As chunk attached to an inode is replaced when new tag is added / removed, we also need to remove old fsnotify mark and add a new one on such occasion. This is cumbersome and makes locking rules somewhat difficult to follow.

Fix these problems by allocating fsnotify mark independently of chunk and keeping it all the time while there is some chunk attached to an inode. Also add documentation about the locking rules so that things are easier to follow.

Signed-off-by Jan Kara <[email protected]>

---
 kernel/audit_tree.c | 163 +++++++++++++++++++++++++++-------------------------
 1 file changed, 85 insertions(+), 78 deletions(-)

kernel/audit_tree.c +2 -0

index aec9b27a20ff..40f61de77dd0 100644
@@ -272,6 +273,20 @@ static struct audit_chunk *find_chunk(struct node *p)
        return container_of(p, struct audit_chunk, owners[0]);
 }
 
+static void replace_mark_chunk(struct fsnotify_mark *entry,
+                              struct audit_chunk *chunk)

Quoted 8 lines · Jan Kara

kernel/audit_tree.c +0 -0

index aec9b27a20ff..40f61de77dd0 100644
@@ -272,6 +273,20 @@ static struct audit_chunk *find_chunk(struct node *p)
        return container_of(p, struct audit_chunk, owners[0]);
 }
 

kernel/audit_tree.c static struct audit_chunk *find_chunk(struct node *p) · line 287

Quoted 12 lines

kernel/audit_tree.c +12 -0

+static void replace_mark_chunk(struct fsnotify_mark *entry,
+                              struct audit_chunk *chunk)
+{
+       struct audit_chunk *old;
+
+       assert_spin_locked(&hash_lock);
+       old = AUDIT_M(entry)->chunk;
+       AUDIT_M(entry)->chunk = chunk;
+       if (chunk)
+               chunk->mark = entry;
+       if (old)
+               old->mark = NULL;

Is it necessary that we check to see if chunk and old are non-NULL? It seems like we would always want to set chunk->mark to entry and set old->mark to NULL, yes?

kernel/audit_tree.c static void untag_chunk(struct node *p) · line 345

Quoted 8 lines

kernel/audit_tree.c +2 -2

@@ -321,29 +341,31 @@ static void untag_chunk(struct node *p)
 
        mutex_lock(&entry->group->mark_mutex);
        /*
-        * mark_mutex protects mark from getting detached and thus also from
-        * mark->connector->obj getting NULL.
+        * mark_mutex protects mark stabilizes chunk attached to the mark so we
+        * can check whether it didn't change while we've dropped hash_lock.

I think your new text could use some revision, the "protects mark stabilizes chunk" is odd.

kernel/audit_tree.c static void untag_chunk(struct node *p) · line 351

Quoted 8 lines

kernel/audit_tree.c +3 -2

         */
-       if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
+       if (!(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED) ||
+           AUDIT_M(entry)->chunk != chunk) {
                mutex_unlock(&entry->group->mark_mutex);
                if (new)
-                       fsnotify_put_mark(new->mark);
+                       kfree(new);

Since we are just calling kfree() now we can do away with the "if (new)" check.