[PATCH] overflow: Update is_non_negative() and is_negative() comment

Vincent Mailhol posted 1 patch 1 month ago
include/linux/overflow.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Vincent Mailhol 1 month ago
The is_non_negative() and is_negative() function-like macros initially
exist as a workaround to silence the -Wtype-limits warning. Now that
this warning is disabled, those two macros have lost their raison
d'être. Or so we thought.

In reality, smatch still produces a similar warning and so, it is
unfortunately still too early to undo this workaround.

Update the comment to point to smatch instead of GCC's -Wtype-limits.
Add a link to the thread in which this was discovered.

Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Link: https://lore.kernel.org/all/CANiq72=jRT+6+2PBgshsK-TpxPiRK70H-+3D6sYaN-fdfC83qw@mail.gmail.com/
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 include/linux/overflow.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 736f633b2d5f..d84194fc783b 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -37,8 +37,8 @@
 #define type_min(t)	__type_min(typeof(t))
 
 /*
- * Avoids triggering -Wtype-limits compilation warning,
- * while using unsigned data types to check a < 0.
+ * Avoids triggering "unsigned 'a' is never less than zero" smatch warning,
+ * Link: https://lore.kernel.org/all/acdd84b2-e893-419c-8a46-da55d695dda2@kernel.org
  */
 #define is_non_negative(a) ((a) > 0 || (a) == 0)
 #define is_negative(a) (!(is_non_negative(a)))
-- 
2.52.0
Re: [PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Dan Carpenter 1 month ago
Thanks Randy, for sending this to me.  I'm on the sparse list, but
I've been on vacation and haven't caught up with my email.  I can
easily silence this in Smatch.

regards,
dan carpenter

diff --git a/check_unsigned_lt_zero.c b/check_unsigned_lt_zero.c
index bfeb3261f91d..ac3e650704ce 100644
--- a/check_unsigned_lt_zero.c
+++ b/check_unsigned_lt_zero.c
@@ -105,7 +105,8 @@ static bool is_allowed_zero(struct expression *expr)
 	    strcmp(macro, "STRTO_H") == 0 ||
 	    strcmp(macro, "SUB_EXTEND_USTAT") == 0 ||
 	    strcmp(macro, "TEST_CASTABLE_TO_TYPE_VAR") == 0 ||
-	    strcmp(macro, "TEST_ONE_SHIFT") == 0)
+	    strcmp(macro, "TEST_ONE_SHIFT") == 0 ||
+	    strcmp(macro, "check_shl_overflow") == 0)
 		return true;
 	return false;
 }
Re: [PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Vincent Mailhol 1 month ago
On 03/01/2026 at 11:02, Dan Carpenter wrote:
> Thanks Randy, for sending this to me.  I'm on the sparse list, but
> I've been on vacation and haven't caught up with my email. 

Welcome back, hope you enjoyed your holidays!

>I can easily silence this in Smatch.

Thanks. I ran this locally, I can confirm that this silences the
warning. So:

Tested-by: Vincent Mailhol <mailhol@kernel.org>

> diff --git a/check_unsigned_lt_zero.c b/check_unsigned_lt_zero.c
> index bfeb3261f91d..ac3e650704ce 100644
> --- a/check_unsigned_lt_zero.c
> +++ b/check_unsigned_lt_zero.c
> @@ -105,7 +105,8 @@ static bool is_allowed_zero(struct expression *expr)
>  	    strcmp(macro, "STRTO_H") == 0 ||
>  	    strcmp(macro, "SUB_EXTEND_USTAT") == 0 ||
>  	    strcmp(macro, "TEST_CASTABLE_TO_TYPE_VAR") == 0 ||
> -	    strcmp(macro, "TEST_ONE_SHIFT") == 0)
> +	    strcmp(macro, "TEST_ONE_SHIFT") == 0 ||
> +	    strcmp(macro, "check_shl_overflow") == 0)

But, for the long term, wouldn't it better to just ignore all the code
coming from macro extensions instead of maintaining this allow-list?

