| Message ID | 2470e028-9b05-2013-7198-1fdad071d999@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/9p: use a dedicated spinlock for modifying IDR | expand |
Tetsuo Handa wrote on Sun, Sep 04, 2022 at 03:09:28PM +0900: > syzbot is reporting inconsistent lock state in p9_req_put(), for > p9_tag_remove() from p9_req_put() from IRQ context is using > spin_lock_irqsave() on "struct p9_client"->lock but other locations > not from IRQ context are using spin_lock(). Ah, I was wondering what this problem could have been, but it's yet another instance of trans_fd abusing the client's lock when it really should get its own... I didn't realize mixing spin_lock_irq*() and spin_lock() was the problem, thank you. > Since spin_lock() => spin_lock_irqsave() conversion on this lock will > needlessly disable IRQ for infrequent event, and p9_tag_remove() needs > to disable IRQ only for modifying IDR (RCU read lock can be used for > reading IDR), let's introduce a spinlock dedicated for serializing > idr_alloc()/idr_alloc_u32()/idr_remove() calls. Since this spinlock > will be held as innermost lock, circular locking dependency problem > won't happen by adding this spinlock. We have an idr per client though so this is needlessly adding contention between multiple 9p mounts. That probably doesn't matter too much in the general case, but adding a different lock per connection is cheap so let's do it the other way around: could you add a lock to the p9_conn struct in net/9p/trans_fd.c and replace c->lock by it there? That should have identical effect as other transports don't use client's .lock ; and the locking in trans_fd.c is to protect req's status and trans_fd's own lists which is orthogonal to client's lock that protects tag allocations. I agree it should work in theory. (If you don't have time to do this this patch has been helpful enough and I can do it eventually) Thanks, -- Dominique
On 2022/09/04 15:36, Dominique Martinet wrote: > We have an idr per client though so this is needlessly adding contention > between multiple 9p mounts. > > That probably doesn't matter too much in the general case, I thought this is not a hot operation where contention matters. > but adding a > different lock per connection is cheap so let's do it the other way > around: could you add a lock to the p9_conn struct in net/9p/trans_fd.c > and replace c->lock by it there? Not impossible, but may not be cheap for lockdep. > That should have identical effect as other transports don't use client's > .lock ; and the locking in trans_fd.c is to protect req's status and > trans_fd's own lists which is orthogonal to client's lock that protects > tag allocations. I agree it should work in theory. > > (If you don't have time to do this this patch has been helpful enough and > I can do it eventually) Sent as https://lkml.kernel.org/r/68540a56-6a5f-87e9-3c21-49c58758bfaf@I-love.SAKURA.ne.jp . By the way, I think credit for the patch on https://syzkaller.appspot.com/bug?extid=50f7e8d06c3768dd97f3 goes to Hillf Danton...
diff --git a/net/9p/client.c b/net/9p/client.c index 0a6110e15d0f..20f0a2d8dd38 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -28,6 +28,8 @@ #define CREATE_TRACE_POINTS #include <trace/events/9p.h> +static DEFINE_SPINLOCK(p9_idr_lock); + #define DEFAULT_MSIZE (128 * 1024) /* Client Option Parsing (code inspired by NFS code) @@ -283,14 +285,14 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) INIT_LIST_HEAD(&req->req_list); idr_preload(GFP_NOFS); - spin_lock_irq(&c->lock); + spin_lock_irq(&p9_idr_lock); if (type == P9_TVERSION) tag = idr_alloc(&c->reqs, req, P9_NOTAG, P9_NOTAG + 1, GFP_NOWAIT); else tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT); req->tc.tag = tag; - spin_unlock_irq(&c->lock); + spin_unlock_irq(&p9_idr_lock); idr_preload_end(); if (tag < 0) goto free; @@ -364,9 +366,9 @@ static void p9_tag_remove(struct p9_client *c, struct p9_req_t *r) u16 tag = r->tc.tag; p9_debug(P9_DEBUG_MUX, "freeing clnt %p req %p tag: %d\n", c, r, tag); - spin_lock_irqsave(&c->lock, flags); + spin_lock_irqsave(&p9_idr_lock, flags); idr_remove(&c->reqs, tag); - spin_unlock_irqrestore(&c->lock, flags); + spin_unlock_irqrestore(&p9_idr_lock, flags); } int p9_req_put(struct p9_client *c, struct p9_req_t *r) @@ -813,10 +815,10 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) refcount_set(&fid->count, 1); idr_preload(GFP_KERNEL); - spin_lock_irq(&clnt->lock); + spin_lock_irq(&p9_idr_lock); ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1, GFP_NOWAIT); - spin_unlock_irq(&clnt->lock); + spin_unlock_irq(&p9_idr_lock); idr_preload_end(); if (!ret) { trace_9p_fid_ref(fid, P9_FID_REF_CREATE); @@ -835,9 +837,9 @@ static void p9_fid_destroy(struct p9_fid *fid) p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid); trace_9p_fid_ref(fid, P9_FID_REF_DESTROY); clnt = fid->clnt; - spin_lock_irqsave(&clnt->lock, flags); + spin_lock_irqsave(&p9_idr_lock, flags); idr_remove(&clnt->fids, fid->fid); - spin_unlock_irqrestore(&clnt->lock, flags); + spin_unlock_irqrestore(&p9_idr_lock, flags); kfree(fid->rdir); kfree(fid); }
syzbot is reporting inconsistent lock state in p9_req_put(), for p9_tag_remove() from p9_req_put() from IRQ context is using spin_lock_irqsave() on "struct p9_client"->lock but other locations not from IRQ context are using spin_lock(). Since spin_lock() => spin_lock_irqsave() conversion on this lock will needlessly disable IRQ for infrequent event, and p9_tag_remove() needs to disable IRQ only for modifying IDR (RCU read lock can be used for reading IDR), let's introduce a spinlock dedicated for serializing idr_alloc()/idr_alloc_u32()/idr_remove() calls. Since this spinlock will be held as innermost lock, circular locking dependency problem won't happen by adding this spinlock. Link: https://syzkaller.appspot.com/bug?extid=2f20b523930c32c160cc [1] Reported-by: syzbot <syzbot+2f20b523930c32c160cc@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- This patch is not tested, for reproducer is not available. net/9p/client.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)