Re: [PATCH 09/10] audit: Allocate fsnotify mark independently of chunk

[Paul Moore <paul@paul-moore.com>]

On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack@suse.cz> wrote:
> 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 <jack@suse.cz>
> ---
>  kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 14 deletions(-)

...

> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index bce3b04a365d..aec9b27a20ff 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -38,6 +38,11 @@ struct audit_chunk {
>         } owners[];
>  };
>
> +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)
>         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);
> +}

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)?

>  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);
> +}

I think you've said you've already fixed the above (thanks for the
review Amir!).

> +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;
>  }

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).]

--
paul moore
www.paul-moore.com