Re: [PATCH] mm: memcg: initialize *locked in memcg1_oom_prepare() stub

[SeongJae Park <sj@kernel.org>]

On Fri, 26 Jun 2026 14:53:20 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Fri, Jun 26, 2026 at 05:43:02AM -0700, Breno Leitao wrote:
> > mem_cgroup_oom() passes an uninitialized "locked" to memcg1_oom_prepare()
> > and reads it back in memcg1_oom_finish():
> > 
> > 	bool locked, ret;
> > 	...
> > 	if (!memcg1_oom_prepare(memcg, &locked))
> > 		return false;
> > 	ret = mem_cgroup_out_of_memory(memcg, mask, order);
> > 	memcg1_oom_finish(memcg, locked);
> > 
> > This relies on memcg1_oom_prepare() setting *locked whenever it returns
> > true.  The CONFIG_MEMCG_V1=y version does, but the stub used when
> > CONFIG_MEMCG_V1=n returns true without touching *locked, so
> > memcg1_oom_finish() consumes an uninitialized value.  On a memcg OOM this
> > is reported by UBSAN:
> > 
> >   UBSAN: invalid-load in mm/memcontrol.c:1932:27
> >   load of value 0 is not a valid value for type 'bool' (aka '_Bool')
> > 
> > Initialize *locked to false in the stub; with cgroup v1 compiled out
> > there is no OOM lock to take.
> > 
> > Fixes: e93d4166b40a ("mm: memcg: put cgroup v1-specific code under a config option")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> I prefer this way over the idea to initialize in the caller. For the
> actual implementation, the protocol is that the thing is initialized
> when the function returns true. This version of the fix maintains that
> for the dummy as well:

I agree.  I also feel the caller code is _slightly_ easier to read as is, than
adding the initialization there.  If it is initialized there, I would assume it
will be used somewhere.  But after finding out it is not used for early return
cases including memcg1_oom_prepare() reuturning false case, I would be confused
about the inefficiency.  Using a variable after passing its pointer to a
function depending on the function's return value makes me assume the variable
will be set inside the function.

The code is simple enough to read in any way, and my taste is sometimes just
weird, though.

Anyway nice fix, thank you!

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]