| Message ID | 20220220144606.5695-1-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Initialize ret to 0 in scrub_simple_mirror() | expand |
On 2022/2/20 22:46, Souptick Joarder wrote: > From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> > Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.com> Although the patch is not yet merged, but I have to say, it's a false alert. 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 &&
On Mon, Feb 21, 2022 at 5:46 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2022/2/20 22:46, Souptick Joarder wrote: > > From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> > > Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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 :) > > 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 &&
On 2022/2/22 15:50, Souptick Joarder wrote: > On Mon, Feb 21, 2022 at 5:46 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> On 2022/2/20 22:46, Souptick Joarder wrote: >>> From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> >>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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 && >
On Tue, Feb 22, 2022 at 1:34 PM Qu Wenruo <wqu@suse.com> wrote: > > > > On 2022/2/22 15:50, Souptick Joarder wrote: > > On Mon, Feb 21, 2022 at 5:46 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > >> > >> > >> > >> On 2022/2/20 22:46, Souptick Joarder wrote: > >>> From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> > >>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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. Hmm, kernel test robot reported similar false alert in few other modules as well. > > > 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 && > > >
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@gmx.com> wrote: >>> >>> >>> >>> On 2022/2/20 22:46, Souptick Joarder wrote: >>>> From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> >>>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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 && >> >
On 2022/2/24 17:48, Yujie Liu wrote: > Hi, > > Sorry for the noise of this false alert. No problem at all, in fact your bots are awesome detecting real bugs like previously it detects some uninitialized values from my patches. Kudos to your guys! LKP bots really rocks. > > 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. BTW, do performance benchmarks also go through the same procedure? Although your bots are awesome at detect compiling warning/errors, sometimes it's not that straightforward to reproduce the same performance regressions. So it may be worthy some extra steps to verify certain performance regressions. Thanks, Qu > > 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@gmx.com> >>> wrote: >>>> >>>> >>>> >>>> On 2022/2/20 22:46, Souptick Joarder wrote: >>>>> From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> >>>>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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 && >>> >>
On Thu, Feb 24, 2022 at 3:18 PM Yujie Liu <yujie.liu@intel.com> wrote: > > 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? https://marc.info/?l=linux-mm&m=164487037605771&w=2 > 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@gmx.com> wrote: > >>> > >>> > >>> > >>> On 2022/2/20 22:46, Souptick Joarder wrote: > >>>> From: "Souptick Joarder (HPE)" <jrdr.linux@gmail.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@intel.com> > >>>> Signed-off-by: Souptick Joarder (HPE) <jrdr.linux@gmail.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 && > >> > >
Hi Qu, On 2/24/2022 17:56, Qu Wenruo wrote: >> >> 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. > > BTW, do performance benchmarks also go through the same procedure? > Yes, runtime reports(including boot test, performance benchmarks, etc.) also go through internal check procedure, but compared with build issues, sometimes we are not that confident about performance regressions. > Although your bots are awesome at detect compiling warning/errors, > sometimes it's not that straightforward to reproduce the same > performance regressions. > > So it may be worthy some extra steps to verify certain performance > regressions. > Runtime issues are much complicated than build issues, the performance may fluctuate depending on different software and hardware environment, so not that straightforward to reproduce. Please feel free to contact LKP folks if found any false reports of regressions or any issues in reproducing steps. We can learn more experience from user feedback and keep optimizing our work flow to improve the accuracy of catching real regressions. Thanks, Yujie
Hi Souptick, On 2/25/2022 13:16, Souptick Joarder wrote: > On Thu, Feb 24, 2022 at 3:18 PM Yujie Liu <yujie.liu@intel.com> wrote: >> >> Souptick, could you help to provide the original report by link or attachment? > > https://marc.info/?l=linux-mm&m=164487037605771&w=2 > this link is a notification mail of build status, but not a formal report, so there may be many false alerts in this mail, please ignore them. Thanks, Yujie
Hi Souptick, On 2/25/2022 18:02, Yujie Liu wrote: > Hi Souptick, > > On 2/25/2022 13:16, Souptick Joarder wrote: >> On Thu, Feb 24, 2022 at 3:18 PM Yujie Liu <yujie.liu@intel.com> wrote: >>> >>> Souptick, could you help to provide the original report by link or attachment? >> >> https://marc.info/?l=linux-mm&m=164487037605771&w=2 >> > > this link is a notification mail of build status, but not a formal report, > so there may be many false alerts in this mail, please ignore them. > Sorry, previous info was not accurate, here are some updates: In a build notification mail(such as above link https://marc.info/?l=linux-mm&m=164487037605771&w=2), there is a hint line like: possible Error/Warning in current branch (please contact us if interested): the errors/warnings in this section are not fully verified like the ones generated by static analysis tools. We will make this line clearer to mention that they could be false positives. Thanks, Yujie
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 &&