linux-kernel - Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker

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: <0c5a157f-82cb-0ac8-9955-8f19469454bb@redhat.com>
Date:   Tue, 7 Dec 2021 11:15:33 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Yang Shi <shy828301@...il.com>
Cc:     Kirill Tkhai <ktkhai@...tuozzo.com>,
        Michal Hocko <mhocko@...e.com>, Nico Pache <npache@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shakeel Butt <shakeelb@...gle.com>,
        Roman Gushchin <guro@...com>, Vlastimil Babka <vbabka@...e.cz>,
        Vladimir Davydov <vdavydov.dev@...il.com>, raquini@...hat.com
Subject: Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on
 offlined nodes

>>
>> Short term I tend to like [2], because it avoids having to mess with all
>> such instances to eventually get it right and the temporary overhead
>> until we have the code reworked should be really negligible ...
> 
> Thanks, David. Basically either option looks fine to me. But I'm a
> little bit concerned about [2]. It silently changes the node requested
> by the callers. It actually papers over potential bugs? And what if

Hi,

It's just a preferred node, so we do have a node fallback already via
the zonelist to other nodes for proper online nodes -- and would have
the proper node fallback when preallcoating all pgdat.

*Not* doing the fallback with a preferred node that is not online could
be considered a BUG instead.

Note that [2] was just a quick draft. We might have to do some minor
adjustments to handle __GFP_THISNODE properly.

But after all, we have:

VM_WARN_ON((gfp_mask & __GFP_THISNODE) && !node_online(nid));

in __alloc_pages_node() and __folio_alloc_node(). So it might not be
worth adjusting at all.

> the callers specify __GFP_THISNODE (I didn't search if such callers
> really exist in the current code)?
> 
> How's about a helper function, for example, called
> kvmalloc_best_node()? It does:
> 
> void * kvmalloc_best_node(unsigned long size, int flag, int nid)
> {
>     bool onlined = node_online(nid);
> 
>     WARN_ON_ONCE((flag & __GFP_THISNODE) && !onlined);
> 
>     if (!onlined)
>         nid = -1;
> 
>     return kvmalloc_node(size, GFP_xxx, nid);
> }

We still have to "fix" each and every affected for_each_node() ... code
until we have preallcoation of pgdat for all possible nodes.

-- 
Thanks,

David / dhildenb