Re: [PATCH] mm: initialize variable for mem_cgroup_end_page_stat

[Michal Hocko]

On Thu 30-10-14 13:26:32, Johannes Weiner wrote:
> On Thu, Oct 30, 2014 at 04:31:59PM +0100, Michal Hocko wrote:
> > On Thu 30-10-14 10:24:47, Sasha Levin wrote:
> > > On 10/30/2014 10:14 AM, Johannes Weiner wrote:
> > > >> The problem is that you are attempting to read 'locked' when you call
> > > >> > mem_cgroup_end_page_stat(), so it gets used even before you enter the
> > > >> > function - and using uninitialized variables is undefined.
> > > > We are not using that value anywhere if !memcg.  What path are you
> > > > referring to?
> > > 
> > > You're using that value as soon as you are passing it to a function, it
> > > doesn't matter what happens inside that function.
> > 
> > I have discussed that with our gcc guys and you are right. Strictly
> > speaking the compiler is free to do
> > if (!memcg) abort();
> > mem_cgroup_end_page_stat(...);
> > 
> > but it is highly unlikely that this will ever happen. Anyway better be
> > safe than sorry. I guess the following should be sufficient and even
> > more symmetric:
> 
> The functional aspect of this is a terrible motivation for this
> change.  Sure the compiler could, but it doesn't, and it won't.
> 
> But there is some merit in keeping the checker's output meaningful as
> long as it doesn't obfuscate the interface too much.
> 
> > From 6c3e748af7ee24984477e850bb93d65f83914903 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@xxxxxxx>
> > Date: Thu, 30 Oct 2014 16:18:23 +0100
> > Subject: [PATCH] mm, memcg: fix potential undefined when for page stat
> >  accounting
> > 
> > since d7365e783edb (mm: memcontrol: fix missed end-writeback page
> > accounting) mem_cgroup_end_page_stat consumes locked and flags variables
> > directly rather than via pointers which might trigger C undefined
> > behavior as those variables are initialized only in the slow path of
> > mem_cgroup_begin_page_stat.
> > Although mem_cgroup_end_page_stat handles parameters correctly and
> > touches them only when they hold a sensible value it is caller which
> > loads a potentially uninitialized value which then might allow compiler
> > to do crazy things.
> 
> I'm not opposed to passing pointers into end_page_stat(), but please
> mention the checker in the changelog.

Done.
 
> > Fix this by using pointer parameters for both locked and flags. This is
> > even better from the API point of view because it is symmetrical to
> > mem_cgroup_begin_page_stat.
> 
> Uhm, locked and flags are return values in begin_page_stat() but input
> arguments in end_page_stat().  Symmetry obfuscates that, so that's not
> an upside at all.  It's a cost that we can pay to keep the checker

Well, I would use a typedef to obfuscate those values because nobody
except for mem_cgroup_{begin,end}_page_stat should touch them. But we
are not doing typedefs in kernel...

> benefits, but the underlying nastiness remains.  It comes from the
> fact that we use conditional locking to avoid the read-side spinlock,
> rather than using a reader-friendly lock to begin with.
 
> So let's change it to pointers, but at the same time be clear that
> this doesn't make the code better.  It just fixes the checker.

No it is not about the checker which is correct here actually. A simple
load to setup parameter from an uninitialized variable is an undefined
behavior (that load happens unconditionally). This has nothing to do
with the way how we use locked and flags inside the function.

New version with an updated changelog
---