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

On Fri 31-07-26 00:22:41, Youngjae Kwon wrote:
> When a mark gets a new event bit, fanotify and inotify 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.
> 
> Always request recalculation when an existing mark's calculated mask
> actually changes. For fanotify, compare fsnotify_calc_mask() before and
> after normal-mask, ignore-mask, and flag updates. Keep the cached aggregate
> check for unchanged updates, and keep the flag helper's inode-reference
> handling.
> 
> 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]>
> Signed-off-by: Youngjae Kwon <[email protected]>

Looks mostly good, just one question below:

>  	spin_lock(&fsn_mark->lock);
> +	old_mask = fsnotify_calc_mask(fsn_mark);
>  	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS))
>  		fsn_mark->mask |= mask;
>  	else
>  		fsn_mark->ignore_mask |= mask;
>  
> -	recalc = fsnotify_calc_mask(fsn_mark) &
> -		~fsnotify_conn_mask(fsn_mark->connector);
> -
> -	recalc |= fanotify_mark_update_flags(fsn_mark, fan_flags);
> +	recalc = fanotify_mark_update_flags(fsn_mark, fan_flags);
> +	new_mask = fsnotify_calc_mask(fsn_mark);
> +	recalc |= old_mask != new_mask;
> +	recalc |= new_mask & ~fsnotify_conn_mask(fsn_mark->connector);

How could this last line ever change anything? If old_mask == new_mask,
then it would look like a bug that some bit from the new_mask (and thus
old_mask) isn't already included in the aggregate connector mask?

I can fix this up on commit but I want to understand why you've kept the
line here just in case I'm missing something...

								Honza

>  	spin_unlock(&fsn_mark->lock);
>  
>  	return recalc;
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index ed37491c1..c44f2daa1 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -565,17 +565,8 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  	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);
> -
> -	}
> +	if (old_mask != new_mask)
> +		fsnotify_recalc_mask(fsn_mark->connector);
>  
>  	/* return the wd */
>  	ret = i_mark->wd;
> -- 
> 2.50.1
-- 
Jan Kara <[email protected]>
SUSE Labs, CR