[PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Muchun Song]

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>
---
 changelog in v2:
 1. Do not use a comparison with the root_mem_cgroup

 mm/memcontrol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1337775b04f3..8c8b4c3ed5a0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2961,6 +2961,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();
 
-- 
2.20.1

[PATCH v2] mm: memcg/slab: Fix use after free in obj_cgroup_charge

[Muchun Song]

The rcu_read_lock/unlock only can guarantee that the memcg will
not be freed, but it cannot guarantee the success of css_get to
memcg.

If the whole process of a cgroup offlining is completed between
reading a objcg->memcg pointer and bumping the css reference on
another CPU, and there are exactly 0 external references to this
memory cgroup (how we get to the obj_cgroup_charge() then?),
css_get() can change the ref counter from 0 back to 1.

Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 changelog in v2:
 1. Add unlikely and update the commit log suggested by Roman.

 mm/memcontrol.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c8b4c3ed5a0..d9cdf899c6fc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3221,8 +3221,10 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
 	 * independently later.
 	 */
 	rcu_read_lock();
+retry:
 	memcg = obj_cgroup_memcg(objcg);
-	css_get(&memcg->css);
+	if (unlikely(!css_tryget(&memcg->css)))
+		goto retry;
 	rcu_read_unlock();
 
 	nr_pages = size >> PAGE_SHIFT;
-- 
2.20.1

[PATCH v2] mm: memcg/slab: Rename *_lruvec_slab_state to *_lruvec_kmem_state

[Muchun Song]

The *_lruvec_slab_state is also suitable for pages allocated from buddy,
not just for the slab objects. But the function name seems to tell us that
only slab object is applicable. So we can rename the keyword of slab to
kmem.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 changelog in v2:
 1. Remove VM_BUG_ON suggested by Roman.

 include/linux/memcontrol.h | 18 +++++++++---------
 kernel/fork.c              |  2 +-
 mm/memcontrol.c            |  2 +-
 mm/workingset.c            |  8 ++++----
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d7e339bf72dc..95807bf6be64 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -793,15 +793,15 @@ void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 			      int val);
 void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 			int val);
-void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val);
+void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val);
 
-static inline void mod_lruvec_slab_state(void *p, enum node_stat_item idx,
+static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 					 int val)
 {
 	unsigned long flags;
 
 	local_irq_save(flags);
-	__mod_lruvec_slab_state(p, idx, val);
+	__mod_lruvec_kmem_state(p, idx, val);
 	local_irq_restore(flags);
 }
 
@@ -1227,7 +1227,7 @@ static inline void mod_lruvec_page_state(struct page *page,
 	mod_node_page_state(page_pgdat(page), idx, val);
 }
 
-static inline void __mod_lruvec_slab_state(void *p, enum node_stat_item idx,
+static inline void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 					   int val)
 {
 	struct page *page = virt_to_head_page(p);
@@ -1235,7 +1235,7 @@ static inline void __mod_lruvec_slab_state(void *p, enum node_stat_item idx,
 	__mod_node_page_state(page_pgdat(page), idx, val);
 }
 
-static inline void mod_lruvec_slab_state(void *p, enum node_stat_item idx,
+static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 					 int val)
 {
 	struct page *page = virt_to_head_page(p);
@@ -1330,14 +1330,14 @@ static inline void __dec_lruvec_page_state(struct page *page,
 	__mod_lruvec_page_state(page, idx, -1);
 }
 
-static inline void __inc_lruvec_slab_state(void *p, enum node_stat_item idx)
+static inline void __inc_lruvec_kmem_state(void *p, enum node_stat_item idx)
 {
-	__mod_lruvec_slab_state(p, idx, 1);
+	__mod_lruvec_kmem_state(p, idx, 1);
 }
 
-static inline void __dec_lruvec_slab_state(void *p, enum node_stat_item idx)
+static inline void __dec_lruvec_kmem_state(void *p, enum node_stat_item idx)
 {
-	__mod_lruvec_slab_state(p, idx, -1);
+	__mod_lruvec_kmem_state(p, idx, -1);
 }
 
 /* idx can be of type enum memcg_stat_item or node_stat_item */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4b328aecabb2..4fb0bbc3b041 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -384,7 +384,7 @@ static void account_kernel_stack(struct task_struct *tsk, int account)
 		mod_lruvec_page_state(vm->pages[0], NR_KERNEL_STACK_KB,
 				      account * (THREAD_SIZE / 1024));
 	else
-		mod_lruvec_slab_state(stack, NR_KERNEL_STACK_KB,
+		mod_lruvec_kmem_state(stack, NR_KERNEL_STACK_KB,
 				      account * (THREAD_SIZE / 1024));
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d9cdf899c6fc..2dde734df7d1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -866,7 +866,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 		__mod_memcg_lruvec_state(lruvec, idx, val);
 }
 
-void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val)
+void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
 {
 	pg_data_t *pgdat = page_pgdat(virt_to_page(p));
 	struct mem_cgroup *memcg;
diff --git a/mm/workingset.c b/mm/workingset.c
index 50d53f3699e4..2c707c92dd89 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -445,12 +445,12 @@ void workingset_update_node(struct xa_node *node)
 	if (node->count && node->count == node->nr_values) {
 		if (list_empty(&node->private_list)) {
 			list_lru_add(&shadow_nodes, &node->private_list);
-			__inc_lruvec_slab_state(node, WORKINGSET_NODES);
+			__inc_lruvec_kmem_state(node, WORKINGSET_NODES);
 		}
 	} else {
 		if (!list_empty(&node->private_list)) {
 			list_lru_del(&shadow_nodes, &node->private_list);
-			__dec_lruvec_slab_state(node, WORKINGSET_NODES);
+			__dec_lruvec_kmem_state(node, WORKINGSET_NODES);
 		}
 	}
 }
@@ -541,7 +541,7 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 	}
 
 	list_lru_isolate(lru, item);
