| Message ID | 20231128075532.110251-1-haibo.li@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix comparison of unsigned expression < 0 | expand |
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.
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!
> 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.
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
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!
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
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(-)