linux-kernel - Re: [patch 1/6] md: remove dependency on __GFP

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: <alpine.DEB.2.00.1008231229410.14622@chino.kir.corp.google.com>
Date:	Mon, 23 Aug 2010 12:35:22 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	Neil Brown <neilb@...e.de>, Alasdair G Kergon <agk@...hat.com>,
	linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 1/6] md: remove dependency on __GFP_NOFAIL

On Mon, 23 Aug 2010, Andrew Morton wrote:

> > diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
> > --- a/drivers/md/dm-region-hash.c
> > +++ b/drivers/md/dm-region-hash.c
> > @@ -289,8 +289,12 @@ static struct dm_region *__rh_alloc(struct dm_region_hash *rh, region_t region)
> >  	struct dm_region *reg, *nreg;
> >  
> >  	nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC);
> > -	if (unlikely(!nreg))
> > -		nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL);
> > +	if (unlikely(!nreg)) {
> > +		/* FIXME: this may potentially loop forever */
> > +		do {
> > +			nreg = kmalloc(sizeof(*nreg), GFP_NOIO);
> > +		} while (!nreg);
> > +	}
> >  
> >  	nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
> >  		      DM_RH_CLEAN : DM_RH_NOSYNC;
> 
> erm.
> 
> The reason for adding GFP_NOFAIL in the first place was my observation
> that the kernel contained lots of open-coded retry-for-ever loops.
> 
> All of these are wrong, bad, buggy and mustfix.  So we consolidated the
> wrongbadbuggymustfix concept into the core MM so that miscreants could
> be easily identified and hopefully fixed.
> 

That consolidation would have been unnecessary, then, since all 
allocations with order < PAGE_ALLOC_COSTLY_ORDER automatically loop 
indefinitely in the page allocator.  struct dm_region allocations would 
already do that.

So this retry loop doesn't actually do anything that the page allocator 
already doesn't, with or without __GFP_NOFAIL.  The difference here is 
that

 - it doesn't depend on the page allocator's implementation, which may
   change over time, and

 - it adds documentation so that the subsystems doing these loops can
   (hopefully) fix these problems later, although their appear to be
   geniune cases where little other options are available.

> I think that simply undoing that change is a bad idea - it allows the
> wrongbadbuggymustfix code to hide from view.
> 

It removes several branches from the page allocator.

> The correct way to remove __GFP_NOFAIL is to fix the
> wrongbadbuggymustfix code properly.
> 

If the prerequisite for removing __GFP_NOFAIL is that nobody must ever 
loop indefinitely looking for memory or smaller order allocations don't 
implicitly retry, then there's little chance it'll ever get removed since 
they've existed for years without anybody cleaning them up.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/