* [PATCH] net/9p: use a dedicated spinlock for trans_fd [not found] <2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKURA.ne.jp> @ 2022-09-04 11:29 ` Dominique Martinet 2022-09-04 13:03 ` Tetsuo Handa 2022-10-06 13:16 ` Christian Schoenebeck 0 siblings, 2 replies; 5+ messages in thread From: Dominique Martinet @ 2022-09-04 11:29 UTC (permalink / raw) To: v9fs-developer, Tetsuo Handa Cc: syzkaller-bugs, linux_oss, linux-kernel, Dominique Martinet, syzbot Shamelessly copying the explanation from Tetsuo Handa's suggested patch[1] (slightly reworded): syzbot is reporting inconsistent lock state in p9_req_put()[2], for p9_tag_remove() from p9_req_put() from IRQ context is using spin_lock_irqsave() on "struct p9_client"->lock but trans_fd (not from IRQ context) is using spin_lock(). Since the locks actually protect different things in client.c and in trans_fd.c, just replace trans_fd.c's lock by a new one specific to the transport instead of using spin_lock_irq* variants everywhere (client.c's protect the idr for tag allocations, while trans_fd.c's protects its own req list and request status field that acts as the transport's state machine) Link: https://lkml.kernel.org/r/2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKURA.ne.jp [1] Link: https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2] Reported-by: syzbot <syzbot+2f20b523930c32c160cc@syzkaller.appspotmail.com> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- Tetsuo Handa-san, thank you very much! I'm sorry for not respecting your opinion but it's been a pleasure to have submissions from someone on JST :) Both this and your previous patch only impact trans_fd which I can't test super easily, so while I've sent the patch here I'll only queue it to -next hopefully next week after I've had time to setup a compatible server again... net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index ef5760971f1e..5b4807411281 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -91,6 +91,7 @@ struct p9_poll_wait { * @mux_list: list link for mux to manage multiple connections (?) * @client: reference to client instance for this connection * @err: error state + * @req_lock: lock protecting req_list and requests statuses * @req_list: accounting for requests which have been sent * @unsent_req_list: accounting for requests that haven't been sent * @rreq: read request @@ -114,6 +115,7 @@ struct p9_conn { struct list_head mux_list; struct p9_client *client; int err; + spinlock_t req_lock; struct list_head req_list; struct list_head unsent_req_list; struct p9_req_t *rreq; @@ -189,10 +191,10 @@ static void p9_conn_cancel(struct p9_conn *m, int err) p9_debug(P9_DEBUG_ERROR, "mux %p err %d\n", m, err); - spin_lock(&m->client->lock); + spin_lock(&m->req_lock); if (m->err) { - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); return; } @@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err) list_move(&req->req_list, &cancel_list); } - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) { p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req); @@ -360,7 +362,7 @@ static void p9_read_work(struct work_struct *work) if ((m->rreq) && (m->rc.offset == m->rc.capacity)) { p9_debug(P9_DEBUG_TRANS, "got new packet\n"); m->rreq->rc.size = m->rc.offset; - spin_lock(&m->client->lock); + spin_lock(&m->req_lock); if (m->rreq->status == REQ_STATUS_SENT) { list_del(&m->rreq->req_list); p9_client_cb(m->client, m->rreq, REQ_STATUS_RCVD); @@ -369,14 +371,14 @@ static void p9_read_work(struct work_struct *work) p9_debug(P9_DEBUG_TRANS, "Ignore replies associated with a cancelled request\n"); } else { - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); p9_debug(P9_DEBUG_ERROR, "Request tag %d errored out while we were reading the reply\n", m->rc.tag); err = -EIO; goto error; } - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); m->rc.sdata = NULL; m->rc.offset = 0; m->rc.capacity = 0; @@ -454,10 +456,10 @@ static void p9_write_work(struct work_struct *work) } if (!m->wsize) { - spin_lock(&m->client->lock); + spin_lock(&m->req_lock); if (list_empty(&m->unsent_req_list)) { clear_bit(Wworksched, &m->wsched); - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); return; } @@ -472,7 +474,7 @@ static void p9_write_work(struct work_struct *work) m->wpos = 0; p9_req_get(req); m->wreq = req; - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); } p9_debug(P9_DEBUG_TRANS, "mux %p pos %d size %d\n", @@ -670,10 +672,10 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req) if (m->err < 0) return m->err; - spin_lock(&client->lock); + spin_lock(&m->req_lock); req->status = REQ_STATUS_UNSENT; list_add_tail(&req->req_list, &m->unsent_req_list); - spin_unlock(&client->lock); + spin_unlock(&m->req_lock); if (test_and_clear_bit(Wpending, &m->wsched)) n = EPOLLOUT; @@ -688,11 +690,13 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req) static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) { + struct p9_trans_fd *ts = client->trans; + struct p9_conn *m = &ts->conn; int ret = 1; p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); - spin_lock(&client->lock); + spin_lock(&m->req_lock); if (req->status == REQ_STATUS_UNSENT) { list_del(&req->req_list); @@ -700,21 +704,24 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) p9_req_put(client, req); ret = 0; } - spin_unlock(&client->lock); + spin_unlock(&m->req_lock); return ret; } static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) { + struct p9_trans_fd *ts = client->trans; + struct p9_conn *m = &ts->conn; + p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); - spin_lock(&client->lock); + spin_lock(&m->req_lock); /* Ignore cancelled request if message has been received * before lock. */ if (req->status == REQ_STATUS_RCVD) { - spin_unlock(&client->lock); + spin_unlock(&m->req_lock); return 0; } @@ -723,7 +730,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) */ list_del(&req->req_list); req->status = REQ_STATUS_FLSHD; - spin_unlock(&client->lock); + spin_unlock(&m->req_lock); + p9_req_put(client, req); return 0; @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) client->trans = ts; client->status = Connected; + spin_lock_init(&ts->conn.req_lock); return 0; @@ -866,6 +875,7 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) p->wr = p->rd = file; client->trans = p; client->status = Connected; + spin_lock_init(&p->conn.req_lock); p->rd->f_flags |= O_NONBLOCK; -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net/9p: use a dedicated spinlock for trans_fd 2022-09-04 11:29 ` [PATCH] net/9p: use a dedicated spinlock for trans_fd Dominique Martinet @ 2022-09-04 13:03 ` Tetsuo Handa 2022-10-06 13:16 ` Christian Schoenebeck 1 sibling, 0 replies; 5+ messages in thread From: Tetsuo Handa @ 2022-09-04 13:03 UTC (permalink / raw) To: Dominique Martinet, v9fs-developer Cc: syzkaller-bugs, linux_oss, linux-kernel, syzbot On 2022/09/04 20:29, Dominique Martinet wrote: > Since the locks actually protect different things in client.c and in > trans_fd.c, just replace trans_fd.c's lock by a new one specific to the > transport instead of using spin_lock_irq* variants everywhere > (client.c's protect the idr for tag allocations, while > trans_fd.c's protects its own req list and request status field > that acts as the transport's state machine) OK, I figured out what this patch changes. $ grep -nF -- '->lock' *.[ch] client.c:286: spin_lock_irq(&c->lock); client.c:293: spin_unlock_irq(&c->lock); client.c:367: spin_lock_irqsave(&c->lock, flags); client.c:369: spin_unlock_irqrestore(&c->lock, flags); client.c:816: spin_lock_irq(&clnt->lock); client.c:819: spin_unlock_irq(&clnt->lock); client.c:838: spin_lock_irqsave(&clnt->lock, flags); client.c:840: spin_unlock_irqrestore(&clnt->lock, flags); client.c:945: spin_lock_init(&clnt->lock); trans_virtio.c:139: spin_lock_irqsave(&chan->lock, flags); trans_virtio.c:151: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:268: spin_lock_irqsave(&chan->lock, flags); trans_virtio.c:287: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:296: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:303: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:474: spin_lock_irqsave(&chan->lock, flags); trans_virtio.c:515: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:524: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:532: spin_unlock_irqrestore(&chan->lock, flags); trans_virtio.c:621: spin_lock_init(&chan->lock); trans_xen.c:142: spin_lock_irqsave(&ring->lock, flags); trans_xen.c:149: spin_unlock_irqrestore(&ring->lock, flags); trans_xen.c:164: spin_unlock_irqrestore(&ring->lock, flags); trans_xen.c:314: spin_lock_init(&ring->lock); This patch changes "struct p9_client"->lock to be used for only protecting idr, as explained at * @lock: protect @fids and @reqs line. Makes sense and looks correct. > Tetsuo Handa-san, thank you very much! > I'm sorry for not respecting your opinion but it's been a pleasure to > have submissions from someone on JST :) Thank you for responding quickly. :-) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net/9p: use a dedicated spinlock for trans_fd 2022-09-04 11:29 ` [PATCH] net/9p: use a dedicated spinlock for trans_fd Dominique Martinet 2022-09-04 13:03 ` Tetsuo Handa @ 2022-10-06 13:16 ` Christian Schoenebeck 2022-10-07 1:05 ` Dominique Martinet 1 sibling, 1 reply; 5+ messages in thread From: Christian Schoenebeck @ 2022-10-06 13:16 UTC (permalink / raw) To: v9fs-developer, Tetsuo Handa, Dominique Martinet, syzbot Cc: syzkaller-bugs, linux-kernel On Sonntag, 4. September 2022 13:29:28 CEST Dominique Martinet wrote: > Shamelessly copying the explanation from Tetsuo Handa's suggested > patch[1] (slightly reworded): > syzbot is reporting inconsistent lock state in p9_req_put()[2], > for p9_tag_remove() from p9_req_put() from IRQ context is using > spin_lock_irqsave() on "struct p9_client"->lock but trans_fd > (not from IRQ context) is using spin_lock(). > > Since the locks actually protect different things in client.c and in > trans_fd.c, just replace trans_fd.c's lock by a new one specific to the > transport instead of using spin_lock_irq* variants everywhere > (client.c's protect the idr for tag allocations, while > trans_fd.c's protects its own req list and request status field > that acts as the transport's state machine) > > Link: > https://lkml.kernel.org/r/2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKUR > A.ne.jp [1] Link: > https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2] > Reported-by: syzbot <syzbot+2f20b523930c32c160cc@syzkaller.appspotmail.com> > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> > --- > Tetsuo Handa-san, thank you very much! > I'm sorry for not respecting your opinion but it's been a pleasure to > have submissions from someone on JST :) > > Both this and your previous patch only impact trans_fd which I can't > test super easily, so while I've sent the patch here I'll only queue it > to -next hopefully next week after I've had time to setup a compatible > server again... > > net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 16 deletions(-) > Late on the party, sorry. Note that you already queued a slightly different version than this patch here, anyway, one thing ... > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index ef5760971f1e..5b4807411281 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -91,6 +91,7 @@ struct p9_poll_wait { > * @mux_list: list link for mux to manage multiple connections (?) > * @client: reference to client instance for this connection > * @err: error state > + * @req_lock: lock protecting req_list and requests statuses > * @req_list: accounting for requests which have been sent > * @unsent_req_list: accounting for requests that haven't been sent > * @rreq: read request > @@ -114,6 +115,7 @@ struct p9_conn { > struct list_head mux_list; > struct p9_client *client; > int err; > + spinlock_t req_lock; > struct list_head req_list; > struct list_head unsent_req_list; > struct p9_req_t *rreq; > @@ -189,10 +191,10 @@ static void p9_conn_cancel(struct p9_conn *m, int err) > > p9_debug(P9_DEBUG_ERROR, "mux %p err %d\n", m, err); > > - spin_lock(&m->client->lock); > + spin_lock(&m->req_lock); > > if (m->err) { > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > return; > } > > @@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err) > list_move(&req->req_list, &cancel_list); > } > > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > > list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) { > p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req); > @@ -360,7 +362,7 @@ static void p9_read_work(struct work_struct *work) > if ((m->rreq) && (m->rc.offset == m->rc.capacity)) { > p9_debug(P9_DEBUG_TRANS, "got new packet\n"); > m->rreq->rc.size = m->rc.offset; > - spin_lock(&m->client->lock); > + spin_lock(&m->req_lock); > if (m->rreq->status == REQ_STATUS_SENT) { > list_del(&m->rreq->req_list); > p9_client_cb(m->client, m->rreq, REQ_STATUS_RCVD); > @@ -369,14 +371,14 @@ static void p9_read_work(struct work_struct *work) > p9_debug(P9_DEBUG_TRANS, > "Ignore replies associated with a cancelled request\n"); > } else { > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > p9_debug(P9_DEBUG_ERROR, > "Request tag %d errored out while we were reading the reply\n", > m->rc.tag); > err = -EIO; > goto error; > } > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > m->rc.sdata = NULL; > m->rc.offset = 0; > m->rc.capacity = 0; > @@ -454,10 +456,10 @@ static void p9_write_work(struct work_struct *work) > } > > if (!m->wsize) { > - spin_lock(&m->client->lock); > + spin_lock(&m->req_lock); > if (list_empty(&m->unsent_req_list)) { > clear_bit(Wworksched, &m->wsched); > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > return; > } > > @@ -472,7 +474,7 @@ static void p9_write_work(struct work_struct *work) > m->wpos = 0; > p9_req_get(req); > m->wreq = req; > - spin_unlock(&m->client->lock); > + spin_unlock(&m->req_lock); > } > > p9_debug(P9_DEBUG_TRANS, "mux %p pos %d size %d\n", > @@ -670,10 +672,10 @@ static int p9_fd_request(struct p9_client *client, > struct p9_req_t *req) if (m->err < 0) > return m->err; > > - spin_lock(&client->lock); > + spin_lock(&m->req_lock); > req->status = REQ_STATUS_UNSENT; > list_add_tail(&req->req_list, &m->unsent_req_list); > - spin_unlock(&client->lock); > + spin_unlock(&m->req_lock); > > if (test_and_clear_bit(Wpending, &m->wsched)) > n = EPOLLOUT; > @@ -688,11 +690,13 @@ static int p9_fd_request(struct p9_client *client, > struct p9_req_t *req) > > static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) > { > + struct p9_trans_fd *ts = client->trans; > + struct p9_conn *m = &ts->conn; > int ret = 1; > > p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); > > - spin_lock(&client->lock); > + spin_lock(&m->req_lock); > > if (req->status == REQ_STATUS_UNSENT) { > list_del(&req->req_list); > @@ -700,21 +704,24 @@ static int p9_fd_cancel(struct p9_client *client, > struct p9_req_t *req) p9_req_put(client, req); > ret = 0; > } > - spin_unlock(&client->lock); > + spin_unlock(&m->req_lock); > > return ret; > } > > static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) > { > + struct p9_trans_fd *ts = client->trans; > + struct p9_conn *m = &ts->conn; > + > p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); > > - spin_lock(&client->lock); > + spin_lock(&m->req_lock); > /* Ignore cancelled request if message has been received > * before lock. > */ > if (req->status == REQ_STATUS_RCVD) { > - spin_unlock(&client->lock); > + spin_unlock(&m->req_lock); > return 0; > } > > @@ -723,7 +730,8 @@ static int p9_fd_cancelled(struct p9_client *client, > struct p9_req_t *req) */ > list_del(&req->req_list); > req->status = REQ_STATUS_FLSHD; > - spin_unlock(&client->lock); > + spin_unlock(&m->req_lock); > + > p9_req_put(client, req); > > return 0; > @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd, > int wfd) > > client->trans = ts; > client->status = Connected; > + spin_lock_init(&ts->conn.req_lock); Are you sure this is the right place for spin_lock_init()? Not rather in p9_conn_create()? > return 0; > > @@ -866,6 +875,7 @@ static int p9_socket_open(struct p9_client *client, > struct socket *csocket) p->wr = p->rd = file; > client->trans = p; > client->status = Connected; > + spin_lock_init(&p->conn.req_lock); Same here. > > p->rd->f_flags |= O_NONBLOCK; The rest LGTM. Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net/9p: use a dedicated spinlock for trans_fd 2022-10-06 13:16 ` Christian Schoenebeck @ 2022-10-07 1:05 ` Dominique Martinet 2022-10-07 9:29 ` Christian Schoenebeck 0 siblings, 1 reply; 5+ messages in thread From: Dominique Martinet @ 2022-10-07 1:05 UTC (permalink / raw) To: Christian Schoenebeck Cc: v9fs-developer, Tetsuo Handa, syzbot, syzkaller-bugs, linux-kernel Christian Schoenebeck wrote on Thu, Oct 06, 2022 at 03:16:40PM +0200: > > net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++---------------- > > 1 file changed, 26 insertions(+), 16 deletions(-) > > Late on the party, sorry. Note that you already queued a slightly different > version than this patch here, anyway, one thing ... Did I? Oh, I apparently reworded the commit message a bit, sorry: (where HEAD is this patch and 1622... the queued patch) $ git range-diff HEAD^- 16228c9a4215^- 1: e35fb8546af2 ! 1: 16228c9a4215 net/9p: use a dedicated spinlock for trans_fd @@ Commit message Since the locks actually protect different things in client.c and in trans_fd.c, just replace trans_fd.c's lock by a new one specific to the - transport instead of using spin_lock_irq* variants everywhere - (client.c's protect the idr for tag allocations, while - trans_fd.c's protects its own req list and request status field + transport (client.c's protect the idr for fid/tag allocations, + while trans_fd.c's protects its own req list and request status field that acts as the transport's state machine) - Link: https://lkml.kernel.org/r/20220904112928.1308799-1-asmadeus@codewreck.org + Link: https://lore.kernel.org/r/20220904112928.1308799-1-asmadeus@codewreck.org Link: https://lkml.kernel.org/r/2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKURA.ne.jp [1] Link: https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2] Reported-by: syzbot <syzbot+2f20b523930c32c160cc@syzkaller.appspotmail.com> > > @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int rfd, > > int wfd) > > > > client->trans = ts; > > client->status = Connected; > > + spin_lock_init(&ts->conn.req_lock); > > Are you sure this is the right place for spin_lock_init()? Not rather in > p9_conn_create()? Good point, 'ts->conn' (named... m over there for some reason...) is mostly initialized in p9_conn_create; it makes much more sense to move it there despite being slightly further away from the allocation. It's a bit of a pain to check as the code is spread over many paths (fd, unix, tcp) but it looks equivalent to me: - p9_fd_open is only called from p9_fd_create which immediately calls p9_conn_create - below p9_socket_open itself immediately calls p9_conn_create I've moved the init and updated my next branch after very basic check https://github.com/martinetd/linux/commit/e5cfd99e9ea6c13b3f0134585f269c509247ac0e: ---- diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 5b4807411281..d407f31bb49d 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -591,6 +591,7 @@ static void p9_conn_create(struct p9_client *client) INIT_LIST_HEAD(&m->mux_list); m->client = client; + spin_lock_init(&m->req_lock); INIT_LIST_HEAD(&m->req_list); INIT_LIST_HEAD(&m->unsent_req_list); INIT_WORK(&m->rq, p9_read_work); @@ -840,7 +841,6 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) client->trans = ts; client->status = Connected; - spin_lock_init(&ts->conn.req_lock); return 0; @@ -875,7 +875,6 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) p->wr = p->rd = file; client->trans = p; client->status = Connected; - spin_lock_init(&p->conn.req_lock); p->rd->f_flags |= O_NONBLOCK; ---- > The rest LGTM. Thank you for the look :) -- Dominique ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net/9p: use a dedicated spinlock for trans_fd 2022-10-07 1:05 ` Dominique Martinet @ 2022-10-07 9:29 ` Christian Schoenebeck 0 siblings, 0 replies; 5+ messages in thread From: Christian Schoenebeck @ 2022-10-07 9:29 UTC (permalink / raw) To: Dominique Martinet Cc: v9fs-developer, Tetsuo Handa, syzbot, syzkaller-bugs, linux-kernel On Freitag, 7. Oktober 2022 03:05:39 CEST Dominique Martinet wrote: > Christian Schoenebeck wrote on Thu, Oct 06, 2022 at 03:16:40PM +0200: > > > net/9p/trans_fd.c | 42 ++++++++++++++++++++++++++---------------- > > > 1 file changed, 26 insertions(+), 16 deletions(-) > > > > Late on the party, sorry. Note that you already queued a slightly > > different > > version than this patch here, anyway, one thing ... > > Did I? Oh, I apparently reworded the commit message a bit, sorry: > > (where HEAD is this patch and 1622... the queued patch) > > $ git range-diff HEAD^- 16228c9a4215^- > 1: e35fb8546af2 ! 1: 16228c9a4215 net/9p: use a dedicated spinlock for > trans_fd @@ Commit message > > Since the locks actually protect different things in client.c and > in trans_fd.c, just replace trans_fd.c's lock by a new one specific to the > - transport instead of using spin_lock_irq* variants everywhere - > (client.c's protect the idr for tag allocations, while > - trans_fd.c's protects its own req list and request status field > + transport (client.c's protect the idr for fid/tag allocations, > + while trans_fd.c's protects its own req list and request status > field that acts as the transport's state machine) > > - Link: > https://lkml.kernel.org/r/20220904112928.1308799-1-asmadeus@codewreck.org + > Link: > https://lore.kernel.org/r/20220904112928.1308799-1-asmadeus@codewreck.org > Link: > https://lkml.kernel.org/r/2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKUR > A.ne.jp [1] Link: > https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [2] > Reported-by: syzbot <syzbot+2f20b523930c32c160cc@syzkaller.appspotmail.com> No, that's not what I meant, but my bad, it was the following chunk that didn't apply here: diff a/net/9p/trans_fd.c b/net/9p/trans_fd.c (rejected hunks) @@ -205,7 +207,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err) list_move(&req->req_list, &cancel_list); } - spin_unlock(&m->client->lock); + spin_unlock(&m->req_lock); list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) { p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req); And that was because this patch was based on: https://github.com/martinetd/linux/commit/52f1c45dde9136f964d I usually tag patches depending on another patch not being merged yet (and not being tied to the same series) like: Based-on: <message-id> > > > @@ -832,6 +840,7 @@ static int p9_fd_open(struct p9_client *client, int > > > rfd, int wfd) > > > > > > client->trans = ts; > > > client->status = Connected; > > > > > > + spin_lock_init(&ts->conn.req_lock); > > > > Are you sure this is the right place for spin_lock_init()? Not rather in > > p9_conn_create()? > > Good point, 'ts->conn' (named... m over there for some reason...) is > mostly initialized in p9_conn_create; it makes much more sense to move > it there despite being slightly further away from the allocation. > > It's a bit of a pain to check as the code is spread over many paths (fd, > unix, tcp) but it looks equivalent to me: > - p9_fd_open is only called from p9_fd_create which immediately calls > p9_conn_create > - below p9_socket_open itself immediately calls p9_conn_create Yeah, looks pretty much the same, but better to have init code at the same place. Either or. > I've moved the init and updated my next branch after very basic check > https://github.com/martinetd/linux/commit/e5cfd99e9ea6c13b3f0134585f269c5092 > 47ac0e: ---- > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index 5b4807411281..d407f31bb49d 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -591,6 +591,7 @@ static void p9_conn_create(struct p9_client *client) > INIT_LIST_HEAD(&m->mux_list); > m->client = client; > > + spin_lock_init(&m->req_lock); > INIT_LIST_HEAD(&m->req_list); > INIT_LIST_HEAD(&m->unsent_req_list); > INIT_WORK(&m->rq, p9_read_work); > @@ -840,7 +841,6 @@ static int p9_fd_open(struct p9_client *client, int rfd, > int wfd) > > client->trans = ts; > client->status = Connected; > - spin_lock_init(&ts->conn.req_lock); > > return 0; > > @@ -875,7 +875,6 @@ static int p9_socket_open(struct p9_client *client, > struct socket *csocket) p->wr = p->rd = file; > client->trans = p; > client->status = Connected; > - spin_lock_init(&p->conn.req_lock); > > p->rd->f_flags |= O_NONBLOCK; > > ---- > > > The rest LGTM. > > Thank you for the look :) With that changed, you can add my RB-tag. :) Thanks! Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-07 9:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKURA.ne.jp> 2022-09-04 11:29 ` [PATCH] net/9p: use a dedicated spinlock for trans_fd Dominique Martinet 2022-09-04 13:03 ` Tetsuo Handa 2022-10-06 13:16 ` Christian Schoenebeck 2022-10-07 1:05 ` Dominique Martinet 2022-10-07 9:29 ` Christian Schoenebeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).