netdev - Re: [PATCH] net/9p: use a dedicated spinlock for modifying IDR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxRHYaqqISAr5Rif@codewreck.org>
Date:   Sun, 4 Sep 2022 15:36:17 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Christian Schoenebeck <linux_oss@...debyte.com>,
        syzbot <syzbot+2f20b523930c32c160cc@...kaller.appspotmail.com>,
        v9fs-developer@...ts.sourceforge.net,
        syzkaller-bugs@...glegroups.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net/9p: use a dedicated spinlock for modifying IDR

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