* [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()
@ 2021-03-30 23:44 Hugh Dickins
2021-03-31 13:54 ` Shakeel Butt
0 siblings, 1 reply; 5+ messages in thread
From: Hugh Dickins @ 2021-03-30 23:44 UTC (permalink / raw)
To: Andrew Morton
Cc: kernel test robot, Yang Shi, Stephen Rothwell, Roman Gushchin,
Kirill Tkhai, Vlastimil Babka, Shakeel Butt, Dave Chinner,
Johannes Weiner, Michal Hocko, LKML,
Linux Memory Management List, lkp, lkp
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@intel.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: Yang Shi <shy828301@gmail.com>
---
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
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()
2021-03-30 23:44 [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info() Hugh Dickins
@ 2021-03-31 13:54 ` Shakeel Butt
2021-03-31 18:06 ` Yang Shi
0 siblings, 1 reply; 5+ messages in thread
From: Shakeel Butt @ 2021-03-31 13:54 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, kernel test robot, Yang Shi, Stephen Rothwell,
Roman Gushchin, Kirill Tkhai, Vlastimil Babka, Dave Chinner,
Johannes Weiner, Michal Hocko, LKML,
Linux Memory Management List, lkp, kernel test robot
On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins <hughd@google.com> 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@intel.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: Yang Shi <shy828301@gmail.com>
> ---
> 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().
>
> 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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()
2021-03-31 13:54 ` Shakeel Butt
@ 2021-03-31 18:06 ` Yang Shi
2021-03-31 21:12 ` Hugh Dickins
0 siblings, 1 reply; 5+ messages in thread
From: Yang Shi @ 2021-03-31 18:06 UTC (permalink / raw)
To: Shakeel Butt
Cc: Hugh Dickins, Andrew Morton, kernel test robot, Stephen Rothwell,
Roman Gushchin, Kirill Tkhai, Vlastimil Babka, Dave Chinner,
Johannes Weiner, Michal Hocko, LKML,
Linux Memory Management List, lkp, kernel test robot
On Wed, Mar 31, 2021 at 6:54 AM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins <hughd@google.com> 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@intel.com>
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Cc: Yang Shi <shy828301@gmail.com>
> > ---
> > 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)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()
2021-03-31 18:06 ` Yang Shi
@ 2021-03-31 21:12 ` Hugh Dickins
2021-03-31 23:14 ` Yang Shi
0 siblings, 1 reply; 5+ messages in thread
From: Hugh Dickins @ 2021-03-31 21:12 UTC (permalink / raw)
To: Yang Shi
Cc: Shakeel Butt, Hugh Dickins, Andrew Morton, kernel test robot,
Stephen Rothwell, Roman Gushchin, Kirill Tkhai, Vlastimil Babka,
Dave Chinner, Johannes Weiner, Michal Hocko, LKML,
Linux Memory Management List, lkp, kernel test robot
On Wed, 31 Mar 2021, Yang Shi wrote:
> On Wed, Mar 31, 2021 at 6:54 AM Shakeel Butt <shakeelb@google.com> wrote:
> > On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins <hughd@google.com> 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@intel.com>
> > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > Cc: Yang Shi <shy828301@gmail.com>
> > > ---
> > > 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.
Yes, looking at it again, I am sure that Shakeel is right, and
that my patch was overkill - no need for shrinker_rwsem there.
Whether it's RCU-safe to free the info there, I have not reviewed at
all: but shrinker_rwsem would not help even if there were an issue.
> I prepared a patch:
Unsigned, white-space damaged, so does not apply.
>
> 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.
> + */
Just because I mis-inferred from the use of shrinker_info_protected()
that shrinker_rwsem was needed here, is no reason to add that comment:
imagine how unhelpfully bigger the kernel source would be if we added
a comment everywhere I had misunderstood something!
> + info = rcu_dereference_protected(pn->shrinker_info, true);
> kvfree(info);
> rcu_assign_pointer(pn->shrinker_info, NULL);
> }
That does it, but I bikeshedded with myself in the encyclopaedic
rcupdate.h, and decided rcu_replace_pointer(pn->shrinker_info, NULL, true)
would be best. But now see that patch won't fit so well into your series,
and I can't spend more time writing up a justification for it.
I think Andrew should simply delete my fix patch from his queue,
and edit out the
@@ -232,7 +239,7 @@ void free_shrinker_info(struct mem_cgrou
for_each_node(nid) {
pn = memcg->nodeinfo[nid];
- info = rcu_dereference_protected(pn->shrinker_info, true);
+ info = shrinker_info_protected(memcg, nid);
kvfree(info);
rcu_assign_pointer(pn->shrinker_info, NULL);
}
hunk from your mm-vmscan-add-shrinker_info_protected-helper.patch
which will then restore free_shrinker_info() to what you propose above.
Thanks,
Hugh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info()
2021-03-31 21:12 ` Hugh Dickins
@ 2021-03-31 23:14 ` Yang Shi
0 siblings, 0 replies; 5+ messages in thread
From: Yang Shi @ 2021-03-31 23:14 UTC (permalink / raw)
To: Hugh Dickins
Cc: Shakeel Butt, Andrew Morton, kernel test robot, Stephen Rothwell,
Roman Gushchin, Kirill Tkhai, Vlastimil Babka, Dave Chinner,
Johannes Weiner, Michal Hocko, LKML,
Linux Memory Management List, lkp, kernel test robot
On Wed, Mar 31, 2021 at 2:13 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Wed, 31 Mar 2021, Yang Shi wrote:
> > On Wed, Mar 31, 2021 at 6:54 AM Shakeel Butt <shakeelb@google.com> wrote:
> > > On Tue, Mar 30, 2021 at 4:44 PM Hugh Dickins <hughd@google.com> 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@intel.com>
> > > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > > Cc: Yang Shi <shy828301@gmail.com>
> > > > ---
> > > > 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.
>
> Yes, looking at it again, I am sure that Shakeel is right, and
> that my patch was overkill - no need for shrinker_rwsem there.
>
> Whether it's RCU-safe to free the info there, I have not reviewed at
> all: but shrinker_rwsem would not help even if there were an issue.
>
> > I prepared a patch:
>
> Unsigned, white-space damaged, so does not apply.
>
> >
> > 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.
> > + */
>
> Just because I mis-inferred from the use of shrinker_info_protected()
> that shrinker_rwsem was needed here, is no reason to add that comment:
> imagine how unhelpfully bigger the kernel source would be if we added
> a comment everywhere I had misunderstood something!
Yes, I agree the comment may incur more confusion. Better remove it.
>
> > + info = rcu_dereference_protected(pn->shrinker_info, true);
> > kvfree(info);
> > rcu_assign_pointer(pn->shrinker_info, NULL);
> > }
>
> That does it, but I bikeshedded with myself in the encyclopaedic
> rcupdate.h, and decided rcu_replace_pointer(pn->shrinker_info, NULL, true)
> would be best. But now see that patch won't fit so well into your series,
> and I can't spend more time writing up a justification for it.
>
> I think Andrew should simply delete my fix patch from his queue,
> and edit out the
> @@ -232,7 +239,7 @@ void free_shrinker_info(struct mem_cgrou
>
> for_each_node(nid) {
> pn = memcg->nodeinfo[nid];
> - info = rcu_dereference_protected(pn->shrinker_info, true);
> + info = shrinker_info_protected(memcg, nid);
> kvfree(info);
> rcu_assign_pointer(pn->shrinker_info, NULL);
> }
> hunk from your mm-vmscan-add-shrinker_info_protected-helper.patch
> which will then restore free_shrinker_info() to what you propose above.
Yes. I saw Andrew already had this fix in -mm tree.
>
> Thanks,
> Hugh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-31 23:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 23:44 [PATCH mmotm] mm: vmscan: fix shrinker_rwsem in free_shrinker_info() Hugh Dickins
2021-03-31 13:54 ` Shakeel Butt
2021-03-31 18:06 ` Yang Shi
2021-03-31 21:12 ` Hugh Dickins
2021-03-31 23:14 ` Yang Shi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).