| Message ID | 345de429-a88b-7097-d177-adecf9fed342@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | 9p/trans_fd: always use O_NONBLOCK read/write | expand |
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@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Tested-by: syzbot <syzbot+8b41a1365f1106fd0f33@syzkaller.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@crudebyte.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;
Christian Schoenebeck wrote on Thu, Oct 06, 2022 at 04:55:23PM +0200: > > 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 */ Good point, I've sneaked in this comment: /* prevent workers from hanging on IO when fd is a pipe */ https://github.com/martinetd/linux/commit/ef575281b21e9a34dfae544a187c6aac2ae424a9 > Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> Thank you! -- Dominique
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; 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;