-	__dec_lruvec_slab_state(node, WORKINGSET_NODES);
+	__dec_lruvec_kmem_state(node, WORKINGSET_NODES);
 
 	spin_unlock(lru_lock);
 
@@ -564,7 +564,7 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 	 * shadow entries we were tracking ...
 	 */
 	xas_store(&xas, NULL);
-	__inc_lruvec_slab_state(node, WORKINGSET_NODERECLAIM);
+	__inc_lruvec_kmem_state(node, WORKINGSET_NODERECLAIM);
 
 out_invalid:
 	xa_unlock_irq(&mapping->i_pages);
-- 
2.20.1

[PATCH v2] mm: memcontrol: Simplify the mem_cgroup_page_lruvec

[Muchun Song]

We can reuse the code of mem_cgroup_lruvec() to simplify the code
of the mem_cgroup_page_lruvec().

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 changelog in v2:
 1. Move mem_cgroup_node_lruvec to memcontrol.c to avoid abuse.

 include/linux/memcontrol.h | 41 ++++-------------------------
 mm/memcontrol.c            | 53 ++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 52 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 95807bf6be64..bbdc694d26b1 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -445,48 +445,17 @@ void mem_cgroup_uncharge_list(struct list_head *page_list);
 
 void mem_cgroup_migrate(struct page *oldpage, struct page *newpage);
 
-static struct mem_cgroup_per_node *
+static inline struct mem_cgroup_per_node *
 mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
 {
 	return memcg->nodeinfo[nid];
 }
 
-/**
- * mem_cgroup_lruvec - get the lru list vector for a memcg & node
- * @memcg: memcg of the wanted lruvec
- *
- * Returns the lru list vector holding pages for a given @memcg &
- * @node combination. This can be the node lruvec, if the memory
- * controller is disabled.
- */
-static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
-					       struct pglist_data *pgdat)
-{
-	struct mem_cgroup_per_node *mz;
-	struct lruvec *lruvec;
-
-	if (mem_cgroup_disabled()) {
-		lruvec = &pgdat->__lruvec;
-		goto out;
-	}
-
-	if (!memcg)
-		memcg = root_mem_cgroup;
-
-	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
-	lruvec = &mz->lruvec;
-out:
-	/*
-	 * Since a node can be onlined after the mem_cgroup was created,
-	 * we have to be prepared to initialize lruvec->pgdat here;
-	 * and if offlined then reonlined, we need to reinitialize it.
-	 */
-	if (unlikely(lruvec->pgdat != pgdat))
-		lruvec->pgdat = pgdat;
-	return lruvec;
-}
+struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
+				 struct pglist_data *pgdat);
 
-struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
+struct lruvec *mem_cgroup_page_lruvec(struct page *page,
+				      struct pglist_data *pgdat);
 
 struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index bbd40c5af61e..28095a1711aa 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1332,18 +1332,15 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
 	return ret;
 }
 
