Re: [PATCH v3] fsnotify: Fix stale object mask after concurrent mark updates

On Sun 02-08-26 10:58:00, Youngjae Kwon wrote:
> When a mark gets a new event bit, fanotify and inotify may avoid
> recalculating the object mask if the cached aggregate already contains that
> bit. This is racy with a recalculation triggered by a concurrent update to
> another mark on the same connector.
> 
> The concurrent scan can read the mark before the new bit is added, while
> the updater reads the old aggregate before that scan publishes its result.
> The updater then skips recalculation and the scan publishes a mask without
> the bit, leaving the object mask stale after both updates complete.
> 
> This can be reproduced with two fanotify groups watching the same inode:
> one thread removes FAN_MODIFY from one existing mark while another thread
> adds FAN_MODIFY to the other mark. After both fanotify_mark() calls return,
> writes can fail to produce FAN_MODIFY for the group whose mark now contains
> the bit. This was reproduced on an unmodified v6.12.95 kernel. The
> equivalent inotify interleaving loses IN_MODIFY events.
> 
> For normal fanotify additions, recalculate whenever the raw mark mask
> changes. The normal mask is not cleared asynchronously, so an unchanged
> addition cannot introduce missing interest. Always recalculate ignore-mask
> updates because FS_MODIFY handling may clear the ignore mask without taking
> mark->lock, making snapshot comparisons unreliable.
> 
> Always recalculate after updating an existing inotify watch. Its replace
> path temporarily sets mark->mask to zero, so a concurrent scan can observe
> zero even when the old and final masks are equal. Assigning the replacement
> mask directly would avoid the transient zero, but existing-watch updates
> are infrequent, so unconditional recalculation is simpler.
> 
> Link: https://lore.kernel.org/all/CACwKKmCZdiZDoFuYm6LZhQ=XvHPk0fNKH=X3LmoXMqakYqJaNw@mail.gmail.com/
> Fixes: 63c882a05416 ("inotify: reimplement inotify using fsnotify")
> Fixes: 912ee3946c5e ("fanotify: do not call fanotify_update_object_mask in fanotify_add_mark")
> Cc: [email protected] # needs adjustments for <= 7.0
> Suggested-by: Jan Kara <[email protected]>
> Suggested-by: Amir Goldstein <[email protected]>
> Signed-off-by: Youngjae Kwon <[email protected]>

Thanks the patch looks good to me, I've added it to my tree. Amir, are you
happy with the changes like this?

								Honza

> ---
> Changes in v3:
> - Preserve the normal fanotify duplicate-add fast path by recalculating
>   only when the raw normal mask actually changes.
> - Recalculate fanotify ignore-mask updates unconditionally because the
>   ignore mask may be cleared concurrently without mark->lock.
> - Keep v2's unconditional recalculation for existing inotify watch updates.
> 
> I will send backports for stable kernels through 7.0.y after this lands.
> 
>  fs/notify/fanotify/fanotify_user.c | 12 +++++++-----
>  fs/notify/inotify/inotify_user.c   | 15 +--------------
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 9ee373ff5..9c27db2b7 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1321,16 +1321,18 @@ static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
>  static bool fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
>  				      __u32 mask, unsigned int fan_flags)
>  {
> +	__u32 old_mask;
>  	bool recalc;
>  
>  	spin_lock(&fsn_mark->lock);
> -	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS))
> +	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS)) {
> +		old_mask = fsn_mark->mask;
>  		fsn_mark->mask |= mask;
> -	else
> +		recalc = old_mask != fsn_mark->mask;
> +	} else {
>  		fsn_mark->ignore_mask |= mask;
> -
> -	recalc = fsnotify_calc_mask(fsn_mark) &
> -		~fsnotify_conn_mask(fsn_mark->connector);
> +		recalc = true;
> +	}
>  
>  	recalc |= fanotify_mark_update_flags(fsn_mark, fan_flags);
>  	spin_unlock(&fsn_mark->lock);
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index ed37491c1..5f19c24ec 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -539,7 +539,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  {
>  	struct fsnotify_mark *fsn_mark;
>  	struct inotify_inode_mark *i_mark;
> -	__u32 old_mask, new_mask;
>  	int replace = !(arg & IN_MASK_ADD);
>  	int create = (arg & IN_MASK_CREATE);
>  	int ret;
> @@ -555,27 +554,15 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  	i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
>  
>  	spin_lock(&fsn_mark->lock);
> -	old_mask = fsn_mark->mask;
>  	if (replace) {
>  		fsn_mark->mask = 0;
>  		fsn_mark->flags &= ~INOTIFY_MARK_FLAGS;
>  	}
>  	fsn_mark->mask |= inotify_arg_to_mask(inode, arg);
>  	fsn_mark->flags |= inotify_arg_to_flags(arg);
> -	new_mask = fsn_mark->mask;
>  	spin_unlock(&fsn_mark->lock);
>  
> -	if (old_mask != new_mask) {
> -		/* more bits in old than in new? */
> -		int dropped = (old_mask & ~new_mask);
> -		/* more bits in this fsn_mark than the inode's mask? */
> -		int do_inode = (new_mask & ~READ_ONCE(inode->i_fsnotify_mask));
> -
> -		/* update the inode with this new fsn_mark */
> -		if (dropped || do_inode)
> -			fsnotify_recalc_mask(fsn_mark->connector);
> -
> -	}
> +	fsnotify_recalc_mask(fsn_mark->connector);
>  
>  	/* return the wd */
>  	ret = i_mark->wd;
> -- 
> 2.50.1
-- 
Jan Kara <[email protected]>
SUSE Labs, CR