>  		return true;
>  	return false;
>  }


Yours sincerely,
Vincent Mailhol
Re: [PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Dan Carpenter 1 month ago
On Sat, Jan 03, 2026 at 12:10:45PM +0100, Vincent Mailhol wrote:
> On 03/01/2026 at 11:02, Dan Carpenter wrote:
> > Thanks Randy, for sending this to me.  I'm on the sparse list, but
> > I've been on vacation and haven't caught up with my email. 
> 
> Welcome back, hope you enjoyed your holidays!
> 
> >I can easily silence this in Smatch.
> 
> Thanks. I ran this locally, I can confirm that this silences the
> warning. So:
> 
> Tested-by: Vincent Mailhol <mailhol@kernel.org>
> 
> > diff --git a/check_unsigned_lt_zero.c b/check_unsigned_lt_zero.c
> > index bfeb3261f91d..ac3e650704ce 100644
> > --- a/check_unsigned_lt_zero.c
> > +++ b/check_unsigned_lt_zero.c
> > @@ -105,7 +105,8 @@ static bool is_allowed_zero(struct expression *expr)
> >  	    strcmp(macro, "STRTO_H") == 0 ||
> >  	    strcmp(macro, "SUB_EXTEND_USTAT") == 0 ||
> >  	    strcmp(macro, "TEST_CASTABLE_TO_TYPE_VAR") == 0 ||
> > -	    strcmp(macro, "TEST_ONE_SHIFT") == 0)
> > +	    strcmp(macro, "TEST_ONE_SHIFT") == 0 ||
> > +	    strcmp(macro, "check_shl_overflow") == 0)
> 
> But, for the long term, wouldn't it better to just ignore all the code
> coming from macro extensions instead of maintaining this allow-list?
> 

Of course, that idea occured to me, but so far the allow list is not
very burdensome to maintain.  I maybe should disable it for all
macros unless the --spammy option is used...

regards,
dan carpenter
Re: [PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Vincent Mailhol 1 month ago
On 03/01/2026 at 17:56, Dan Carpenter wrote:
> On Sat, Jan 03, 2026 at 12:10:45PM +0100, Vincent Mailhol wrote:
>> On 03/01/2026 at 11:02, Dan Carpenter wrote:
>>> Thanks Randy, for sending this to me.  I'm on the sparse list, but
>>> I've been on vacation and haven't caught up with my email. 
>>
>> Welcome back, hope you enjoyed your holidays!
>>
>>> I can easily silence this in Smatch.
>>
>> Thanks. I ran this locally, I can confirm that this silences the
>> warning. So:
>>
>> Tested-by: Vincent Mailhol <mailhol@kernel.org>
>>
>>> diff --git a/check_unsigned_lt_zero.c b/check_unsigned_lt_zero.c
>>> index bfeb3261f91d..ac3e650704ce 100644
>>> --- a/check_unsigned_lt_zero.c
>>> +++ b/check_unsigned_lt_zero.c
>>> @@ -105,7 +105,8 @@ static bool is_allowed_zero(struct expression *expr)
>>>  	    strcmp(macro, "STRTO_H") == 0 ||
>>>  	    strcmp(macro, "SUB_EXTEND_USTAT") == 0 ||
>>>  	    strcmp(macro, "TEST_CASTABLE_TO_TYPE_VAR") == 0 ||
>>> -	    strcmp(macro, "TEST_ONE_SHIFT") == 0)
>>> +	    strcmp(macro, "TEST_ONE_SHIFT") == 0 ||
>>> +	    strcmp(macro, "check_shl_overflow") == 0)
>>
>> But, for the long term, wouldn't it better to just ignore all the code
>> coming from macro extensions instead of maintaining this allow-list?
>>
> 
> Of course, that idea occured to me, but so far the allow list is not
> very burdensome to maintain.

Indeed, but my concern was more on how people would treat such smatch
warnings coming from the kernel test robot. It is very uncommon to have
an allow-list hard coded into the static analyzer. Actually, this is the
first time I see this. My fear here is that people will just uglify the
code rather than sending a patch to extend the allow list in smatch.

