linux-kernel - Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete __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: <20140322002159.e1428d6a58899b38b379733c@skynet.be>
Date:	Sat, 22 Mar 2014 00:21:59 +0100
From:	Fabian Frederick <fabf@...net.be>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 1/1] fs/reiserfs/journal.c: Remove obsolete  __GFP_NOFAIL

On Fri, 21 Mar 2014 13:00:55 -0700
Andrew Morton <akpm@...ux-foundation.org> wrote:

> On Fri, 21 Mar 2014 17:18:30 +0100 Fabian Frederick <fabf@...net.be> wrote:
> 
> > Loop around congestion_wait on allocation failure/alloc_journal_list
> > like already fixed in other FS.
> > 
> > ...
> >
> > --- a/fs/reiserfs/journal.c
> > +++ b/fs/reiserfs/journal.c
> > @@ -2487,8 +2487,13 @@ static int journal_read(struct super_block *sb)
> >  static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
> >  {
> >  	struct reiserfs_journal_list *jl;
> > -	jl = kzalloc(sizeof(struct reiserfs_journal_list),
> > -		     GFP_NOFS | __GFP_NOFAIL);
> > +
> > +	do {
> > +		jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> > +		if (unlikely(!jl))
> > +			congestion_wait(BLK_RW_ASYNC, HZ/50);
> > +	} while (!jl)
> > +
> 
> Dammit, who has been running around converting __GFP_NOFAIL into
> open-coded congestion_wait() loops?
> 
> The whole point of __GFP_NOFAIL is to centralise this
> wait-for-memory-for-ever operation.  So it is implemented in a common
> (core) place and so that we can easily locate these problematic
> callers.
> 
> This comment in ext4:
> 
> 			/*
> 			 * If __GFP_FS is not present, then we may be
> 			 * being called from inside the fs writeback
> 			 * layer, so we MUST NOT fail.  Since
> 			 * __GFP_NOFAIL is going away, we will arrange
> 			 * to retry the allocation ourselves.
> 			 */
> 
> is exactly wrong.  Yes, we'd like __GFP_NOFAIL to go away, but it
> cannot go away until buggy callsites such as this one are *fixed*. 
> Removing the __GFP_NOFAIL usage simply hides the buggy code from casual
> searchers.
> 
> argh.
> 
> What we should do is to fix all these call sites so they can handle
> memory exhaustion.  That's hard so in the interim they should be using
> __GFP_NOFAIL.
> 

Ok, if even ext4 comments are wrong, things gonna be very difficult :)
Any sample of a callsite transition done well ?  (git id ?)
--
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/