| Message ID | 20201027080256.76497-2-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some bugs in memcg/slab | expand |
On Tue, Oct 27, 2020 at 04:02:52PM +0800, Muchun Song wrote: > Consider the following memcg hierarchy. > > root > / \ > A B > > If we get the objcg of memcg A failed, the get_obj_cgroup_from_current > can return the wrong objcg for the root memcg. > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/memcontrol.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Hi Muchun, thank you for the patchset! A generic note: it's usually not a good idea to group fixes with non-fixes in one patchset if fixes are planned to be backported to stable. > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 1337775b04f3..fcbd79c5023e 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2945,7 +2945,7 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p) > > __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > { > - struct obj_cgroup *objcg = NULL; > + struct obj_cgroup *objcg; > struct mem_cgroup *memcg; > > if (memcg_kmem_bypass()) > @@ -2964,6 +2964,9 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > } > rcu_read_unlock(); > > + if (memcg == root_mem_cgroup) > + return NULL; > + > return objcg; > } I agree, it's a bug. Good catch, thanks! I would prefer a slightly different fix though. Because we're going to change how the root memory cgroup is handled (an rfc version posted here: https://lkml.org/lkml/2020/10/21/612), it's better to not use a comparison with the root_mem_cgroup and handle the obj_cgroup_tryget() return code instead. Something like this: @@ -2973,6 +2973,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) objcg = rcu_dereference(memcg->objcg); if (objcg && obj_cgroup_tryget(objcg)) break; + objcg = NULL; } rcu_read_unlock(); Or a more explicit: if (obj_cgroup_tryget(objcg)) { ... } else { ... } Also, please, add: Cc: stable@vger.kernel.org Thanks!
On Tue, Oct 27, 2020 at 10:42:36AM -0700, Roman Gushchin wrote: > On Tue, Oct 27, 2020 at 04:02:52PM +0800, Muchun Song wrote: > > Consider the following memcg hierarchy. > > > > root > > / \ > > A B > > > > If we get the objcg of memcg A failed, the get_obj_cgroup_from_current > > can return the wrong objcg for the root memcg. > > > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > mm/memcontrol.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > Hi Muchun, > > thank you for the patchset! > > A generic note: it's usually not a good idea to group fixes with non-fixes > in one patchset if fixes are planned to be backported to stable. > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 1337775b04f3..fcbd79c5023e 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2945,7 +2945,7 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p) > > > > __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > { > > - struct obj_cgroup *objcg = NULL; > > + struct obj_cgroup *objcg; > > struct mem_cgroup *memcg; > > > > if (memcg_kmem_bypass()) > > @@ -2964,6 +2964,9 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > } > > rcu_read_unlock(); > > > > + if (memcg == root_mem_cgroup) > > + return NULL; > > + > > return objcg; > > } > > I agree, it's a bug. Good catch, thanks! > > I would prefer a slightly different fix though. Because we're going to change how > the root memory cgroup is handled (an rfc version posted here: > https://lkml.org/lkml/2020/10/21/612), it's better to not use a comparison > with the root_mem_cgroup and handle the obj_cgroup_tryget() return code instead. > Something like this: > > @@ -2973,6 +2973,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > objcg = rcu_dereference(memcg->objcg); > if (objcg && obj_cgroup_tryget(objcg)) > break; > + objcg = NULL; > } > rcu_read_unlock(); > > Or a more explicit: > if (obj_cgroup_tryget(objcg)) { > ... > } else { > ... > } > > Also, please, add: > Cc: stable@vger.kernel.org After a second thought, it's also a highly theoretical race. In order to have obj_cgroup_tryget() failed we need to enter get_obj_cgroup_from_current() racing with cgroup offlining (which is already tricky) and then have memcg_reparent_objcgs() finish in between reading memcg->objcg and bumping the reference counter. So I'm not sure we need a stable@ backport, sorry for the confusion. Thanks!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1337775b04f3..fcbd79c5023e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2945,7 +2945,7 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p) __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) { - struct obj_cgroup *objcg = NULL; + struct obj_cgroup *objcg; struct mem_cgroup *memcg; if (memcg_kmem_bypass()) @@ -2964,6 +2964,9 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) } rcu_read_unlock(); + if (memcg == root_mem_cgroup) + return NULL; + return objcg; }
Consider the following memcg hierarchy. root / \ A B If we get the objcg of memcg A failed, the get_obj_cgroup_from_current can return the wrong objcg for the root memcg. Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/memcontrol.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)