linux-kernel - Re: [PATCH 1/2 v2] bcachefs: do not use PF_MEMALLOC

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALOAHbA+s2akXcb2kBAOqiUSn7RQaBtvUEGMFwU=dBroBh_+qQ@mail.gmail.com>
Date: Thu, 29 Aug 2024 22:03:13 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Dave Chinner <david@...morbit.com>, Michal Hocko <mhocko@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Christoph Hellwig <hch@....de>, jack@...e.cz, 
	Christian Brauner <brauner@...nel.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, 
	linux-bcachefs@...r.kernel.org, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 1/2 v2] bcachefs: do not use PF_MEMALLOC_NORECLAIM

On Thu, Aug 29, 2024 at 9:32 PM Kent Overstreet
<kent.overstreet@...ux.dev> wrote:
>
> On Thu, Aug 29, 2024 at 11:12:18PM GMT, Dave Chinner wrote:
> > On Thu, Aug 29, 2024 at 06:02:32AM -0400, Kent Overstreet wrote:
> > > On Wed, Aug 28, 2024 at 02:09:57PM GMT, Dave Chinner wrote:
> > > > On Tue, Aug 27, 2024 at 08:15:43AM +0200, Michal Hocko wrote:
> > > > > From: Michal Hocko <mhocko@...e.com>
> > > > >
> > > > > bch2_new_inode relies on PF_MEMALLOC_NORECLAIM to try to allocate a new
> > > > > inode to achieve GFP_NOWAIT semantic while holding locks. If this
> > > > > allocation fails it will drop locks and use GFP_NOFS allocation context.
> > > > >
> > > > > We would like to drop PF_MEMALLOC_NORECLAIM because it is really
> > > > > dangerous to use if the caller doesn't control the full call chain with
> > > > > this flag set. E.g. if any of the function down the chain needed
> > > > > GFP_NOFAIL request the PF_MEMALLOC_NORECLAIM would override this and
> > > > > cause unexpected failure.
> > > > >
> > > > > While this is not the case in this particular case using the scoped gfp
> > > > > semantic is not really needed bacause we can easily pus the allocation
> > > > > context down the chain without too much clutter.
> > > > >
> > > > > Acked-by: Christoph Hellwig <hch@....de>
> > > > > Signed-off-by: Michal Hocko <mhocko@...e.com>
> > > >
> > > > Looks good to me.
> > > >
> > > > Reviewed-by: Dave Chinner <dchinner@...hat.com>
> > >
> > > Reposting what I wrote in the other thread:
> >
> > I've read the thread. I've heard what you have had to say. Like
> > several other people, I think your position is just not practical or
> > reasonable.
> >
> > I don't care about the purity or the safety of the API - the
> > practical result of PF_MEMALLOC_NORECLAIM is that __GFP_NOFAIL
> > allocation can now fail and that will cause unexpected kernel
> > crashes.  Keeping existing code and API semantics working correctly
> > (i.e. regression free) takes precedence over new functionality or
> > API features that people want to introduce.
> >
> > That's all there is to it. This is not a hill you need to die on.
>
> And more than that, this is coming from you saying "We didn't have to
> handle memory allocation failures in IRIX, why can't we be like IRIX?
> All those error paths are a pain to test, why can't we get rid of them?"
>
> Except that's bullshit; at the very least any dynamically sized
> allocation _definitely_ has to have an error path that's tested, and if
> there's questions about the context a code path might run in, that
> that's another reason.
>
> GFP_NOFAIL is the problem here, and if it's encouraging this brain
> damaged "why can't we just get rid of error paths?" thinking, then it
> should be removed.
>
> Error paths have to exist, and they have to be tested.

I completely agree.
Adding a dead loop in the core of the page allocator just to bypass
error handling is a reckless idea.

-- 
Regards
Yafang