From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752794AbbIKQl4 (ORCPT ); Fri, 11 Sep 2015 12:41:56 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:43438 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751553AbbIKQly (ORCPT ); Fri, 11 Sep 2015 12:41:54 -0400 Date: Fri, 11 Sep 2015 09:41:53 -0700 From: Greg KH To: Rasmus Villemoes Cc: Viresh Kumar , linaro-kernel@lists.linaro.org, Rafael Wysocki , sboyd@codeaurora.org, open list Subject: Re: [PATCH] debugfs: don't access 4 bytes for a boolean Message-ID: <20150911164153.GD5608@kroah.com> References: <3d6f65fa15363650f2d10ca58b9d9d243e98980f.1441961769.git.viresh.kumar@linaro.org> <874mj1cigi.fsf@rasmusvillemoes.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <874mj1cigi.fsf@rasmusvillemoes.dk> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 11, 2015 at 01:18:37PM +0200, Rasmus Villemoes wrote: > On Fri, Sep 11 2015, Viresh Kumar 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