linux-kernel - Re: [PATCH] bcache: avoid clang -Wunintialized warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b2f72346-47e1-0a47-64bc-d35490e60818@suse.de>
Date:   Fri, 26 Apr 2019 22:27:17 +0800
From:   Coly Li <colyli@...e.de>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Kent Overstreet <kent.overstreet@...il.com>,
        clang-built-linux@...glegroups.com,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Jens Axboe <axboe@...nel.dk>,
        Shenghui Wang <shhuiw@...mail.com>,
        Hannes Reinecke <hare@...e.com>, linux-bcache@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bcache: avoid clang -Wunintialized warning

On 2019/4/26 9:01 下午, Nathan Chancellor wrote:
> On Fri, Apr 26, 2019 at 10:43:01AM +0800, Coly Li wrote:
>> On 2019/4/26 2:08 上午, Nathan Chancellor wrote:
>>> On Fri, Mar 22, 2019 at 03:35:00PM +0100, Arnd Bergmann wrote:
>>>> clang has identified a code path in which it thinks a
>>>> variable may be unused:
>>>>
>>>> drivers/md/bcache/alloc.c:333:4: error: variable 'bucket' is used uninitialized whenever 'if' condition is false
>>>>       [-Werror,-Wsometimes-uninitialized]
>>>>                         fifo_pop(&ca->free_inc, bucket);
>>>>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> drivers/md/bcache/util.h:219:27: note: expanded from macro 'fifo_pop'
>>>>  #define fifo_pop(fifo, i)       fifo_pop_front(fifo, (i))
>>>>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>> drivers/md/bcache/util.h:189:6: note: expanded from macro 'fifo_pop_front'
>>>>         if (_r) {                                                       \
>>>>             ^~
>>>> drivers/md/bcache/alloc.c:343:46: note: uninitialized use occurs here
>>>>                         allocator_wait(ca, bch_allocator_push(ca, bucket));
>>>>                                                                   ^~~~~~
>>>> drivers/md/bcache/alloc.c:287:7: note: expanded from macro 'allocator_wait'
>>>>                 if (cond)                                               \
>>>>                     ^~~~
>>>> drivers/md/bcache/alloc.c:333:4: note: remove the 'if' if its condition is always true
>>>>                         fifo_pop(&ca->free_inc, bucket);
>>>>                         ^
>>>> drivers/md/bcache/util.h:219:27: note: expanded from macro 'fifo_pop'
>>>>  #define fifo_pop(fifo, i)       fifo_pop_front(fifo, (i))
>>>>                                 ^
>>>> drivers/md/bcache/util.h:189:2: note: expanded from macro 'fifo_pop_front'
>>>>         if (_r) {                                                       \
>>>>         ^
>>>> drivers/md/bcache/alloc.c:331:15: note: initialize the variable 'bucket' to silence this warning
>>>>                         long bucket;
>>>>                                    ^
>>>>
>>>> This cannot happen in practice because we only enter the loop
>>>> if there is at least one element in the list.
>>>>
>>>> Slightly rearranging the code makes this clearer to both the
>>>> reader and the compiler, which avoids the warning.
>>>>
>>>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>>>> ---
>>>>  drivers/md/bcache/alloc.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
>>>> index 5002838ea476..f8986effcb50 100644
>>>> --- a/drivers/md/bcache/alloc.c
>>>> +++ b/drivers/md/bcache/alloc.c
>>>> @@ -327,10 +327,11 @@ static int bch_allocator_thread(void *arg)
>>>>  		 * possibly issue discards to them, then we add the bucket to
>>>>  		 * the free list:
>>>>  		 */
>>>> -		while (!fifo_empty(&ca->free_inc)) {
>>>> +		while (1) {
>>>>  			long bucket;
>>>>  
>>>> -			fifo_pop(&ca->free_inc, bucket);
>>>> +			if (!fifo_pop(&ca->free_inc, bucket))
>>>> +				break;
>>>>  
>>>>  			if (ca->discard) {
>>>>  				mutex_unlock(&ca->set->bucket_lock);
>>>> -- 
>>>> 2.20.0
>>>>
>>>
>>> Hi all,
>>>
>>> Could someone please review/pick this up? This is one of two remaining
>>> -Wsometimes-uninitialized warnings among arm, arm64, and x86_64
>>> all{yes,mod}config and I'd like to get it turned on as soon as possible
>>> to catch more bugs.
>>
>> Hi Nathan,
>>
>> It is in Jens' block tree for-next branch already, for Linux v5.2 merge
>> window.
>>
>> Thanks.
>>
>> -- 
>>
>> Coly Li
> 
> Hi Coly,
> 
> Thank you for the reply and heads up, it hadn't hit -next when I sent
> that message and I didn't check Jens' tree.
> 
> I appreciate you picking it up!

Hi Nathan,

You are welcome, and thanks for the contribution to make bcache better :-)

-- 

Coly Li