Re: [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()

On Wed, Mar 31, 2021 at 6:54 AM Shakeel Butt [off-list ref] wrote:

On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins [off-list ref] wrote:

quoted

Lockdep warns mm/vmscan.c: suspicious rcu_dereference_protected() usage!
when free_shrinker_info() is called from mem_cgroup_css_free(): there it
is called with no locking, whereas alloc_shrinker_info() calls it with
down_write of shrinker_rwsem - which seems appropriate.  Rearrange that
so free_shrinker_info() can manage the shrinker_rwsem for itself.

Link: https://lkml.kernel.org/r/20210317140615.GB28839@xsang-OptiPlex-9020
Reported-by: kernel test robot <redacted>
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: Yang Shi <redacted>
---
Sorry, I've made no attempt to work out precisely where in the series
the locking went missing, nor tried to fit this in as a fix on top of
mm-vmscan-add-shrinker_info_protected-helper.patch
which Oliver reported (and which you notated in mmotm's "series" file).
This patch just adds the fix to the end of the series, after
mm-vmscan-shrink-deferred-objects-proportional-to-priority.patch

The patch "mm: vmscan: add shrinker_info_protected() helper" replaces
rcu_dereference_protected(shrinker_info, true) with
rcu_dereference_protected(shrinker_info,
lockdep_is_held(&shrinker_rwsem)).

I think we don't really need shrinker_rwsem in free_shrinker_info()
which is called from css_free(). The bits of the map have already been
'reparented' in css_offline. I think we can remove
lockdep_is_held(&shrinker_rwsem) for free_shrinker_info().

Thanks, Hugh and Shakeel. I missed the report.

I think Shakeel is correct, shrinker_rwsem is not required in css_free
path so Shakeel's proposal should be able to fix it. I prepared a
patch:

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 64bf07cc20f2..7348c26d4cac 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c

@@ -251,7 +251,12 @@ void free_shrinker_info(struct mem_cgroup *memcg)
        for_each_node(nid) {
                pn = memcg->nodeinfo[nid];
-               info = shrinker_info_protected(memcg, nid);
+               /*
+                * Don't use shrinker_info_protected() helper since
+                * free_shrinker_info() could be called by css_free()
+                * without holding shrinker_rwsem.
+                */
+               info = rcu_dereference_protected(pn->shrinker_info, true);
                kvfree(info);
                rcu_assign_pointer(pn->shrinker_info, NULL);
        }

quoted

 mm/vmscan.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

--- mmotm/mm/vmscan.c   2021-03-28 17:26:54.935553064 -0700
+++ linux/mm/vmscan.c   2021-03-30 15:55:13.374459559 -0700

@@ -249,18 +249,20 @@ void free_shrinker_info(struct mem_cgrou
        struct shrinker_info *info;
        int nid;

+       down_write(&shrinker_rwsem);
        for_each_node(nid) {
                pn = memcg->nodeinfo[nid];
                info = shrinker_info_protected(memcg, nid);
                kvfree(info);
                rcu_assign_pointer(pn->shrinker_info, NULL);
        }
+       up_write(&shrinker_rwsem);
 }

 int alloc_shrinker_info(struct mem_cgroup *memcg)
 {
        struct shrinker_info *info;
-       int nid, size, ret = 0;
+       int nid, size;
        int map_size, defer_size = 0;

        down_write(&shrinker_rwsem);

@@ -270,9 +272,9 @@ int alloc_shrinker_info(struct mem_cgrou
        for_each_node(nid) {
                info = kvzalloc_node(sizeof(*info) + size, GFP_KERNEL, nid);
                if (!info) {
+                       up_write(&shrinker_rwsem);
                        free_shrinker_info(memcg);
-                       ret = -ENOMEM;
-                       break;
+                       return -ENOMEM;
                }
                info->nr_deferred = (atomic_long_t *)(info + 1);
                info->map = (void *)info->nr_deferred + defer_size;

@@ -280,7 +282,7 @@ int alloc_shrinker_info(struct mem_cgrou
        }
        up_write(&shrinker_rwsem);

-       return ret;
+       return 0;
 }

 static inline bool need_expand(int nr_max)