Re: [PATCH] fix comparison of unsigned expression < 0

[Dan Carpenter <dan.carpenter@linaro.org>]

On Wed, Nov 29, 2023 at 04:01:47AM +0100, Andrey Konovalov wrote:
> On Wed, Nov 29, 2023 at 2:22 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 28 Nov 2023 15:55:32 +0800 Haibo Li <haibo.li@mediatek.com> wrote:
> >
> > > Kernel test robot reported:
> > >
> > > '''
> > > mm/kasan/report.c:637 kasan_non_canonical_hook() warn:
> > > unsigned 'addr' is never less than zero.
> > > '''
> > > The KASAN_SHADOW_OFFSET is 0 on loongarch64.
> > >
> > > To fix it,check the KASAN_SHADOW_OFFSET before do comparison.
> > >
> > > --- a/mm/kasan/report.c
> > > +++ b/mm/kasan/report.c
> > > @@ -634,10 +634,10 @@ void kasan_non_canonical_hook(unsigned long addr)
> > >  {
> > >       unsigned long orig_addr;
> > >       const char *bug_type;
> > > -
> > > +#if KASAN_SHADOW_OFFSET > 0
> > >       if (addr < KASAN_SHADOW_OFFSET)
> > >               return;
> > > -
> > > +#endif
> >
> > We'd rather not add ugly ifdefs for a simple test like this.  If we
> > replace "<" with "<=", does it fix?  I suspect that's wrong.
> 
> Changing the comparison into "<=" would be wrong.
> 

I would say that changing it to <= is seldom the correct thing.  I've
wanted to make that trigger a warning as well.

> But I actually don't think we need to fix anything here.
> 
> This issue looks quite close to a similar comparison with 0 issue
> Linus shared his opinion on here:
> 
> https://lore.kernel.org/all/Pine.LNX.4.58.0411230958260.20993@ppc970.osdl.org/
> 
> I don't know if the common consensus with the regard to issues like
> that changed since then. But if not, perhaps we can treat this kernel
> test robot report as a false positive.

I would say that the consensus has changed somewhere around 2015 or
so.  Unsigned comparisons to zero used to be one of the most common
types of bugs in new code but now almost all subsystems have turned on
the GCC warning for this.

However, this is a Smatch warning and I agree with Linus on this.  For
example, Smatch doesn't complain about the example code the Linus
mentioned.

	if (a < 0 || a > X)

And in this case, it's a one liner fix for me to add KASAN_SHADOW_OFFSET
as an allowed macro and silence the warning.

regards,
dan carpenter