[syzbot] [net?] [v9fs?] KCSAN: data-race in p9_fd_create / p9_fd_create (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 53663f4103ff Merge tag 'nfs-for-6.5-2' of git://git.linux-..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=103fc55fa80000
kernel config: https://syzkaller.appspot.com/x/.config?x=f12c32a009b80107
dashboard link: https://syzkaller.appspot.com/bug?extid=e441aeeb422763cc5511
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/f32101f0e8a2/disk-53663f41.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/2e577e9d7daf/vmlinux-53663f41.xz
kernel image: https://storage.googleapis.com/syzbot-assets/af8ae7d4d06e/bzImage-53663f41.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e441ae...@syzkaller.appspotmail.com

==================================================================
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]
__se_sys_mount+0x27f/0x2d0 fs/namespace.c:3861
__x64_sys_mount+0x67/0x80 fs/namespace.c:3861
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

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]
__se_sys_mount+0x27f/0x2d0 fs/namespace.c:3861
__x64_sys_mount+0x67/0x80 fs/namespace.c:3861
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

value changed: 0x00008002 -> 0x00008802

Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 15563 Comm: syz-executor.3 Not tainted 6.5.0-rc7-syzkaller-00013-g53663f4103ff #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Dominique Martinet]

Yes well that doesn't seem too hard to hit, both threads are just
setting O_NONBLOCK to the same fd in parallel (0x800 is 04000,
O_NONBLOCK)

I'm not quite sure why that'd be a problem; and I'm also pretty sure
that wouldn't work anyway (9p has no muxing or anything that'd allow
sharing the same fd between multiple mounts)

Can this be flagged "don't care" ?

--
Dominique Martinet | Asmadeus

[Marco Elver]

On Tue, Aug 29, 2023 at 07:57PM +0900, Dominique Martinet wrote:
[...]

> Yes well that doesn't seem too hard to hit, both threads are just
> setting O_NONBLOCK to the same fd in parallel (0x800 is 04000,
> O_NONBLOCK)
>
> I'm not quite sure why that'd be a problem; and I'm also pretty sure
> that wouldn't work anyway (9p has no muxing or anything that'd allow
> sharing the same fd between multiple mounts)
>
> Can this be flagged "don't care" ?

If it's an intentional data race, it could be marked data_race() [1].

However, staring at this code for a bit, I wonder why the f_flags are
set on open, and not on initialization somewhere...

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt

Anyway, a patch like the below would document that the data race is
intended and we assume that there is no way (famous last words) the
compiler or the CPU can mess it up (and KCSAN won't report it again).

------ >8 ------

From: Marco Elver <el...@google.com>
Date: Tue, 29 Aug 2023 15:48:58 +0200
Subject: [PATCH] 9p: Annotate data-racy writes to file::f_flags

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. 2 tasks mount the same
filesystem.

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+e441ae...@syzkaller.appspotmail.com
Signed-off-by: Marco Elver <el...@google.com>
---
net/9p/trans_fd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 00b684616e8d..9b01e15a758b 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -833,13 +833,13 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
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;
+ 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.42.0.rc2.253.gd59a3bf2b4-goog

[Dominique Martinet]

Marco Elver wrote on Tue, Aug 29, 2023 at 04:11:38PM +0200:
> On Tue, Aug 29, 2023 at 07:57PM +0900, Dominique Martinet wrote:
> [...]
> > Yes well that doesn't seem too hard to hit, both threads are just
> > setting O_NONBLOCK to the same fd in parallel (0x800 is 04000,
> > O_NONBLOCK)
> >
> > I'm not quite sure why that'd be a problem; and I'm also pretty sure
> > that wouldn't work anyway (9p has no muxing or anything that'd allow
> > sharing the same fd between multiple mounts)
> >
> > Can this be flagged "don't care" ?
>
> If it's an intentional data race, it could be marked data_race() [1].

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt

Thanks!

> However, staring at this code for a bit, I wonder why the f_flags are
> set on open, and not on initialization somewhere...

This open is during the mount initialization (mount/p9_client_create,
full path in the stack); there's no more initialization-ish code we
have.
The problem here is that we allow to pass any old arbitrary fd, so the
user can open their fd how they want and abuse mount to use it on
multiple mounts, even if that has no way of working (as I mentionned,
there's no control flow at all -- you'll create two completely separate
client state machines that'll both try to read and/or write (separate
fds) on the same fd, and it'll all get jumbled up.

