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: <a48c16d6-07df-ff44-67e6-f0942672ec28@redhat.com>
Date:   Mon, 6 Dec 2021 20:01:34 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Yang Shi <shy828301@...il.com>, Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     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

On 06.12.21 19:42, Yang Shi wrote:
> On Mon, Dec 6, 2021 at 5:19 AM Kirill Tkhai <ktkhai@...tuozzo.com> wrote:
>>
>> On 06.12.2021 13:45, David Hildenbrand wrote:
>>>> This doesn't seen complete. Slab shrinkers are used in the reclaim
>>>> context. Previously offline nodes could be onlined later and this would
>>>> lead to NULL ptr because there is no hook to allocate new shrinker
>>>> infos. This would be also really impractical because this would have to
>>>> update all existing memcgs...
>>>
>>> Instead of going through the trouble of updating...
>>>
>>> ...  maybe just keep for_each_node() and check if the target node is
>>> offline. If it's offline, just allocate from the first online node.
>>> After all, we're not using __GFP_THISNODE, so there are no guarantees
>>> either way ...
>>
>> Hm, can't we add shrinker maps allocation to __try_online_node() in addition
>> to this patch?
> 
> I think the below fix (an example, doesn't cover all affected
> callsites) should be good enough for now? It doesn't touch the hot
> path of the page allocator.
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index fb9584641ac7..1252a33f7c28 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -222,13 +222,15 @@ static int expand_one_shrinker_info(struct
> mem_cgroup *memcg,
>         int size = map_size + defer_size;
> 
>         for_each_node(nid) {
> +               int tmp = nid;
>                 pn = memcg->nodeinfo[nid];
>                 old = shrinker_info_protected(memcg, nid);
>                 /* Not yet online memcg */
>                 if (!old)
>                         return 0;
> -
> -               new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, nid);
> +               if (!node_online(nid))
> +                       tmp = -1;
> +               new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, tmp);
>                 if (!new)
>                         return -ENOMEM;
> 
> It used to use kvmalloc instead of kvmalloc_node(). The commit
> 86daf94efb11d7319fbef5e480018c4807add6ef ("mm/memcontrol.c: allocate
> shrinker_map on appropriate NUMA node") changed to use *_node()
> version. The justification was that "kswapd is always bound to
> specific node. So allocate shrinker_map from the related NUMA node to
> respect its NUMA locality." There is no kswapd for offlined node, so
> just allocate shrinker info on node 0. This is also what
> alloc_mem_cgroup_per_node_info() does.

Yes, that's what I refer to as fixing it in the caller -- similar to
[1]. Michals point is to not require such node_online() checks at all,
neither in the caller nor in the buddy.

I see 2 options short-term

1) What we have in [1].
2) What I proposed in [2], fixing it for all such instances until we
have something better.

Long term I tend to agree that what Michal proposes is better.

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 ...



[1] https://lkml.kernel.org/r/20211108202325.20304-1-amakhalov@vmware.com
[2]
https://lkml.kernel.org/r/51c65635-1dae-6ba4-daf9-db9df0ec35d8@redhat.com

-- 
Thanks,

David / dhildenb