Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount

[Marco Elver]

On Tue, 24 Oct 2023 at 01:37, Dominique Martinet <asmadeus@xxxxxxxxxxxxx> wrote:
>
> From: Marco Elver <elver@xxxxxxxxxx>
>
> syzbot reported:
>
>  | BUG: KCSAN: data-race in p9_fd_create / p9_fd_create
>  |
>  | read-write to 0xffff888130fb3d48 of 4 bytes by task 15599 on cpu 0:
>  |  p9_fd_open net/9p/trans_fd.c:842 [inline]
>  |  p9_fd_create+0x210/0x250 net/9p/trans_fd.c:1092
>  |  p9_client_create+0x595/0xa70 net/9p/client.c:1010
>  |  v9fs_session_init+0xf9/0xd90 fs/9p/v9fs.c:410
>  |  v9fs_mount+0x69/0x630 fs/9p/vfs_super.c:123
>  |  legacy_get_tree+0x74/0xd0 fs/fs_context.c:611
>  |  vfs_get_tree+0x51/0x190 fs/super.c:1519
>  |  do_new_mount+0x203/0x660 fs/namespace.c:3335
>  |  path_mount+0x496/0xb30 fs/namespace.c:3662
>  |  do_mount fs/namespace.c:3675 [inline]
>  |  __do_sys_mount fs/namespace.c:3884 [inline]
>  |  [...]
>  |
>  | read-write to 0xffff888130fb3d48 of 4 bytes by task 15563 on cpu 1:
>  |  p9_fd_open net/9p/trans_fd.c:842 [inline]
>  |  p9_fd_create+0x210/0x250 net/9p/trans_fd.c:1092
>  |  p9_client_create+0x595/0xa70 net/9p/client.c:1010
>  |  v9fs_session_init+0xf9/0xd90 fs/9p/v9fs.c:410
>  |  v9fs_mount+0x69/0x630 fs/9p/vfs_super.c:123
>  |  legacy_get_tree+0x74/0xd0 fs/fs_context.c:611
>  |  vfs_get_tree+0x51/0x190 fs/super.c:1519
>  |  do_new_mount+0x203/0x660 fs/namespace.c:3335
>  |  path_mount+0x496/0xb30 fs/namespace.c:3662
>  |  do_mount fs/namespace.c:3675 [inline]
>  |  __do_sys_mount fs/namespace.c:3884 [inline]
>  |  [...]
>  |
>  | value changed: 0x00008002 -> 0x00008802
>
> Within p9_fd_open(), O_NONBLOCK is added to f_flags of the read and
> write files. This may happen concurrently if e.g. mounting process
> modifies the fd in another thread.
>
> Mark the plain read-modify-writes as intentional data-races, with the
> assumption that the result of executing the accesses concurrently will
> always result in the same result despite the accesses themselves not
> being atomic.
>
> Reported-by: syzbot+e441aeeb422763cc5511@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/ZO38mqkS0TYUlpFp@xxxxxxxxxxxxxxxx
> Signed-off-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> ---
>
> Hi Marco, sorry for taking ages to process this patch. I've reworded the
> commit message a bit and added a comment, so given this has your name on
> it please have a look.
> I'm planning to send this to Linus during the merge window next week

No worries, and thank you!

>  net/9p/trans_fd.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index f226953577b2..d89c88986950 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -836,14 +836,16 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
>                 goto out_free_ts;
>         if (!(ts->rd->f_mode & FMODE_READ))
>                 goto out_put_rd;
> -       /* prevent workers from hanging on IO when fd is a pipe */
> -       ts->rd->f_flags |= O_NONBLOCK;
> +       /* Prevent workers from hanging on IO when fd is a pipe

Add '.' at end of sentence(s)?

> +        * We don't support userspace messing with the fd after passing it
> +        * to mount, so flag possible data race for KCSAN */

The comment should explain why the data race is safe, independent of
KCSAN. I still don't quite get why it's safe.

The case that syzbot found was 2 concurrent mount. Is that also disallowed?

Maybe something like: "We don't support userspace messing with the fd
after passing it to the first mount. While it's not officially
supported, concurrent modification of flags is unlikely to break this
code. So that tooling (like KCSAN) knows about it, mark them as
intentional data races."

> +       data_race(ts->rd->f_flags |= O_NONBLOCK);
>         ts->wr = fget(wfd);
>         if (!ts->wr)
>                 goto out_put_rd;
>         if (!(ts->wr->f_mode & FMODE_WRITE))
>                 goto out_put_wr;
> -       ts->wr->f_flags |= O_NONBLOCK;
> +       data_race(ts->wr->f_flags |= O_NONBLOCK);
>
>         client->trans = ts;
>         client->status = Connected;
> --
> 2.41.0
>