Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231023233704.1185154-1-asmadeus@codewreck.org>
 <20231023233704.1185154-2-asmadeus@codewreck.org> <CANpmjNOUSci41NssMKgrNjC5P0doPWPekiizMOfjrr11CV-ogQ@mail.gmail.com>
 <ZTd13Eh1ryuyQqyr@codewreck.org>
In-Reply-To: <ZTd13Eh1ryuyQqyr@codewreck.org>
From:   Marco Elver <elver@google.com>
Date:   Tue, 24 Oct 2023 09:49:32 +0200
Message-ID: <CANpmjNMGbzg7MLFY-hpA0=bt24bMkmfTE9cvSjWTVLLG20WLNg@mail.gmail.com>
Subject: Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     v9fs@lists.linux.dev, ericvh@kernel.org, linux_oss@crudebyte.com,
        lucho@ionkov.net, linux-kernel@vger.kernel.org,
        syzbot+e441aeeb422763cc5511@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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!