> I maybe should disable it for all macros unless the --spammy option is used...

IMHO, that would be an even better approach. That said, I am happy
enough with your previous patch which resolves my issue and which is way
better than updating the is_non_negative() and is_negative() comments as
I did in my patch!


Yours sincerely,
Vincent Mailhol
Re: [PATCH] overflow: Update is_non_negative() and is_negative() comment
Posted by Dan Carpenter 1 month ago
On Sat, Jan 03, 2026 at 08:40:17PM +0100, Vincent Mailhol wrote:
> On 03/01/2026 at 17:56, Dan Carpenter wrote:
> > On Sat, Jan 03, 2026 at 12:10:45PM +0100, Vincent Mailhol wrote:
> >> On 03/01/2026 at 11:02, Dan Carpenter wrote:
> >>> Thanks Randy, for sending this to me.  I'm on the sparse list, but
> >>> I've been on vacation and haven't caught up with my email. 
> >>
> >> Welcome back, hope you enjoyed your holidays!
> >>
> >>> I can easily silence this in Smatch.
> >>
> >> Thanks. I ran this locally, I can confirm that this silences the
> >> warning. So:
> >>
> >> Tested-by: Vincent Mailhol <mailhol@kernel.org>
> >>
> >>> diff --git a/check_unsigned_lt_zero.c b/check_unsigned_lt_zero.c
> >>> index bfeb3261f91d..ac3e650704ce 100644
> >>> --- a/check_unsigned_lt_zero.c
> >>> +++ b/check_unsigned_lt_zero.c
> >>> @@ -105,7 +105,8 @@ static bool is_allowed_zero(struct expression *expr)
> >>>  	    strcmp(macro, "STRTO_H") == 0 ||
> >>>  	    strcmp(macro, "SUB_EXTEND_USTAT") == 0 ||
> >>>  	    strcmp(macro, "TEST_CASTABLE_TO_TYPE_VAR") == 0 ||
> >>> -	    strcmp(macro, "TEST_ONE_SHIFT") == 0)
> >>> +	    strcmp(macro, "TEST_ONE_SHIFT") == 0 ||
> >>> +	    strcmp(macro, "check_shl_overflow") == 0)
> >>
> >> But, for the long term, wouldn't it better to just ignore all the code
> >> coming from macro extensions instead of maintaining this allow-list?
> >>
> > 
> > Of course, that idea occured to me, but so far the allow list is not
> > very burdensome to maintain.
> 
> Indeed, but my concern was more on how people would treat such smatch
> warnings coming from the kernel test robot. It is very uncommon to have
> an allow-list hard coded into the static analyzer. Actually, this is the
> first time I see this. My fear here is that people will just uglify the
> code rather than sending a patch to extend the allow list in smatch.
> 

People need to learn to ignore false positives.  The zero day bot sends
a one time email and if you just delete it, then it's gone forever.
Forget about it.

It's really limitting to try be totally static checker clean.  Things
like passing a zero to ERR_PTR() for example.  It's a perfectly valid way
to return NULL and the fs/ subsystem uses this a lot.  But 80% when you
see it in new code, then it's a bug where they returned the wrong
variable or something.  It's a high quality warning.  The solution is to
look at it one time, when the code is fresh and then never look at it
again.

In this case when I disable the unsigned < 0 checking for macros then it
means that if someone does if (WARN_ON(u32_var < 0)) { it's silenced.  I
don't want that.

I could create a much better way to silence false positives like this
if I passed an expression pointer to the sm_warning()...  It's a bit of
a re-work.

regards,
dan carpenter












It's really limitting
if we can only check 

> > I maybe should disable it for all macros unless the --spammy option is used...
> 
> IMHO, that would be an even better approach. That said, I am happy
> enough with your previous patch which resolves my issue and which is way
> better than updating the is_non_negative() and is_negative() comments as
> I did in my patch!
> 
> 
> Yours sincerely,
> Vincent Mailhol