[PATCH] fsnotify: Avoid data race between fsnotify_recalc_mask() and fsnotify_object_watched()

[PATCH] fsnotify: Avoid data race between fsnotify_recalc_mask() and fsnotify_object_watched()

[Jan Kara]

When __fsnotify_recalc_mask() recomputes the mask on the watched object,
the compiler can "optimize" the code to perform partial updates to the
mask (including zeroing it at the beginning). Thus places checking
the object mask without conn->lock such as fsnotify_object_watched()
could see invalid states of the mask. Make sure the mask update is
performed by one memory store using WRITE_ONCE().

Reported-by: syzbot+701037856c25b143f1ad@syzkaller.appspotmail.com
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Link: https://lore.kernel.org/all/CACT4Y+Zk0ohwwwHSD63U2-PQ=UuamXczr1mKBD6xtj2dyYKBvA@mail.gmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/mark.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

I plan to merge this fix through my tree.

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index c3eefa70633c..74a8a8ed42ff 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -245,7 +245,11 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		    !(mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF))
 			want_iref = true;
 	}
-	*fsnotify_conn_mask_p(conn) = new_mask;
+	/*
+	 * We use WRITE_ONCE() to prevent silly compiler optimizations from
+	 * confusing readers not holding conn->lock with partial updates.
+	 */
+	WRITE_ONCE(*fsnotify_conn_mask_p(conn), new_mask);
 
 	return fsnotify_update_iref(conn, want_iref);
 }
-- 
2.35.3

Re: [PATCH] fsnotify: Avoid data race between fsnotify_recalc_mask() and fsnotify_object_watched()

[Jan Kara]

On Mon 15-07-24 14:36:10, Jan Kara wrote:
> When __fsnotify_recalc_mask() recomputes the mask on the watched object,
> the compiler can "optimize" the code to perform partial updates to the
> mask (including zeroing it at the beginning). Thus places checking
> the object mask without conn->lock such as fsnotify_object_watched()
> could see invalid states of the mask. Make sure the mask update is
> performed by one memory store using WRITE_ONCE().
> 
> Reported-by: syzbot+701037856c25b143f1ad@syzkaller.appspotmail.com
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Link: https://lore.kernel.org/all/CACT4Y+Zk0ohwwwHSD63U2-PQ=UuamXczr1mKBD6xtj2dyYKBvA@mail.gmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>

Apparently my brain is still in vacation mode and although this will
probably be good enough to stop the data race from happening, it will not
be probably good enough for KCSAN as WRITE_ONCE() should better be paired
with READ_ONCE(). I'll send v2.

								Honza

> ---
>  fs/notify/mark.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> I plan to merge this fix through my tree.
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index c3eefa70633c..74a8a8ed42ff 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -245,7 +245,11 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
>  		    !(mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF))
>  			want_iref = true;
>  	}
> -	*fsnotify_conn_mask_p(conn) = new_mask;
> +	/*
> +	 * We use WRITE_ONCE() to prevent silly compiler optimizations from
> +	 * confusing readers not holding conn->lock with partial updates.
> +	 */
> +	WRITE_ONCE(*fsnotify_conn_mask_p(conn), new_mask);
>  
>  	return fsnotify_update_iref(conn, want_iref);
>  }
> -- 
> 2.35.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR