Re: [PATCH 1/2] fanotify: store fsid in mark instead of in connector

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Christian Brauner <brauner@kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/2] fanotify: store fsid in mark instead of in connector
Date: Thu, 30 Nov 2023 15:25:39 +0100	[thread overview]
Message-ID: <20231130142539.g4hhcsk4hk2oimdv@quack3> (raw)
In-Reply-To: <20231118183018.2069899-2-amir73il@gmail.com>

On Sat 18-11-23 20:30:17, Amir Goldstein wrote:
> Some filesystems like fuse and nfs have zero or non-unique fsid.
> We would like to avoid reporting ambiguous fsid in events, so we need
> to avoid marking objects with same fsid and different sb.
> 
> To make this easier to enforce, store the fsid in the marks of the group
> instead of in the shared conenctor.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Very nice! I like the result. Just a few nits below.

> +static inline __kernel_fsid_t *fanotify_mark_fsid(struct fsnotify_mark *mark)
> +{
> +	return &FANOTIFY_MARK(mark)->fsid;
> +}

I guess, there's no big win in using this helper compared to using
FANOTIFY_MARK(mark)->fsid so I'd just drop this helper.

> @@ -530,6 +528,7 @@ struct fsnotify_mark {
>  #define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY	0x0100
>  #define FSNOTIFY_MARK_FLAG_NO_IREF		0x0200
>  #define FSNOTIFY_MARK_FLAG_HAS_IGNORE_FLAGS	0x0400
> +#define FSNOTIFY_MARK_FLAG_HAS_FSID		0x0800
>  	unsigned int flags;		/* flags [mark->lock] */
>  };

So this flag is in fact private to fanotify notification framework. Either
we could just drop this flag and use

  FANOTIFY_MARK(mark)->fsid[0] != 0 || FANOTIFY_MARK(mark)->fsid[1] != 0

instead or we could at least add a comment that this flags is in fact
private to fanotify?

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