Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Tue, 24 Oct 2023 16:44:28 +0900
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Marco Elver <elver@google.com>
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
Subject: Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd
 mount
Message-ID: <ZTd13Eh1ryuyQqyr@codewreck.org>
References: <20231023233704.1185154-1-asmadeus@codewreck.org>
 <20231023233704.1185154-2-asmadeus@codewreck.org>
 <CANpmjNOUSci41NssMKgrNjC5P0doPWPekiizMOfjrr11CV-ogQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CANpmjNOUSci41NssMKgrNjC5P0doPWPekiizMOfjrr11CV-ogQ@mail.gmail.com>
Precedence: bulk

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.

> 
> > +        * 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.
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.
	 */

Thanks,
-- 
Dominique Martinet | Asmadeus