[PATCH] fix comparison of unsigned expression < 0

[PATCH] fix comparison of unsigned expression < 0

[Haibo Li]

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.

Signed-off-by: Haibo Li <haibo.li@mediatek.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/
  202311270743.3oTCwYPd-lkp@intel.com/
---
 mm/kasan/report.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index e77facb62900..dafec2df0268 100644
--- 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
 	orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
 	/*
 	 * For faults near the shadow address for NULL, we can be fairly certain
-- 
2.18.0

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

[Andrew Morton]

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.

But really, some hardwired comparison with an absolute address seems
lazy.  If KASAN_SHADOW_OFFSET is variable on a per-architecture basis
then the expression which checks the validity of an arbitrary address
should also be per-architecture.

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

[Andrey Konovalov]

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.

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.

Thanks!

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

[Haibo Li]

> 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.
>
> 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.
>
> Thanks!

Thanks for the information.Let's keep it as unchanged.

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

[Dan Carpenter]

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

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

[Andrey Konovalov]

On Mon, Dec 4, 2023 at 5:12 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> > 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.

Hi Dan,

If this sounds like a good idea to you, please add an exception.

From the KASAN side, I think adding an exception for this case makes sense.

Thank you!