Re: [PATCH 12/14] audit: Simplify locking around untag_chunk()

Jan Kara
2018-10-19T08:22:10Z

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Thu 18-10-18 08:27:40, Richard Guy Briggs wrote:
> > if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
> > - mutex_unlock(&entry->group->mark_mutex);
> > - if (new)
> > - fsnotify_put_mark(new->mark);
> > - goto out;
> > + mutex_unlock(&audit_tree_group->mark_mutex);
> > + return;
>
> This isn't an error, but more a question of style. Since the end of the
> function has been simplified, this could just be "goto out_mutex", or
> remove the one remaining "goto out_mutex" after the next patch and do
> the same as here since other exits aren't so clean.

Good point, I've switch this to "goto out_mutex". The result is attached.
Can I add your Reviewed-by tag?

Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0012-audit-Simplify-locking-around-untag_chunk.patch --]
[-- Type: text/x-patch, Size: 4776 bytes --]

>From d06a004e90d6ee99b19e95a343f947b11dc8aed5 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 15 Oct 2018 10:56:31 +0200
Subject: [PATCH 12/14] audit: Simplify locking around untag_chunk()

untag_chunk() has to be called with hash_lock, it drops it and
reacquires it when returning. The unlocking of hash_lock is thus hidden
from the callers of untag_chunk() with is rather error prone. Reorganize
the code so that untag_chunk() is called without hash_lock, only with
mark reference preventing the chunk from going away.

Since this requires some more code in the caller of untag_chunk() to
assure forward progress, factor out loop pruning tree from all chunks
into a common helper function.

Signed-off-by: Jan Kara <jack@suse.cz>
---
kernel/audit_tree.c | 77 ++++++++++++++++++++++++++++-------------------------
1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 145e8c92dd31..82b27da7031c 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -332,28 +332,18 @@ static int chunk_count_trees(struct audit_chunk *chunk)
return ret;
}

-static void untag_chunk(struct node *p)
+static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry)
{
- struct audit_chunk *chunk = find_chunk(p);
- struct fsnotify_mark *entry = chunk->mark;
- struct audit_chunk *new = NULL;
+ struct audit_chunk *new;
int size;

- remove_chunk_node(chunk, p);
- fsnotify_get_mark(entry);
- spin_unlock(&hash_lock);
-
- mutex_lock(&entry->group->mark_mutex);
+ mutex_lock(&audit_tree_group->mark_mutex);
/*
* mark_mutex protects mark from getting detached and thus also from
* mark->connector->obj getting NULL.
*/
- if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
- mutex_unlock(&entry->group->mark_mutex);
- if (new)
- fsnotify_put_mark(new->mark);
- goto out;
- }
+ if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
+ goto out_mutex;

size = chunk_count_trees(chunk);
if (!size) {
@@ -363,9 +353,9 @@ static void untag_chunk(struct node *p)
list_del_rcu(&chunk->hash);
spin_unlock(&hash_lock);
fsnotify_detach_mark(entry);
- mutex_unlock(&entry->group->mark_mutex);
+ mutex_unlock(&audit_tree_group->mark_mutex);
fsnotify_free_mark(entry);
- goto out;
+ return;
}

new = alloc_chunk(size);
@@ -387,16 +377,13 @@ static void untag_chunk(struct node *p)
replace_chunk(new, chunk);
spin_unlock(&hash_lock);
fsnotify_detach_mark(entry);
- mutex_unlock(&entry->group->mark_mutex);
+ mutex_unlock(&audit_tree_group->mark_mutex);
fsnotify_free_mark(entry);
fsnotify_put_mark(new->mark); /* drop initial reference */
- goto out;
+ return;

out_mutex:
- mutex_unlock(&entry->group->mark_mutex);
-out:
- fsnotify_put_mark(entry);
- spin_lock(&hash_lock);
+ mutex_unlock(&audit_tree_group->mark_mutex);
}

/* Call with group->mark_mutex held, releases it */
@@ -579,22 +566,45 @@ static void kill_rules(struct audit_tree *tree)
}

/*
- * finish killing struct audit_tree
+ * Remove tree from chunks. If 'tagged' is set, remove tree only from tagged
+ * chunks. The function expects tagged chunks are all at the beginning of the
+ * chunks list.
*/
-static void prune_one(struct audit_tree *victim)
+static void prune_tree_chunks(struct audit_tree *victim, bool tagged)
{
spin_lock(&hash_lock);
while (!list_empty(&victim->chunks)) {
struct node *p;
+ struct audit_chunk *chunk;
+ struct fsnotify_mark *mark;
+
+ p = list_first_entry(&victim->chunks, struct node, list);
+ /* have we run out of marked? */
+ if (tagged && !(p->index & (1U<<31)))
+ break;
+ chunk = find_chunk(p);
+ mark = chunk->mark;
+ remove_chunk_node(chunk, p);
+ fsnotify_get_mark(mark);
+ spin_unlock(&hash_lock);

- p = list_entry(victim->chunks.next, struct node, list);
+ untag_chunk(chunk, mark);
+ fsnotify_put_mark(mark);

- untag_chunk(p);
+ spin_lock(&hash_lock);
}
spin_unlock(&hash_lock);
put_tree(victim);
}

+/*
+ * finish killing struct audit_tree
+ */
+static void prune_one(struct audit_tree *victim)
+{
+ prune_tree_chunks(victim, false);
+}
+
/* trim the uncommitted chunks from tree */

static void trim_marked(struct audit_tree *tree)
@@ -614,18 +624,11 @@ static void trim_marked(struct audit_tree *tree)
list_add(p, &tree->chunks);
}
}
+ spin_unlock(&hash_lock);

- while (!list_empty(&tree->chunks)) {
- struct node *node;
-
- node = list_entry(tree->chunks.next, struct node, list);
-
- /* have we run out of marked? */
- if (!(node->index & (1U<<31)))
- break;
+ prune_tree_chunks(tree, true);

- untag_chunk(node);
- }
+ spin_lock(&hash_lock);
if (!tree->root && !tree->goner) {
tree->goner = 1;
spin_unlock(&hash_lock);
--
2.16.4

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]