Messages in this thread | | | From | Marco Elver <> | Date | Tue, 24 Oct 2023 09:49:32 +0200 | Subject | Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount |
| |
On Tue, 24 Oct 2023 at 09:44, Dominique Martinet <asmadeus@codewreck.org> wrote: > > Marco Elver wrote on Tue, Oct 24, 2023 at 09:12:56AM +0200: > > > 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)? > > I don't think we have any rule about this in the 9p part of the tree, > looking around there seem to be more comments without '.' than with, but > it's never too late to start... I'll add some in a v2 after we've agreed > with the rest.
Sounds good. I think if there's 1 short sentence (1 line) comment, it's more or less optional. But I'd insist on punctuation as soon as there are 2 or more sentences.
> > > > > + * 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. > > I guess it depends on what we call 'safe' here: if we agree the worst > thing that can happen is weird flags being set when we didn't request > them and socket operations behaving oddly (of the level of block when > they shouldn't), we don't care because there's no way to make concurrent > usage of the fd work anyway.
Yes, that's reasonable.
> If it's possible to get an invalid value there such that a socket > operation ends up executing user-controlled code somewhere, then we've > got a bigger problem and we should take some lock (presumably the same > lock fcntl(F_SETFD) is taking, as that's got more potential for breakage > than another mount in my opinon) > > > The case that syzbot found was 2 concurrent mount. Is that also disallowed? > > Yes, there's no way you'll get a working filesystem out of two mounts > using the same fd as the protocol has no muxing > > > 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." > > I'd word this as much likely to break, how about something like this? > > /* Prevent workers from hanging on IO when fd is a pipe. > * It's technically possible for userspace or concurrent mounts to > * modify this flag concurrently, which will likely result in a > * broken filesystem. However, just having bad flags here should > * not crash the kernel or cause any other sort of bug, so mark this > * particular data race as intentional so that tooling (like KCSAN) > * can allow it and detect further problems. > */
I think this sounds much clearer. Thanks!
| |