linux-kernel - Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

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: <20160422124730.GA11733@dhcp22.suse.cz>
Date:	Fri, 22 Apr 2016 14:47:30 +0200
From:	Michal Hocko <mhocko@...nel.org>
To:	Mikulas Patocka <mpatocka@...hat.com>
Cc:	linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>, Shaohua Li <shli@...nel.org>
Subject: Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Sat 16-04-16 16:31:35, Michal Hocko wrote:
> On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> > 
> > 
> > On Fri, 15 Apr 2016, Michal Hocko wrote:
> > 
> > > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > > 
> > > > 
> > > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > > 
> > > > > From: Michal Hocko <mhocko@...e.com>
> > > > > 
> > > > > copy_params seems to be little bit confused about which allocation flags
> > > > > to use. It enforces GFP_NOIO even though it uses
> > > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > > 
> > > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > > allocating pagetables).
> > > 
> > > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > > flags through the whole call chain and that is why we have
> > > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > > added in favor of memalloc_noio API. Both are clearly redundant.
> > > -- 
> > > Michal Hocko
> > > SUSE Labs
> > 
> > You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> > 
> > if (!(gfp_mask & __GFP_IO))
> > 	noio_flag = memalloc_noio_save();
> > ...
> > if (!(gfp_mask & __GFP_IO))
> > 	memalloc_noio_restore(noio_flag);
> > 
> > That would be better than repeating this hack in every __vmalloc caller 
> > that need GFP_NOIO.
> 
> It is not my intention to change __vmalloc behavior. If you strongly
> oppose the GFP_NOIO change I can drop it from the patch. It is
> __GFP_REPEAT which I am after.

I am dropping the GFP_NOIO part for this patch but now that I am looking
into the code more closely I completely fail why it is needed in the
first place.

copy_params seems to be called only from the ioctl context which doesn't
hold any locks which would lockup during the direct reclaim AFAICS. The
git log shows that the code has used PF_MEMALLOC before which is even
bigger mystery to me. Could you please clarify why this is GFP_NOIO
restricted context? Maybe it needed to be in the past but I do not see
any reason for it to be now so unless I am missing something the
GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
properly because there are copy_from_user calls in the same path which
could page fault and do GFP_KERNEL allocations anyway. I can send follow
up cleanups unless I am missing something subtle here.
-- 
Michal Hocko
SUSE Labs