Re: [PATCH] debugfs: don't access 4 bytes for a boolean

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Sep 11, 2015 at 01:18:37PM +0200, Rasmus Villemoes wrote:
> On Fri, Sep 11 2015, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > Long back 'bool' type used to be a typecast to 'int', but that changed
> > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes
> > just a byte. Anyway, the bool type in kernel is used to store true/false
> > or 1/0 only. So, accessing a single byte should be enough.
> >
> > The problem with current code is that it reads/writes 4 bytes for a
> > boolean, which will read/update 3 excess bytes following the boolean
> > variable. And that can lead to hard to fix bugs. It was a nightmare to
> > crack this one.
> >
> > The debugfs code had this bug since the first time it got introduced,
> > but was never got caught, strange. Maybe the bool variables (monitored
> > by debugfs) were followed by an 'int' or something bigger and the pad
> > bytes made sure, we never see this issue.
> >
> > But the OPP (Operating performance points) library have three booleans
> > allocated to contiguous bytes and this bug got hit quite soon (The
> > debugfs support for OPP is yet to be merged).
> >
> > Fix this by changing type of 'val' pointer to u8 type, so that we only
> > access a single byte.

Nice catch, but let's do this a bit differently (see below).

> If the pointed-to type is supposed to be a bool aka _Bool, shouldn't you
> cast to bool* instead of assuming sizeof(bool)==1? It's probably
> non-existing, but imagine a big-endian architecture where
> sizeof(bool)==4; you'd end up reading/writing the wrong byte.
> 
> > Also, there is another problem I see, which probably should be fixed as
> > well. But I wanted to hear from you before trying to patch the kernel
> > for this.
> >
> > debugfs_create_bool() declares the pointer to be of type u32 *.
> > Shouldn't that be changed to u8 *? There are many users which are
> > typecasting the variables to make debugfs API happy :)
> 
> Hm, yes, that's annoying. But since most people currently do pass an
> u32, treating the pointer as u8* is wrong on big-endian (though of
> course it doesn't matter if the value is only ever checked for being
> zero/non-zero). So it would probably be better to change the
> debugfs_create_bool to actually expect a bool* - there aren't _that_
> many current callers, and some are obviously aware of the weirdness
> (with comments such as 'must be u32 for debugfs_create_bool').

I agree, let's just fix up the api to have the correct type.  I think
when I originally wrote the function, we didn't have a 'bool' type that
was "native" in the c standard.

thanks,

greg k-h