linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Paul Moore <paul@paul-moore.com>
Cc: jack@suse.cz, linux-audit@redhat.com,
	linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
	rgb@redhat.com, amir73il@gmail.com
Subject: Re: [PATCH 09/10] audit: Allocate fsnotify mark independently of chunk
Date: Tue, 4 Sep 2018 16:03:07 +0200	[thread overview]
Message-ID: <20180904140307.GH9444@quack2.suse.cz> (raw)
In-Reply-To: <CAHC9VhTtEwhXHCmQFKXVsQaCQvMWtn=oUk3H97Pg3TuZqhW03A@mail.gmail.com>

On Fri 27-07-18 00:47:37, Paul Moore wrote:
> 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.

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@suse.com>
SUSE Labs, CR

  reply	other threads:[~2018-09-04 18:28 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-10 10:02 [PATCH 0/10 v2] audit: Fix various races when tagging and untagging mounts Jan Kara
2018-07-10 10:02 ` [PATCH 01/10] audit_tree: Remove mark->lock locking Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-09-04  9:53     ` Jan Kara
2018-07-10 10:02 ` [PATCH 02/10] audit: Fix possible spurious -ENOSPC error Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-09-04 10:00     ` Jan Kara
2018-07-10 10:02 ` [PATCH 03/10] audit: Fix possible tagging failures Jan Kara
2018-07-10 10:02 ` [PATCH 04/10] audit: Embed key into chunk Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-07-10 10:02 ` [PATCH 05/10] audit: Make hash table insertion safe against concurrent lookups Jan Kara
2018-07-10 10:02 ` [PATCH 06/10] audit: Factor out chunk replacement code Jan Kara
2018-07-11  7:58   ` Amir Goldstein
2018-07-11  8:26     ` Jan Kara
2018-07-11  9:01       ` Amir Goldstein
2018-07-11  9:23         ` Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-07-10 10:02 ` [PATCH 07/10] audit: Remove pointless check in insert_hash() Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-07-10 10:02 ` [PATCH 08/10] audit: Provide helper for dropping mark's chunk reference Jan Kara
2018-07-10 10:02 ` [PATCH 09/10] audit: Allocate fsnotify mark independently of chunk Jan Kara
2018-07-11  8:57   ` Amir Goldstein
2018-07-11 10:48     ` Amir Goldstein
2018-07-16 15:13       ` Jan Kara
2018-07-27  4:47   ` Paul Moore
2018-09-04 14:03     ` Jan Kara [this message]
2018-09-04 14:07       ` Jan Kara
2018-07-10 10:02 ` [PATCH 10/10] audit: Replace chunk attached to mark instead of replacing mark Jan Kara
2018-07-11 14:17   ` Amir Goldstein
2018-07-27  4:47   ` Paul Moore
2018-09-04 14:11     ` Jan Kara
2018-07-10 10:02 ` [PATCH 11/10 TESTSUITE] audit_testsuite: Add stress test for tree watches Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180904140307.GH9444@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=linux-audit@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=rgb@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).