-/**
- * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
- * @page: the page
- * @pgdat: pgdat of the page
- *
- * This function relies on page->mem_cgroup being stable - see the
- * access rules in commit_charge().
+/*
+ * Note: Do not use this function directly. Please use mem_cgroup_lruvec()
+ * or mem_cgroup_page_lruvec() instead.
  */
-struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgdat)
+static struct lruvec *
+__mem_cgroup_node_lruvec(struct mem_cgroup *memcg, struct pglist_data *pgdat,
+			 int nid)
 {
 	struct mem_cgroup_per_node *mz;
-	struct mem_cgroup *memcg;
 	struct lruvec *lruvec;
 
 	if (mem_cgroup_disabled()) {
@@ -1351,20 +1348,15 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 		goto out;
 	}
 
-	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
-	mz = mem_cgroup_page_nodeinfo(memcg, page);
+	mz = mem_cgroup_nodeinfo(memcg, nid);
 	lruvec = &mz->lruvec;
 out:
 	/*
 	 * Since a node can be onlined after the mem_cgroup was created,
-	 * we have to be prepared to initialize lruvec->zone here;
+	 * we have to be prepared to initialize lruvec->pgdat here;
 	 * and if offlined then reonlined, we need to reinitialize it.
 	 */
 	if (unlikely(lruvec->pgdat != pgdat))
@@ -1372,6 +1364,35 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	return lruvec;
 }
 
