Re: [PATCH] mm: initialize variable for mem_cgroup_end_page_stat

[Johannes Weiner]

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.

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

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/