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

From: Yang Shi
Date: Wed Mar 31 2021 - 14:07:36 EST


On Wed, Mar 31, 2021 at 6:54 AM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote:
>
> On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> >
> > 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 <oliver.sang@xxxxxxxxx>
> > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > Cc: Yang Shi <shy828301@xxxxxxxxx>
> > ---
> > 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);
}
>
> >
> > 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)