linux-kernel - Re: [PATCH] kernfs: convert kernfs_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]

Date: Tue, 9 Jan 2024 09:07:34 -1000
From: Tejun Heo <tj@...nel.org>
To: Andrea Righi <andrea.righi@...onical.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alexei Starovoitov <ast@...nel.org>, linux-kernel@...r.kernel.org,
	Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH] kernfs: convert kernfs_idr_lock to an irq safe raw
 spinlock

On Tue, Jan 09, 2024 at 06:05:09PM +0100, Andrea Righi wrote:
> On Tue, Jan 09, 2024 at 05:35:36PM +0100, Geert Uytterhoeven wrote:
> > Reverting commit c312828c37a72fe2 fixes that.
> > I have seen this issue on several Renesas arm32 and arm64 platforms.
> > 
> > Also, I am wondering if the issue fixed by commit c312828c37a72fe2
> > can still be reproduced on v6.7-rc5 or later?
> 
> Yep, I can still reproduce it (this is with v6.7):
..
> I'm wondering if using a regular spinlock instead of a raw spinlock
> could be a reasonable compromise.

I don't think that'd work on RT as we can end up nesting mutex inside a raw
spinlock.

> We have a GFP_ATOMIC allocation in __kernfs_new_node():
> 
> 	raw_spin_lock_irqsave(&kernfs_idr_lock, irqflags);
> 	ret = idr_alloc_cyclic(&root->ino_idr, kn, 1, 0, GFP_ATOMIC);
> 	...
>         raw_spin_unlock_irqrestore(&kernfs_idr_lock, irqflags);
> 
> That should become valid using a
> spin_lock_irqsave/spin_unlock_irqrestore(), right?

Yeah, this part should be fine. I think the right thing to do here is making
the idr RCU safe so that lookup path doesn't depend on the lock.

Greg, can you please revert c312828c37a72fe2 for now?

Thanks.

-- 
tejun