linux-kernel - Re: [PATCH] vmalloc: respect the GFP_NOIO and 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: <20170703062905.GB3217@dhcp22.suse.cz>
Date:   Mon, 3 Jul 2017 08:31:18 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Mikulas Patocka <mpatocka@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Vlastimil Babka <vbabka@...e.cz>,
        Andreas Dilger <adilger@...ger.ca>,
        John Hubbard <jhubbard@...dia.com>,
        David Miller <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] vmalloc: respect the GFP_NOIO and GFP_NOFS flags

On Fri 30-06-17 20:36:12, Mikulas Patocka wrote:
> 
> 
> On Fri, 30 Jun 2017, Michal Hocko wrote:
> 
> > On Fri 30-06-17 14:11:57, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Fri, 30 Jun 2017, Michal Hocko wrote:
> > > 
> > > > On Thu 29-06-17 22:25:09, Mikulas Patocka wrote:
> > > > > The __vmalloc function has a parameter gfp_mask with the allocation flags,
> > > > > however it doesn't fully respect the GFP_NOIO and GFP_NOFS flags. The
> > > > > pages are allocated with the specified gfp flags, but the pagetables are
> > > > > always allocated with GFP_KERNEL. This allocation can cause unexpected
> > > > > recursion into the filesystem or I/O subsystem.
> > > > > 
> > > > > It is not practical to extend page table allocation routines with gfp
> > > > > flags because it would require modification of architecture-specific code
> > > > > in all architecturs. However, the process can temporarily request that all
> > > > > allocations are done with GFP_NOFS or GFP_NOIO with with the functions
> > > > > memalloc_nofs_save and memalloc_noio_save.
> > > > > 
> > > > > This patch makes the vmalloc code use memalloc_nofs_save or
> > > > > memalloc_noio_save if the supplied gfp flags do not contain __GFP_FS or
> > > > > __GFP_IO. It fixes some possible deadlocks in drivers/mtd/ubi/io.c,
> > > > > fs/gfs2/, fs/btrfs/free-space-tree.c, fs/ubifs/,
> > > > > fs/nfs/blocklayout/extent_tree.c where __vmalloc is used with the GFP_NOFS
> > > > > flag.
> > > > 
> > > > I strongly believe this is a step in the _wrong_ direction. Why? Because
> > > 
> > > What do you think __vmalloc with GFP_NOIO should do? Print a warning? 
> > > Silently ignore the GFP_NOIO flag?
> > 
> > I think noio users are not that much different from nofs users. Simply
> > use the scope API at the place where the scope starts and document why
> > it is needed. vmalloc calls do not have to be any special then and they
> > do not even have to think about proper gfp flags and they can use
> > whatever is the default.
> > -- 
> > Michal Hocko
> > SUSE Labs
> 
> But you didn't answer the question - what should __vmalloc with GFP_NOIO 
> (or GFP_NOFS) do? Silently drop the flag? Print a warning? Or respect the 
> flag?

We can add a warning (or move it from kvmalloc) and hope that the
respective maintainers will fix those places properly. The reason I
didn't add the warning to vmalloc and kept it in kvmalloc was to catch
only new users rather than suddenly splat on existing ones. Note that
there are users with panic_on_warn enabled.

Considering how many NOFS users we have in tree I would rather work with
maintainers to fix them.
 
> Currently, it silently drops the GFP_NOIO or GFP_NOFS flag, but some 
> programmers don't know it and use these flags. You can't blame those 
> programmers for not knowing it.

At least __vmalloc_node is documented to not support all gfp flags.

-- 
Michal Hocko
SUSE Labs