Re: [PATCH] mm: change memcg->oom_group access with atomic operations

[Michal Hocko]

On Mon 20-02-23 23:06:24, Shakeel Butt wrote:
> On Mon, Feb 20, 2023 at 01:09:44PM -0800, Roman Gushchin wrote:
> > On Mon, Feb 20, 2023 at 11:16:38PM +0800, Yue Zhao wrote:
> > > The knob for cgroup v2 memory controller: memory.oom.group
> > > will be read and written simultaneously by user space
> > > programs, thus we'd better change memcg->oom_group access
> > > with atomic operations to avoid concurrency problems.
> > > 
> > > Signed-off-by: Yue Zhao <findns94@xxxxxxxxx>
> > 
> > Hi Yue!
> > 
> > I'm curious, have any seen any real issues which your patch is solving?
> > Can you, please, provide a bit more details.
> > 
> 
> IMHO such details are not needed. oom_group is being accessed
> concurrently and one of them can be a write access. At least
> READ_ONCE/WRITE_ONCE is needed here. Most probably syzbot didn't
> catch this race because it does not know about the memory.oom.group
> interface.

I do agree with Roman here. It is _always_ good to mention whether this
is a tool/review or actual bug triggered fix. Also {READ,WRITE}_ONCE doesn't
guarantee atomicity so it would be good to rephrase the changelog.
Something like:
The knob for cgroup v2 memory controller: memory.oom.group
is not protected by any locking so it can be modified while it is used.
This is not an actual problem because races are unlikely (the knob is
usually configured long before any workloads hits actual memcg oom)
but it is better to use READ_ONCE/WRITE_ONCE to prevent compiler from
doing anything funky.

This patch is not fixing any actual user visible bug but it is in line
of a standard practice.
-- 
Michal Hocko
SUSE Labs