From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751913AbbIKLSl (ORCPT ); Fri, 11 Sep 2015 07:18:41 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:37951 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751350AbbIKLSk (ORCPT ); Fri, 11 Sep 2015 07:18:40 -0400 From: Rasmus Villemoes To: Viresh Kumar Cc: gregkh@linuxfoundation.org, linaro-kernel@lists.linaro.org, Rafael Wysocki , sboyd@codeaurora.org, linux-kernel@vger.kernel.org (open list) Subject: Re: [PATCH] debugfs: don't access 4 bytes for a boolean Organization: D03 References: <3d6f65fa15363650f2d10ca58b9d9d243e98980f.1441961769.git.viresh.kumar@linaro.org> X-Hashcash: 1:20:150911:gregkh@linuxfoundation.org::ywHmm1WizmSr4iVj:000000000000000000000000000000000000R5c X-Hashcash: 1:20:150911:linux-kernel@vger.kernel.org::jWzKZKuLPugeFlBI:0000000000000000000000000000000000kxG X-Hashcash: 1:20:150911:viresh.kumar@linaro.org::orYI/qwcfAQqcah5:000000000000000000000000000000000000003xfQ X-Hashcash: 1:20:150911:linaro-kernel@lists.linaro.org::Dq+t1fmec1wuO26d:000000000000000000000000000000044Z7 X-Hashcash: 1:20:150911:sboyd@codeaurora.org::COSsaUqueA6dAC5W:0000000000000000000000000000000000000000043ut X-Hashcash: 1:20:150911:rjw@rjwysocki.net::OK0dmZSvl9wopM9u:000000000000000000000000000000000000000000007SWY Date: Fri, 11 Sep 2015 13:18:37 +0200 In-Reply-To: <3d6f65fa15363650f2d10ca58b9d9d243e98980f.1441961769.git.viresh.kumar@linaro.org> (Viresh Kumar's message of "Fri, 11 Sep 2015 14:36:06 +0530") Message-ID: <874mj1cigi.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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