Re: [PATCH 10/14] audit: Guarantee forward progress of chunk untagging

Richard Guy Briggs
2018-10-18T19:29:38Z

On 2018-10-17 12:15, Jan Kara wrote:
> When removing chunk from a tree, we do shrink the chunk. This can fail
> for various reasons (due to races, ENOMEM, etc.) and in some cases we
> just bail from untag_chunk() relying on someone else to cleanup.
> Although this currently works, later we will need to add new failure
> situation which would break. Also this simplifies the code and will
> allow us to make locking around untag_chunk() less awkward.

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> kernel/audit_tree.c | 42 +++++++++++++++++-------------------------
> 1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index c98ab2d68a1c..ca2b6baff7aa 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -309,16 +309,28 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
> list_replace_rcu(&old->hash, &new->hash);
> }
>
> +static void remove_chunk_node(struct audit_chunk *chunk, struct node *p)
> +{
> + struct audit_tree *owner = p->owner;
> +
> + if (owner->root == chunk) {
> + list_del_init(&owner->same_root);
> + owner->root = NULL;
> + }
> + list_del_init(&p->list);
> + p->owner = NULL;
> + put_tree(owner);
> +}
> +
> static void untag_chunk(struct node *p)
> {
> struct audit_chunk *chunk = find_chunk(p);
> struct fsnotify_mark *entry = chunk->mark;
> struct audit_chunk *new = NULL;
> - struct audit_tree *owner;
> int size = chunk->count - 1;
>
> + remove_chunk_node(chunk, p);
> fsnotify_get_mark(entry);
> -
> spin_unlock(&hash_lock);
>
> if (size)
> @@ -336,15 +348,10 @@ static void untag_chunk(struct node *p)
> goto out;
> }
>
> - owner = p->owner;
> -
> if (!size) {
> chunk->dead = 1;
> spin_lock(&hash_lock);
> list_del_init(&chunk->trees);
> - if (owner->root == chunk)
> - owner->root = NULL;
> - list_del_init(&p->list);
> list_del_rcu(&chunk->hash);
> spin_unlock(&hash_lock);
> fsnotify_detach_mark(entry);
> @@ -354,21 +361,16 @@ static void untag_chunk(struct node *p)
> }
>
> if (!new)
> - goto Fallback;
> + goto out_mutex;
>
> if (fsnotify_add_mark_locked(new->mark, entry->connector->obj,
> FSNOTIFY_OBJ_TYPE_INODE, 1)) {
> fsnotify_put_mark(new->mark);
> - goto Fallback;
> + goto out_mutex;
> }
>
> chunk->dead = 1;
> spin_lock(&hash_lock);
> - if (owner->root == chunk) {
> - list_del_init(&owner->same_root);
> - owner->root = NULL;
> - }
> - list_del_init(&p->list);
> /*
> * This has to go last when updating chunk as once replace_chunk() is
> * called, new RCU readers can see the new chunk.
> @@ -381,17 +383,7 @@ static void untag_chunk(struct node *p)
> fsnotify_put_mark(new->mark); /* drop initial reference */
> goto out;
>
> -Fallback:
> - // do the best we can
> - spin_lock(&hash_lock);
> - if (owner->root == chunk) {
> - list_del_init(&owner->same_root);
> - owner->root = NULL;
> - }
> - list_del_init(&p->list);
> - p->owner = NULL;
> - put_tree(owner);
> - spin_unlock(&hash_lock);
> +out_mutex:
> mutex_unlock(&entry->group->mark_mutex);
> out:
> fsnotify_put_mark(entry);
> --
> 2.16.4
>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635