linux-kernel - Re: [PATCH 1/1] mm: vmalloc: Fix lockdep warning

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] [day] [month] [year] [list]

Message-ID: <ZghqY6tpKsFvhETX@pc638.lan>
Date: Sat, 30 Mar 2024 20:39:15 +0100
From: Uladzislau Rezki <urezki@...il.com>
To: Baoquan He <bhe@...hat.com>
Cc: Uladzislau Rezki <urezki@...il.com>, linux-mm@...ck.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Lorenzo Stoakes <lstoakes@...il.com>,
	Christoph Hellwig <hch@...radead.org>,
	Matthew Wilcox <willy@...radead.org>,
	Dave Chinner <david@...morbit.com>,
	Oleksiy Avramchenko <oleksiy.avramchenko@...y.com>,
	Jens Axboe <axboe@...nel.dk>, Omar Sandoval <osandov@...com>
Subject: Re: [PATCH 1/1] mm: vmalloc: Fix lockdep warning

On Sat, Mar 30, 2024 at 09:21:25PM +0800, Baoquan He wrote:
> On 03/30/24 at 01:55pm, Uladzislau Rezki wrote:
> > On Fri, Mar 29, 2024 at 03:44:40PM +0800, Baoquan He wrote:
> > > On 03/28/24 at 03:03pm, Uladzislau Rezki (Sony) wrote:
> ......snip 
> > > How about below change about va_start_lowest? Personal preference, not
> > > strong opinion.
> > > 
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 9b1a41e12d70..bd6a66c54ad2 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -1046,19 +1046,19 @@ __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
> > >  static struct vmap_node *
> > >  find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > >  {
> > > -	unsigned long va_start_lowest;
> > > +	unsigned long va_start_lowest = ULONG_MAX;
> > >  	struct vmap_node *vn;
> > >  	int i;
> > >  
> > >  repeat:
> > > -	for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
> > > +	for (i = 0; i < nr_vmap_nodes; i++) {
> > >  		vn = &vmap_nodes[i];
> > >  
> > >  		spin_lock(&vn->busy.lock);
> > >  		*va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> > >  
> > >  		if (*va)
> > > -			if (!va_start_lowest || (*va)->va_start < va_start_lowest)
> > > +			if ((*va)->va_start < va_start_lowest)
> > >  				va_start_lowest = (*va)->va_start;
> > >  		spin_unlock(&vn->busy.lock);
> > >  	}
> > > @@ -1069,7 +1069,7 @@ find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > >  	 * been removed concurrently thus we need to proceed
> > >  	 * with next one what is a rare case.
> > >  	 */
> > > -	if (va_start_lowest) {
> > > +	if (va_start_lowest != ULONG_MAX) {
> > >  		vn = addr_to_node(va_start_lowest);
> > >  
> > >  		spin_lock(&vn->busy.lock);
> > > 
> > > 
> > To me it looks as incomplete. The "va_start_lowest" should be initialized
> > when repeat. Otherwise we can end up with an infinite repeating because
> > va_start_lowest != ULONG_MAX.
> 
> You are right. Anyway, it's just a suggestion from a different code
> style, please feel free to adjust it in or leave the patch as is.
> > 
>
OK!

Thank you.

--
Uladzislau Rezki