+/**
+ * mem_cgroup_lruvec - get the lru list vector for a memcg & node
+ * @memcg: memcg of the wanted lruvec
+ *
+ * Returns the lru list vector holding pages for a given @memcg &
+ * @node combination. This can be the node lruvec, if the memory
+ * controller is disabled.
+ */
+struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
+				 struct pglist_data *pgdat)
+{
+	return __mem_cgroup_node_lruvec(memcg, pgdat, pgdat->node_id);
+}
+
+/**
+ * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
+ * @page: the page
+ * @pgdat: pgdat of the page
+ *
+ * This function relies on page->mem_cgroup being stable - see the
+ * access rules in commit_charge().
+ */
+struct lruvec *mem_cgroup_page_lruvec(struct page *page,
+				      struct pglist_data *pgdat)
+{
+	return __mem_cgroup_node_lruvec(page->mem_cgroup, pgdat,
+					page_to_nid(page));
+}
+
 /**
  * mem_cgroup_update_lru_size - account for adding or removing an lru page
  * @lruvec: mem_cgroup per zone lru vector
-- 
2.20.1

Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Roman Gushchin]

On Wed, Oct 28, 2020 at 11:50:10AM +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>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

Re: [PATCH v2] mm: memcontrol: Simplify the mem_cgroup_page_lruvec

[Michal Hocko]

On Wed 28-10-20 11:50:13, Muchun Song wrote:
[...]
> -struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgdat)
> +static struct lruvec *
> +__mem_cgroup_node_lruvec(struct mem_cgroup *memcg, struct pglist_data *pgdat,
> +			 int nid)

I thought I have made it clear that this is not a good approach. Please
do not repost new version without that being addressed. If there are any
questions then feel free to ask for details.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Shakeel Butt]

On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> Consider the following memcg hierarchy.
>
>                     root
>                    /    \
>                   A      B
>
> If we get the objcg of memcg A failed,

Please fix the above statement.

> 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>
> ---
>  changelog in v2:
>  1. Do not use a comparison with the root_mem_cgroup
>
>  mm/memcontrol.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1337775b04f3..8c8b4c3ed5a0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2961,6 +2961,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;

Roman, in your cleanup, are you planning to have objcg for root memcg as well?

>         }
>         rcu_read_unlock();
>
> --
> 2.20.1
>

Re: [PATCH v2] mm: memcg/slab: Fix use after free in obj_cgroup_charge

[Shakeel Butt]

On Tue, Oct 27, 2020 at 8:51 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The rcu_read_lock/unlock only can guarantee that the memcg will
> not be freed, but it cannot guarantee the success of css_get to
> memcg.
>
> If the whole process of a cgroup offlining is completed between
> reading a objcg->memcg pointer and bumping the css reference on
> another CPU, and there are exactly 0 external references to this
> memory cgroup (how we get to the obj_cgroup_charge() then?),
> css_get() can change the ref counter from 0 back to 1.
>
> Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH v2] mm: memcg/slab: Rename *_lruvec_slab_state to *_lruvec_kmem_state

[Shakeel Butt]

On Tue, Oct 27, 2020 at 8:51 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The *_lruvec_slab_state is also suitable for pages allocated from buddy,
> not just for the slab objects. But the function name seems to tell us that
> only slab object is applicable. So we can rename the keyword of slab to
> kmem.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH v2] mm: memcontrol: Simplify the mem_cgroup_page_lruvec

[Shakeel Butt]

On Thu, Oct 29, 2020 at 2:08 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 28-10-20 11:50:13, Muchun Song wrote:
> [...]
> > -struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgdat)
> > +static struct lruvec *
> > +__mem_cgroup_node_lruvec(struct mem_cgroup *memcg, struct pglist_data *pgdat,
> > +                      int nid)
>
> I thought I have made it clear that this is not a good approach. Please
> do not repost new version without that being addressed. If there are any
> questions then feel free to ask for details.

You can get nid from pgdat (pgdat->node_id) and also pgdat from nid
(NODE_DATA(nid)), so, __mem_cgroup_node_lruvec() only need one of them
as parameter.

Re: [External] Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Muchun Song]

On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > Consider the following memcg hierarchy.
> >
> >                     root
> >                    /    \
> >                   A      B
> >
> > If we get the objcg of memcg A failed,
>
> Please fix the above statement.

Sorry, could you be more specific, I don't quite understand.
Thanks.


>
> > 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>
> > ---
> >  changelog in v2:
> >  1. Do not use a comparison with the root_mem_cgroup
> >
> >  mm/memcontrol.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 1337775b04f3..8c8b4c3ed5a0 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2961,6 +2961,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;
>
> Roman, in your cleanup, are you planning to have objcg for root memcg as well?
>
> >         }
> >         rcu_read_unlock();
> >
> > --
> > 2.20.1
> >



--
Yours,
Muchun

Re: [PATCH v2] mm: memcontrol: Simplify the mem_cgroup_page_lruvec

[Michal Hocko]

On Thu 29-10-20 09:01:37, Shakeel Butt wrote:
> On Thu, Oct 29, 2020 at 2:08 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 28-10-20 11:50:13, Muchun Song wrote:
> > [...]
> > > -struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgdat)
> > > +static struct lruvec *
> > > +__mem_cgroup_node_lruvec(struct mem_cgroup *memcg, struct pglist_data *pgdat,
> > > +                      int nid)
> >
> > I thought I have made it clear that this is not a good approach. Please
> > do not repost new version without that being addressed. If there are any
> > questions then feel free to ask for details.
> 
> You can get nid from pgdat (pgdat->node_id) and also pgdat from nid
> (NODE_DATA(nid)), so, __mem_cgroup_node_lruvec() only need one of them
> as parameter.

Exactly what I've said in the previous version review. I suspect that
the issue is that mem_cgroup_page_nodeinfo (based on page's node_id)
and the given pgdat can mismatch in the existing code but that shouldn't
be a real problem because the mismatch can only happen for lruvec->pgdat
== NULL unless I am missing something.

-- 
Michal Hocko
SUSE Labs

Re: [External] Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Shakeel Butt]

On Thu, Oct 29, 2020 at 9:09 AM Muchun Song <songmuchun@bytedance.com> wrote:
>
> On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > >
> > > Consider the following memcg hierarchy.
> > >
> > >                     root
> > >                    /    \
> > >                   A      B
> > >
> > > If we get the objcg of memcg A failed,
> >
> > Please fix the above statement.
>
> Sorry, could you be more specific, I don't quite understand.

Fix the grammar. Something like "If we failed to get the reference on
objcg of memcg A..."

Re: [External] Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Muchun Song]

On Fri, Oct 30, 2020 at 12:18 AM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Thu, Oct 29, 2020 at 9:09 AM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > > >
> > > > Consider the following memcg hierarchy.
> > > >
> > > >                     root
> > > >                    /    \
> > > >                   A      B
> > > >
> > > > If we get the objcg of memcg A failed,
> > >
> > > Please fix the above statement.
> >
> > Sorry, could you be more specific, I don't quite understand.
>
> Fix the grammar. Something like "If we failed to get the reference on
> objcg of memcg A..."

Got it. Sorry for my poor English, Thanks.

-- 
Yours,
Muchun

Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Roman Gushchin]

On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote:
> On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > Consider the following memcg hierarchy.
> >
> >                     root
> >                    /    \
> >                   A      B
> >
> > If we get the objcg of memcg A failed,
> 
> Please fix the above statement.
> 
> > 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>
> > ---
> >  changelog in v2:
> >  1. Do not use a comparison with the root_mem_cgroup
> >
> >  mm/memcontrol.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 1337775b04f3..8c8b4c3ed5a0 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2961,6 +2961,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;
> 
> Roman, in your cleanup, are you planning to have objcg for root memcg as well?

Yes. I'll just change the for loop to include the root_mem_cgroup.

Thanks!

Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Shakeel Butt]

On Thu, Oct 29, 2020 at 10:10 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote:
> > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > >
> > > Consider the following memcg hierarchy.
> > >
> > >                     root
> > >                    /    \
> > >                   A      B
> > >
> > > If we get the objcg of memcg A failed,
> >
> > Please fix the above statement.
> >
> > > 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>
> > > ---
> > >  changelog in v2:
> > >  1. Do not use a comparison with the root_mem_cgroup
> > >
> > >  mm/memcontrol.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 1337775b04f3..8c8b4c3ed5a0 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -2961,6 +2961,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;
> >
> > Roman, in your cleanup, are you planning to have objcg for root memcg as well?
>
> Yes. I'll just change the for loop to include the root_mem_cgroup.
>

Then do we really need this patch since it's not tagged for stable?

Re: [PATCH v2] mm: memcg/slab: Fix return child memcg objcg for root memcg

[Roman Gushchin]

On Thu, Oct 29, 2020 at 01:34:57PM -0700, Shakeel Butt wrote:
> On Thu, Oct 29, 2020 at 10:10 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote:
> > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > > >
> > > > Consider the following memcg hierarchy.
> > > >
> > > >                     root
> > > >                    /    \
> > > >                   A      B
> > > >
> > > > If we get the objcg of memcg A failed,
> > >
> > > Please fix the above statement.
> > >
> > > > 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>
> > > > ---
> > > >  changelog in v2:
> > > >  1. Do not use a comparison with the root_mem_cgroup
> > > >
> > > >  mm/memcontrol.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index 1337775b04f3..8c8b4c3ed5a0 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -2961,6 +2961,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;
> > >
> > > Roman, in your cleanup, are you planning to have objcg for root memcg as well?
> >
> > Yes. I'll just change the for loop to include the root_mem_cgroup.
> >
> 
> Then do we really need this patch since it's not tagged for stable?

A good question.

Honestly, I really doubt that obj_cgroup_tryget() can fail even with the current code.
But, formally speaking, it's possible. So there is likely no practical difference if
we'll apply this patch or not, even without the root handling cleanup.

But you're right, with the root handling cleanup, it will make even less of a difference.

Thanks!

Re: [PATCH v2] mm: memcg/slab: Fix use after free in obj_cgroup_charge

[Muchun Song]

On Wed, Oct 28, 2020 at 11:50 AM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The rcu_read_lock/unlock only can guarantee that the memcg will
> not be freed, but it cannot guarantee the success of css_get to
> memcg.
>
> If the whole process of a cgroup offlining is completed between
> reading a objcg->memcg pointer and bumping the css reference on
> another CPU, and there are exactly 0 external references to this
> memory cgroup (how we get to the obj_cgroup_charge() then?),
> css_get() can change the ref counter from 0 back to 1.
>
> Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Roman Gushchin <guro@fb.com>

Hi Andrew,

Maybe you forgot to add this to the queue for the merge window?

Thanks.

> ---
>  changelog in v2:
>  1. Add unlikely and update the commit log suggested by Roman.
>
>  mm/memcontrol.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8c8b4c3ed5a0..d9cdf899c6fc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3221,8 +3221,10 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
>          * independently later.
>          */
>         rcu_read_lock();
> +retry:
>         memcg = obj_cgroup_memcg(objcg);
> -       css_get(&memcg->css);
> +       if (unlikely(!css_tryget(&memcg->css)))
> +               goto retry;
>         rcu_read_unlock();
>
>         nr_pages = size >> PAGE_SHIFT;
> --
> 2.20.1
>


--
Yours,
Muchun