linux-kernel - Re: [Outreachy kernel] [RFC PATCH] staging: unisys: visorhba: Convert module from IDR to XArray

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: <20210426132928.GL235567@casper.infradead.org>
Date:   Mon, 26 Apr 2021 14:29:28 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     outreachy-kernel@...glegroups.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        David Kershner <david.kershner@...sys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [Outreachy kernel] [RFC PATCH] staging: unisys: visorhba:
 Convert module from IDR to XArray

On Mon, Apr 26, 2021 at 03:14:42PM +0200, Fabio M. De Francesco wrote:
> > > -	int id;
> > > -	unsigned long flags;
> > > 
> > > -	idr_preload(GFP_KERNEL);
> > > -	spin_lock_irqsave(lock, flags);
> > > -	id = idr_alloc(idrtable, p, 1, INT_MAX, GFP_NOWAIT);
> > > -	spin_unlock_irqrestore(lock, flags);
> > > -	idr_preload_end();
> > > -	/* failure */
> > > -	if (id < 0)
> > > -		return 0;
> > > -	/* idr_alloc() guarantees > 0 */
> > > -	return (unsigned int)(id);
> >
> > And it shouldn't be using GFP_NOWAIT, but GFP_KERNEL, like the IDR code
> > used to do.
> I'm not sure to understand why idr_preload() uses GFP_KERNEL and instead  
> idr_alloc() uses GFP_NOWAIT. I'd better read anew the documentation of the 
> above-mentioned functions  

If you're holding a spinlock, you can't do a GFP_KERNEL allocation,
because it can sleep, and sleeping while holding a spinlock isn't allowed.

The IDR and radix tree have an approach where you first preallocate
memory using GFP_KERNEL and then use GFP_NOWAIT or GFP_ATOMIC after
you've taken the spinlock.  XArray doesn't do that; it takes the spinlock
and does a GFP_NOWAIT allocation.  If it fails, it drops the spinlock,
allocates the memory using GFP_KERNEL, and retries.

> This will not be anymore a problem when I'll restore the use of one namespace 
> per HBA. It's correct?

true ...

> > More generally, the IDR required you call idr_destroy() to avoid leaking
> > preallocated memory.  I changed that, but there are still many drivers
> > that have unnecessary calls to idr_destroy().  It's good form to just
> > delete them and not turn them into calls to xa_destroy().
> >
> This one is a bit obscure to me. I have to look into it more carefully. Maybe 
> I'll ask for some further help.

The IDR used to have a per-idr preallocation, so you had to destroy it
in order to make sure they were freed.  I got rid of that about five
years ago because most IDR users weren't calling idr_destroy().