linux-kernel - Re: [v2 PATCH 3/9] mm: vmscan: guarantee shrinker_slab_memcg() sees valid 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]

Date:   Tue, 15 Dec 2020 18:14:39 +0100
From:   Johannes Weiner <hannes@...xchg.org>
To:     Yang Shi <shy828301@...il.com>
Cc:     guro@...com, ktkhai@...tuozzo.com, shakeelb@...gle.com,
        david@...morbit.com, mhocko@...e.com, akpm@...ux-foundation.org,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH 3/9] mm: vmscan: guarantee shrinker_slab_memcg() sees
 valid shrinker_maps for online memcg

On Mon, Dec 14, 2020 at 02:37:16PM -0800, Yang Shi wrote:
> The shrink_slab_memcg() races with mem_cgroup_css_online(). A visibility of CSS_ONLINE flag
> in shrink_slab_memcg()->mem_cgroup_online() does not guarantee that we will see
> memcg->nodeinfo[nid]->shrinker_maps != NULL.  This may occur because of processor reordering
> on !x86.
> 
> This seems like the below case:
> 
>            CPU A          CPU B
> store shrinker_map      load CSS_ONLINE
> store CSS_ONLINE        load shrinker_map

But we have a separate check on shrinker_maps, so it doesn't matter
that it isn't guaranteed, no?

The only downside I can see is when CSS_ONLINE isn't visible yet and
we bail even though we'd be ready to shrink. Although it's probably
unlikely that there would be any objects allocated already...

Can somebody remind me why we check mem_cgroup_online() at all?

If shrinker_map is set, we can shrink: .css_alloc is guaranteed to be
complete, and by using RCU for the shrinker_map pointer, the map is
also guaranteed to be initialized. There is nothing else happening
during onlining that you may depend on.

If shrinker_map isn't set, we cannot iterate the bitmap. It does not
really matter whether CSS_ONLINE is reordered and visible already.

Agreed with Dave: if we need that synchronization around onlining, it
needs to happen inside the cgroup core. But I wouldn't add that until
somebody actually required it.