>
> Anyway, a patch like the below would document that the data race is
> intended and we assume that there is no way (famous last words) the
> compiler or the CPU can mess it up (and KCSAN won't report it again).

That's good enough for me as my position really is just "don't do
that"... Would that also protect from syzcaller sending the fd to mount
on one side, and calling fcntl(F_SETFL) on the side?
At this rate we might as well just take the file's f_lock as setfl does,
but perhaps there's a way to steal the fd from userspace somehow?

It's not just "don't use this fd for another mount", it really is "don't
use this fd anymore while it is used by a mount".

This is made complicated that we only want to steal half of the fd, you
could imagine a weird setup like this:

┌────────────────────────────────────┐ ┌─────────────────┐
│ │ │ │
│ │ │ kernel client │
│ fd3 tcp to server │ │ │
│ write end ◄─────────────────┼─────────┤ │
│ │ │ │
│ read end ──┐ │ │ │
│ │ │ │ │
│ fd4 pipeA │ MITMing... │ │ │
│ │ │ │ │
│ write end ◄─┘ │ │ │
│ │ │ │
│ fd5 pipeB │ │ │
│ │ │ │
│ read end ───────────────────┼────────►│ │
│ │ │ │
│ │ │ │
└────────────────────────────────────┘ └─────────────────┘

I'm not sure we actually want to support something like that, but it's
currently possible and making mount act like close() on the fd would
break this... :|

So, yeah, well; this is one of these "please don't do this" that
syzcaller has no way of knowing about; it's good to test (please don't
do this has no security guarantee so the kernel shouldn't blow up!),
but if the only fallout is breakage then yeah data_race() is fine.

Compilers and/or CPU might be able to blow this out of proportion, but
hopefully they won't go around modifying another unrelated value in
memory somewhere, and we do fdget so it shouldn't turn into a UAF, so I
guess it's fine?... Just taking f_lock here won't solve anything and
might give the impression we support concurrent uses.


Sorry for rambling, and thanks for the patch; I'm not sure if Eric has
anything planned for next cycle but either of us can take it and call it
a day.
--
Dominique Martinet | Asmadeus

[Marco Elver]

No, data_race() is only for marking intentional data races. In a
production kernel, it's a no-op (generated code is identical). In a
KCSAN kernel, it will make the tool not report such data races.

syzkaller doesn't care, and can still produce such programs (so that
other bug detectors can still see an issue if there is one somewhere).

> At this rate we might as well just take the file's f_lock as setfl does,
> but perhaps there's a way to steal the fd from userspace somehow?
>
> It's not just "don't use this fd for another mount", it really is "don't
> use this fd anymore while it is used by a mount".
>
> This is made complicated that we only want to steal half of the fd, you
> could imagine a weird setup like this:
>
> ┌────────────────────────────────────┐ ┌─────────────────┐
> │ │ │ │
> │ │ │ kernel client │
> │ fd3 tcp to server │ │ │
> │ write end ◄─────────────────┼─────────┤ │
> │ │ │ │
> │ read end ──┐ │ │ │
> │ │ │ │ │
> │ fd4 pipeA │ MITMing... │ │ │
> │ │ │ │ │
> │ write end ◄─┘ │ │ │
> │ │ │ │
> │ fd5 pipeB │ │ │
> │ │ │ │
> │ read end ───────────────────┼────────►│ │
> │ │ │ │
> │ │ │ │
> └────────────────────────────────────┘ └─────────────────┘
>
> I'm not sure we actually want to support something like that, but it's
> currently possible and making mount act like close() on the fd would
> break this... :|
>
> So, yeah, well; this is one of these "please don't do this" that
> syzcaller has no way of knowing about; it's good to test (please don't
> do this has no security guarantee so the kernel shouldn't blow up!),
> but if the only fallout is breakage then yeah data_race() is fine.

Right, if the only breakage is some corruption of the particular file in
user space, and the kernel is still in a good state, then I think this
is fine. However, if the kernel can potentially crash or corrupt
completely unrelated data, it may be a problem.

> Compilers and/or CPU might be able to blow this out of proportion, but
> hopefully they won't go around modifying another unrelated value in
> memory somewhere, and we do fdget so it shouldn't turn into a UAF, so I
> guess it's fine?...

No, the kernel strongly assumes "locally undefined behaviour" for data
races in the worst case, i.e. some garbage value being written into
f_flags. To guard against that we'd have to use READ_ONCE/WRITE_ONCE.

But given the best-effort nature of this based on "don't do this" i.e.
not really supported, data_race() is probably more than enough. You may
want to amend the patch I sent to clarify that (I wasn't aware of it).

> Just taking f_lock here won't solve anything and
> might give the impression we support concurrent uses.
>
>
> Sorry for rambling, and thanks for the patch; I'm not sure if Eric has
> anything planned for next cycle but either of us can take it and call it
> a day.

Thanks for the explanation!

[syzbot]

Auto-closing this bug as obsolete.
Crashes did not happen for a while, no reproducer and no activity.