linux-kernel - Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap

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: <20170403122951.GL24661@dhcp22.suse.cz>
Date:   Mon, 3 Apr 2017 14:29:51 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc:     Shakeel Butt <shakeelb@...gle.com>,
        Seth Jennings <sjenning@...hat.com>,
        Dan Streetman <ddstreet@...e.org>,
        Linux MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] mm/zswap: fix potential deadlock in
 zswap_frontswap_store()

On Mon 03-04-17 14:57:11, Andrey Ryabinin wrote:
> On 04/03/2017 11:47 AM, Michal Hocko wrote:
> > On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
[...]
> >>> @@ -1017,9 +1018,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> >>>
> >>>         /* store */
> >>>         len = dlen + sizeof(struct zswap_header);
> >>> -       ret = zpool_malloc(entry->pool->zpool, len,
> >>> -                          __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> >>> -                          &handle);
> >>> +       ret = zpool_malloc(entry->pool->zpool, len, gfp, &handle);
> > 
> > and here we used to do GFP_NOWAIT alternative already. What is going on
> > here?
> 
> 
> I suspect that there was no particular reason to assemble this
> custom set of gfp flags.  This code probably should have been using
> GFP_NOWAIT|__GFP_NOWARN from the very beginning.

Or just use GFP_KERNEL with a comment that this is called from the
reclaim context and as such is properly addressed at the page allocator
layer. One reason why this makes more sense than GFP_NOWAIT is that
this is easier to follow. When you see GFP_NOWAIT then you usually
expect a best efford opportunistic allocation attempt (especially with
__GFP_NOWARN) which is not the case here because this paths gets a full
memory reserves access. If this is not intentional then use GFP_NOWAIT |
__GFP_NOMEMALLOC | __GFP_NOWARN.

-- 
Michal Hocko
SUSE Labs