linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH stable 4.19] mm: memcontrol: switch to rcu protection in drain_all_stock()
@ 2024-02-26  3:01 GONG, Ruiqi
  2024-02-26  7:16 ` Greg KH
  0 siblings, 1 reply; 2+ messages in thread
From: GONG, Ruiqi @ 2024-02-26  3:01 UTC (permalink / raw)
  To: linux-kernel, stable, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Greg KH
  Cc: cgroups, linux-mm, Wang Weiyang, Xiu Jianfeng

From: Roman Gushchin <guro@fb.com>

commit e1a366be5cb4f849ec4de170d50eebc08bb0af20 upstream.

Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge")
introduced css_tryget()/css_put() calls in drain_all_stock(), which are
supposed to protect the target memory cgroup from being released during
the mem_cgroup_is_descendant() call.

However, it's not completely safe.  In theory, memcg can go away between
reading stock->cached pointer and calling css_tryget().

This can happen if drain_all_stock() races with drain_local_stock()
performed on the remote cpu as a result of a work, scheduled by the
previous invocation of drain_all_stock().

The race is a bit theoretical and there are few chances to trigger it, but
the current code looks a bit confusing, so it makes sense to fix it
anyway.  The code looks like as if css_tryget() and css_put() are used to
protect stocks drainage.  It's not necessary because stocked pages are
holding references to the cached cgroup.  And it obviously won't work for
works, scheduled on other cpus.

So, let's read the stock->cached pointer and evaluate the memory cgroup
inside a rcu read section, and get rid of css_tryget()/css_put() calls.

Link: http://lkml.kernel.org/r/20190802192241.3253165-1-guro@fb.com
Signed-off-by: Roman Gushchin <guro@fb.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Hillf Danton <hdanton@sina.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: stable@vger.kernel.org  # 4.19
Fixes: cdec2e4265df ("memcg: coalesce charging via percpu storage")
Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>
---

This patch [1] fixed a UAF problem in drain_all_stock() existed prior to
5.9, and following discussions [2] mentioned that the fix depends on an
RCU read protection to stock->cached (introduced in 5.4), which doesn't
existed in 4.19. So backport this part to 4.19 as well.

[1]: https://lore.kernel.org/all/20240221081801.69764-1-gongruiqi1@huawei.com/
[2]: https://lore.kernel.org/all/ZdXLgjpUfpwEwAe0@tiehlicka/

 mm/memcontrol.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c04296df1c7..d187bfb43b1f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2094,21 +2094,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)
 	for_each_online_cpu(cpu) {
 		struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);
 		struct mem_cgroup *memcg;
+		bool flush = false;
 
+		rcu_read_lock();
 		memcg = stock->cached;
-		if (!memcg || !stock->nr_pages || !css_tryget(&memcg->css))
-			continue;
-		if (!mem_cgroup_is_descendant(memcg, root_memcg)) {
-			css_put(&memcg->css);
-			continue;
-		}
-		if (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {
+		if (memcg && stock->nr_pages &&
+		    mem_cgroup_is_descendant(memcg, root_memcg))
+			flush = true;
+		rcu_read_unlock();
+
+		if (flush &&
+		    !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {
 			if (cpu == curcpu)
 				drain_local_stock(&stock->work);
 			else
 				schedule_work_on(cpu, &stock->work);
 		}
-		css_put(&memcg->css);
 	}
 	put_cpu();
 	mutex_unlock(&percpu_charge_mutex);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH stable 4.19] mm: memcontrol: switch to rcu protection in drain_all_stock()
  2024-02-26  3:01 [PATCH stable 4.19] mm: memcontrol: switch to rcu protection in drain_all_stock() GONG, Ruiqi
@ 2024-02-26  7:16 ` Greg KH
  0 siblings, 0 replies; 2+ messages in thread
From: Greg KH @ 2024-02-26  7:16 UTC (permalink / raw)
  To: GONG, Ruiqi
  Cc: linux-kernel, stable, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, cgroups, linux-mm,
	Wang Weiyang, Xiu Jianfeng

On Mon, Feb 26, 2024 at 11:01:40AM +0800, GONG, Ruiqi wrote:
> From: Roman Gushchin <guro@fb.com>
> 
> commit e1a366be5cb4f849ec4de170d50eebc08bb0af20 upstream.
> 
> Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge")
> introduced css_tryget()/css_put() calls in drain_all_stock(), which are
> supposed to protect the target memory cgroup from being released during
> the mem_cgroup_is_descendant() call.
> 
> However, it's not completely safe.  In theory, memcg can go away between
> reading stock->cached pointer and calling css_tryget().
> 
> This can happen if drain_all_stock() races with drain_local_stock()
> performed on the remote cpu as a result of a work, scheduled by the
> previous invocation of drain_all_stock().
> 
> The race is a bit theoretical and there are few chances to trigger it, but
> the current code looks a bit confusing, so it makes sense to fix it
> anyway.  The code looks like as if css_tryget() and css_put() are used to
> protect stocks drainage.  It's not necessary because stocked pages are
> holding references to the cached cgroup.  And it obviously won't work for
> works, scheduled on other cpus.
> 
> So, let's read the stock->cached pointer and evaluate the memory cgroup
> inside a rcu read section, and get rid of css_tryget()/css_put() calls.
> 
> Link: http://lkml.kernel.org/r/20190802192241.3253165-1-guro@fb.com
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Hillf Danton <hdanton@sina.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: stable@vger.kernel.org  # 4.19
> Fixes: cdec2e4265df ("memcg: coalesce charging via percpu storage")
> Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>
> ---
> 
> This patch [1] fixed a UAF problem in drain_all_stock() existed prior to
> 5.9, and following discussions [2] mentioned that the fix depends on an
> RCU read protection to stock->cached (introduced in 5.4), which doesn't
> existed in 4.19. So backport this part to 4.19 as well.

Now queued up, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-02-26  7:16 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-26  3:01 [PATCH stable 4.19] mm: memcontrol: switch to rcu protection in drain_all_stock() GONG, Ruiqi
2024-02-26  7:16 ` Greg KH

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).