Re: [PATCH 10/10] audit: Replace chunk attached to mark instead of replacing mark

On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack(a)suse.cz&gt; wrote: > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index aec9b27a20ff..40f61de77dd0 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -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) > +{ > + 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?

> @@ -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.

> */ > - 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.