Re: [PATCH 09/10] audit: Allocate fsnotify mark independently of chunk
On Fri 27-07-18 00:47:37, Paul Moore wrote:
On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack(a)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(a)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.
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?
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.
I want the inline function for type safety. All caps for this function is a
tradition from filesystem space where all FOO_I(inode) or FOO_SB(sb)
helpers are all caps. I then inherited it for fs/notify/. So it is
consistent with some code. But if you still don't like all caps, I can
change the name... Hmm, given your comment below, I guess I'll just change
the name since it will have exactly two call sites.
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)?
Good point. All but one - audit_tree_destroy_watch(). I'll create a helper
for this.
> +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?
No, previously mark got allocated as a part of alloc_chunk() as it was
embedded there. OK, I can call this alloc_mark().
[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).]
I agree some of the names are tad bit confusing. But not that I'd have
great idea how to improve that.
Honza
--
Jan Kara <jack(a)suse.com>
SUSE Labs, CR