[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4870107.4IDB3aycit@silver>
Date: Thu, 06 Oct 2022 16:55:23 +0200
From: Christian Schoenebeck <linux_oss@...debyte.com>
To: Dominique Martinet <asmadeus@...ewreck.org>,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Eric Van Hensbergen <ericvh@...il.com>,
Latchesar Ionkov <lucho@...kov.net>,
syzbot <syzbot+8b41a1365f1106fd0f33@...kaller.appspotmail.com>,
v9fs-developer@...ts.sourceforge.net,
syzkaller-bugs@...glegroups.com, netdev@...r.kernel.org
Subject: Re: [PATCH] 9p/trans_fd: always use O_NONBLOCK read/write
On Freitag, 26. August 2022 17:27:46 CEST Tetsuo Handa wrote:
> syzbot is reporting hung task at p9_fd_close() [1], for p9_mux_poll_stop()
> from p9_conn_destroy() from p9_fd_close() is failing to interrupt already
> started kernel_read() from p9_fd_read() from p9_read_work() and/or
> kernel_write() from p9_fd_write() from p9_write_work() requests.
>
> Since p9_socket_open() sets O_NONBLOCK flag, p9_mux_poll_stop() does not
> need to interrupt kernel_read()/kernel_write(). However, since p9_fd_open()
> does not set O_NONBLOCK flag, but pipe blocks unless signal is pending,
> p9_mux_poll_stop() needs to interrupt kernel_read()/kernel_write() when
> the file descriptor refers to a pipe. In other words, pipe file descriptor
> needs to be handled as if socket file descriptor.
>
> We somehow need to interrupt kernel_read()/kernel_write() on pipes.
>
> A minimal change, which this patch is doing, is to set O_NONBLOCK flag
> from p9_fd_open(), for O_NONBLOCK flag does not affect reading/writing
> of regular files. But this approach changes O_NONBLOCK flag on userspace-
> supplied file descriptors (which might break userspace programs), and
> O_NONBLOCK flag could be changed by userspace. It would be possible to set
> O_NONBLOCK flag every time p9_fd_read()/p9_fd_write() is invoked, but still
> remains small race window for clearing O_NONBLOCK flag.
>
> If we don't want to manipulate O_NONBLOCK flag, we might be able to
> surround kernel_read()/kernel_write() with set_thread_flag(TIF_SIGPENDING)
> and recalc_sigpending(). Since p9_read_work()/p9_write_work() works are
> processed by kernel threads which process global system_wq workqueue,
> signals could not be delivered from remote threads when p9_mux_poll_stop()
> from p9_conn_destroy() from p9_fd_close() is called. Therefore, calling
> set_thread_flag(TIF_SIGPENDING)/recalc_sigpending() every time would be
> needed if we count on signals for making kernel_read()/kernel_write()
> non-blocking.
>
> Link: https://syzkaller.appspot.com/bug?extid=8b41a1365f1106fd0f33 [1]
> Reported-by: syzbot <syzbot+8b41a1365f1106fd0f33@...kaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+8b41a1365f1106fd0f33@...kaller.appspotmail.com>
> ---
> Although syzbot tested that this patch solves hung task problem, syzbot
> cannot verify that this patch will not break functionality of p9 users.
> Please test before applying this patch.
>
> net/9p/trans_fd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
I would also prefer this simpler v1 instead of v2 for now. One nitpicking ...
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index e758978b44be..9870597da583 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -821,11 +821,13 @@ 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;
> + ts->rd->f_flags |= O_NONBLOCK;
... I think this deserves a short comment like:
/* prevent hung task with pipes */
Anyway,
Reviewed-by: Christian Schoenebeck <linux_oss@...debyte.com>
> 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;
>
> client->trans = ts;
> client->status = Connected;
Powered by blists - more mailing lists