Messages in this thread | | | From | Roman Gushchin <> | Subject | Re: [PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock() | Date | Fri, 2 Aug 2019 17:00:34 +0000 |
| |
On Fri, Aug 02, 2019 at 10:59:47AM +0200, Michal Hocko wrote: > On Fri 02-08-19 10:04:22, Michal Hocko wrote: > > On Thu 01-08-19 16:35:13, Roman Gushchin wrote: > > > 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(). > > > > I have to remember how is this whole thing supposed to work, it's been > > some time since I've looked into that. > > OK, I guess I remember now and I do not see how the race is possible. > Stock cache is keeping its memcg alive because it elevates the reference > counting for each cached charge. And that should keep the whole chain up > to the root (of draining) alive, no? Or do I miss something, could you > generate a sequence of events that would lead to use-after-free?
Right, but it's true when you reading a local percpu stock. But here we read a remote stock->cached pointer, which can be cleared by a remote concurrent drain_local_stock() execution.
In theory, it could be the last reference, and the memcg can be destroyed remotely, so we end up trying to call css_tryget() over freed memory.
The race is theoretical, but as I wrote in the thread, I think that it's still worth fixing, because the current code looks confusing (and this confirms my feelings).
Thanks!
| |