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.
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').
Rasmus