linux-kernel - Re: [PATCH] btrfs: Initialize ret to 0 in scrub_simple

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Feb 2022 17:48:17 +0800
From:   Yujie Liu <yujie.liu@...el.com>
To:     Qu Wenruo <wqu@...e.com>, Souptick Joarder <jrdr.linux@...il.com>
CC:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        <dsterba@...e.com>, <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        <linux-btrfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <llvm@...ts.linux.dev>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] btrfs: Initialize ret to 0 in scrub_simple_mirror()

Hi,

Sorry for the noise of this false alert.

For clang analyzer reports, usually we do internal check firstly. We'll send out the
report only when we think that it is highly possible to be a true alert.

We scanned our report history and found this report was produced on 1/26, but it was
still in the internal check domain and was not likely to be sent to Qu or mailing lists,
so we are kind of confusing about this consequence.

Souptick, could you help to provide the original report by link or attachment?
Then we can do some check to figure out whether we have any flaw in our process.

Thanks,
Yujie

On 2/22/2022 16:04, Qu Wenruo wrote:
> 
> 
> On 2022/2/22 15:50, Souptick Joarder wrote:
>> On Mon, Feb 21, 2022 at 5:46 AM Qu Wenruo <quwenruo.btrfs@....com> wrote:
>>>
>>>
>>>
>>> On 2022/2/20 22:46, Souptick Joarder wrote:
>>>> From: "Souptick Joarder (HPE)" <jrdr.linux@...il.com>
>>>>
>>>> Kernel test robot reported below warning ->
>>>> fs/btrfs/scrub.c:3439:2: warning: Undefined or garbage value
>>>> returned to caller [clang-analyzer-core.uninitialized.UndefReturn]
>>>>
>>>> Initialize ret to 0.
>>>>
>>>> Reported-by: kernel test robot <lkp@...el.com>
>>>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@...il.com>
>>>
>>> Although the patch is not yet merged, but I have to say, it's a false alert.
>>
>> Yes, I agree it is a false positive but this patch will at least keep
>> kernel test robot happy :)
> 
> I'd say we should enhance the compiler to fix the false alert.
> 
> Thus adding LLVM list here is correct.
> 
> 
> To me, the root problem is that, we lack the hint to allow clang to know that, @logical_length passed in would not cause u64 overflow.
> 
> Unfortunately the sanity check to prevent overflow is hidden far away inside tree-checker.c.
> 
> Maybe some ASSERT() for overflow check would help LLVM to know that?
> 
> Thanks,
> Qu
> 
>>>
>>> Firstly, the while loop will always get at least one run.
>>>
>>> Secondly, in that loop, we either set ret to some error value and break,
>>> or after at least one find_first_extent_item() and scrub_extent() call,
>>> we increase cur_logical and reached the limit of the while loop and exit.
>>>
>>> So there is no possible routine to leave @ret uninitialized and returned
>>> to caller.
>>>
>>> Thanks,
>>> Qu
>>>
>>>> ---
>>>>    fs/btrfs/scrub.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>>> index 4baa8e43d585..5ca7e5ffbc96 100644
>>>> --- a/fs/btrfs/scrub.c
>>>> +++ b/fs/btrfs/scrub.c
>>>> @@ -3325,7 +3325,7 @@ static int scrub_simple_mirror(struct scrub_ctx *sctx,
>>>>        const u32 max_length = SZ_64K;
>>>>        struct btrfs_path path = {};
>>>>        u64 cur_logical = logical_start;
>>>> -     int ret;
>>>> +     int ret = 0;
>>>>
>>>>        /* The range must be inside the bg */
>>>>        ASSERT(logical_start >= bg->start &&
>>
>