[PATCH net-next v2] af_unix: Fix undefined 'other' error

[PATCH net-next v2] af_unix: Fix undefined 'other' error

[Purva Yeshi]

Fix issue detected by smatch tool:
An "undefined 'other'" error occur in __releases() annotation.

Fix an undefined 'other' error in unix_wait_for_peer() caused by  
__releases(&unix_sk(other)->lock) being placed before 'other' is in  
scope. Since AF_UNIX does not use Sparse annotations, remove it to fix  
the issue.  

Eliminate the error without affecting functionality.  

Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
---
V1 - https://lore.kernel.org/lkml/20250209184355.16257-1-purvayeshi550@gmail.com/
V2 - Remove __releases() annotation as AF_UNIX does not use Sparse annotations.
 net/unix/af_unix.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 34945de1f..319153850 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1508,7 +1508,6 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
 }
 
 static long unix_wait_for_peer(struct sock *other, long timeo)
-	__releases(&unix_sk(other)->lock)
 {
 	struct unix_sock *u = unix_sk(other);
 	int sched;
-- 
2.34.1

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Joe Damato]

On Mon, Feb 10, 2025 at 01:20:06PM +0530, Purva Yeshi wrote:
> Fix issue detected by smatch tool:
> An "undefined 'other'" error occur in __releases() annotation.
> 
> Fix an undefined 'other' error in unix_wait_for_peer() caused by  
> __releases(&unix_sk(other)->lock) being placed before 'other' is in  
> scope. Since AF_UNIX does not use Sparse annotations, remove it to fix  
> the issue.  
> 
> Eliminate the error without affecting functionality.  
> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
> V1 - https://lore.kernel.org/lkml/20250209184355.16257-1-purvayeshi550@gmail.com/
> V2 - Remove __releases() annotation as AF_UNIX does not use Sparse annotations.
>  net/unix/af_unix.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Kuniyuki Iwashima]

From: Purva Yeshi <purvayeshi550@gmail.com>
Date: Mon, 10 Feb 2025 13:20:06 +0530
> Fix issue detected by smatch tool:
> An "undefined 'other'" error occur in __releases() annotation.
> 
> Fix an undefined 'other' error in unix_wait_for_peer() caused by  
> __releases(&unix_sk(other)->lock) being placed before 'other' is in  
> scope. Since AF_UNIX does not use Sparse annotations, remove it to fix  
> the issue.  
> 
> Eliminate the error without affecting functionality.  

The 5 lines of the 3 sentences above have trailing double spaces.
You may want to configure your editor to highlight them.

e.g. for emacs

(setq-default show-trailing-whitespace t)


> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>

Otherwise looks good.

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Purva Yeshi]

On 11/02/25 06:02, Kuniyuki Iwashima wrote:
> From: Purva Yeshi <purvayeshi550@gmail.com>
> Date: Mon, 10 Feb 2025 13:20:06 +0530
>> Fix issue detected by smatch tool:
>> An "undefined 'other'" error occur in __releases() annotation.
>>
>> Fix an undefined 'other' error in unix_wait_for_peer() caused by
>> __releases(&unix_sk(other)->lock) being placed before 'other' is in
>> scope. Since AF_UNIX does not use Sparse annotations, remove it to fix
>> the issue.
>>
>> Eliminate the error without affecting functionality.
> 
> The 5 lines of the 3 sentences above have trailing double spaces.
> You may want to configure your editor to highlight them.
> 
> e.g. for emacs
> 
> (setq-default show-trailing-whitespace t)

Thank you for pointing that out. I will ensure to check for such
issues before submitting future patches.

> 
> 
>>
>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> 
> Otherwise looks good.
> 
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thank you.

Best regards,
Purva Yeshi

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Jakub Kicinski]

On Wed, 12 Feb 2025 19:54:16 +0530 Purva Yeshi wrote:
> > The 5 lines of the 3 sentences above have trailing double spaces.
> > You may want to configure your editor to highlight them.
> > 
> > e.g. for emacs
> > 
> > (setq-default show-trailing-whitespace t)  
> 
> Thank you for pointing that out. I will ensure to check for such
> issues before submitting future patches.

To be clear - please fix this and repost this patch

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Purva Yeshi]

On 13/02/25 00:18, Jakub Kicinski wrote:
> On Wed, 12 Feb 2025 19:54:16 +0530 Purva Yeshi wrote:
>>> The 5 lines of the 3 sentences above have trailing double spaces.
>>> You may want to configure your editor to highlight them.
>>>
>>> e.g. for emacs
>>>
>>> (setq-default show-trailing-whitespace t)
>>
>> Thank you for pointing that out. I will ensure to check for such
>> issues before submitting future patches.
> 
> To be clear - please fix this and repost this patch

Thanks for the clarification. I'll fix the trailing spaces and repost 
the patch shortly.

Best Regards,
Purva Yeshi

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Simon Horman]

+ Iwashima-san, Dan

On Mon, Feb 10, 2025 at 01:20:06PM +0530, Purva Yeshi wrote:
> Fix issue detected by smatch tool:
> An "undefined 'other'" error occur in __releases() annotation.
> 
> Fix an undefined 'other' error in unix_wait_for_peer() caused by  
> __releases(&unix_sk(other)->lock) being placed before 'other' is in  
> scope. Since AF_UNIX does not use Sparse annotations, remove it to fix  
> the issue.  
> 
> Eliminate the error without affecting functionality.  
> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
> V1 - https://lore.kernel.org/lkml/20250209184355.16257-1-purvayeshi550@gmail.com/
> V2 - Remove __releases() annotation as AF_UNIX does not use Sparse annotations.

Hi Iwashima-san, all,

in v1 of this change you commented that:

  Tweaking an annotation with a comment for a static analyzer to fix
  a warning for yet another static analyzer is too much.

  Please remove sparse annotation instead.

  Here's the only place where sparse is used in AF_UNIX code, and we
  don't use sparse even for /proc/net/unix.

And I do understand entirely that we don't want to overly tweak
things to keep static analysis tools happy. But I don't think the
patch description describes the situation completely. So I'd like
to provide a bit more information.

My understanding is that the two static analysis tools under discussion
are Smatch and Sparse, where AFAIK Smatch is a fork of Sparse.

Without this patch, when checking af_unix.c, both Smatch and Sparse report
(only):

 .../af_unix.c:1511:9: error: undefined identifier 'other'
 .../af_unix.c:1511:9: error: undefined identifier 'other'
 .../af_unix.c:1511:9: error: undefined identifier 'other'
 .../af_unix.c:1511:9: error: undefined identifier 'other'

And with either v1 or v2 of this patch applied Smatch reports nothing.
While Sparse reports:

 .../af_unix.c:234:13: warning: context imbalance in 'unix_table_double_lock' - wrong count at exit
 .../af_unix.c:253:28: warning: context imbalance in 'unix_table_double_unlock' - unexpected unlock
 .../af_unix.c:1386:13: warning: context imbalance in 'unix_state_double_lock' - wrong count at exit
 .../af_unix.c:1403:17: warning: context imbalance in 'unix_state_double_unlock' - unexpected unlock
 .../af_unix.c:2089:25: warning: context imbalance in 'unix_dgram_sendmsg' - unexpected unlock
 .../af_unix.c:3335:20: warning: context imbalance in 'unix_get_first' - wrong count at exit
 .../af_unix.c:3366:34: warning: context imbalance in 'unix_get_next' - unexpected unlock
 .../af_unix.c:3396:42: warning: context imbalance in 'unix_seq_stop' - unexpected unlock
 .../af_unix.c:3499:34: warning: context imbalance in 'bpf_iter_unix_hold_batch' - unexpected unlock

TBH, I'm unsure which is worse. Nor how to improve things.

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Dan Carpenter]

I've added the linux-sparse@vger.kernel.org mailing list to the CC.

On Sat, Feb 15, 2025 at 05:24:40PM +0000, Simon Horman wrote:
> My understanding is that the two static analysis tools under discussion
> are Smatch and Sparse, where AFAIK Smatch is a fork of Sparse.
> 
> Without this patch, when checking af_unix.c, both Smatch and Sparse report
> (only):
> 
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
> 

Smatch isn't a fork of Sparse, it uses Sparse as a C front-end.
This warning is really from Sparse, not Smatch.  The warning started
when we changed the definition of unix_sk() in commit b064ba9c3cfa
("af_unix: preserve const qualifier in unix_sk()").

Smatch doesn't actually use these locking annotations at all.  Instead,
Smatch has a giant table with all the locks listed.
https://github.com/error27/smatch/blob/master/smatch_locking.c
Smatch uses the cross function database for this as well if it's
available.

Unfortunately, Smatch does not parse the unix_wait_for_peer() function
correctly.  It sees that something is unlocked but it can't figure out
what.  I believe the problem is that Smatch doesn't parse
container_of_const().  Fixing that has been on my TODO list for a while.
The caller used unix_state_lock() to take the lock and that has a
unix_sk() in it as well.  So smatch doesn't see this lock at all that's
why it doesn't print a warning.

regards,
dan carpenter

> Without this patch, when checking af_unix.c, both Smatch and Sparse report
> (only):
> 
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
>  .../af_unix.c:1511:9: error: undefined identifier 'other'
> 
> And with either v1 or v2 of this patch applied Smatch reports nothing.
> While Sparse reports:
> 
>  .../af_unix.c:234:13: warning: context imbalance in 'unix_table_double_lock' - wrong count at exit
>  .../af_unix.c:253:28: warning: context imbalance in 'unix_table_double_unlock' - unexpected unlock
>  .../af_unix.c:1386:13: warning: context imbalance in 'unix_state_double_lock' - wrong count at exit
>  .../af_unix.c:1403:17: warning: context imbalance in 'unix_state_double_unlock' - unexpected unlock
>  .../af_unix.c:2089:25: warning: context imbalance in 'unix_dgram_sendmsg' - unexpected unlock
>  .../af_unix.c:3335:20: warning: context imbalance in 'unix_get_first' - wrong count at exit
>  .../af_unix.c:3366:34: warning: context imbalance in 'unix_get_next' - unexpected unlock
>  .../af_unix.c:3396:42: warning: context imbalance in 'unix_seq_stop' - unexpected unlock
>  .../af_unix.c:3499:34: warning: context imbalance in 'bpf_iter_unix_hold_batch' - unexpected unlock
> 
> TBH, I'm unsure which is worse. Nor how to improve things.

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Simon Horman]

On Sun, Feb 16, 2025 at 10:33:38PM +0300, Dan Carpenter wrote:
> I've added the linux-sparse@vger.kernel.org mailing list to the CC.
> 
> On Sat, Feb 15, 2025 at 05:24:40PM +0000, Simon Horman wrote:
> > My understanding is that the two static analysis tools under discussion
> > are Smatch and Sparse, where AFAIK Smatch is a fork of Sparse.
> > 
> > Without this patch, when checking af_unix.c, both Smatch and Sparse report
> > (only):
> > 
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> > 
> 
> Smatch isn't a fork of Sparse, it uses Sparse as a C front-end.

Sorry for my mistake there.

> This warning is really from Sparse, not Smatch.  The warning started
> when we changed the definition of unix_sk() in commit b064ba9c3cfa
> ("af_unix: preserve const qualifier in unix_sk()").
> 
> Smatch doesn't actually use these locking annotations at all.  Instead,
> Smatch has a giant table with all the locks listed.
> https://github.com/error27/smatch/blob/master/smatch_locking.c
> Smatch uses the cross function database for this as well if it's
> available.
> 
> Unfortunately, Smatch does not parse the unix_wait_for_peer() function
> correctly.  It sees that something is unlocked but it can't figure out
> what.  I believe the problem is that Smatch doesn't parse
> container_of_const().  Fixing that has been on my TODO list for a while.
> The caller used unix_state_lock() to take the lock and that has a
> unix_sk() in it as well.  So smatch doesn't see this lock at all that's
> why it doesn't print a warning.

So, hypothetically, Smatch could be enhanced and there wouldn't be any
locking warnings with this patch applied?

> 
> regards,
> dan carpenter
> 
> > Without this patch, when checking af_unix.c, both Smatch and Sparse report
> > (only):
> > 
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> >  .../af_unix.c:1511:9: error: undefined identifier 'other'
> > 
> > And with either v1 or v2 of this patch applied Smatch reports nothing.
> > While Sparse reports:
> > 
> >  .../af_unix.c:234:13: warning: context imbalance in 'unix_table_double_lock' - wrong count at exit
> >  .../af_unix.c:253:28: warning: context imbalance in 'unix_table_double_unlock' - unexpected unlock
> >  .../af_unix.c:1386:13: warning: context imbalance in 'unix_state_double_lock' - wrong count at exit
> >  .../af_unix.c:1403:17: warning: context imbalance in 'unix_state_double_unlock' - unexpected unlock
> >  .../af_unix.c:2089:25: warning: context imbalance in 'unix_dgram_sendmsg' - unexpected unlock
> >  .../af_unix.c:3335:20: warning: context imbalance in 'unix_get_first' - wrong count at exit
> >  .../af_unix.c:3366:34: warning: context imbalance in 'unix_get_next' - unexpected unlock
> >  .../af_unix.c:3396:42: warning: context imbalance in 'unix_seq_stop' - unexpected unlock
> >  .../af_unix.c:3499:34: warning: context imbalance in 'bpf_iter_unix_hold_batch' - unexpected unlock
> > 
> > TBH, I'm unsure which is worse. Nor how to improve things.

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Dan Carpenter]

On Mon, Feb 17, 2025 at 11:15:15AM +0000, Simon Horman wrote:
> So, hypothetically, Smatch could be enhanced and there wouldn't be any
> locking warnings with this patch applied?

Heh.  No.  What I meant to say was that none of this has anything to do
with Smatch.  This is all Sparse stuff.  But also I see now that my email
was wrong...

What happened is that we changed unix_sk() and that meant Sparse couldn't
parse the annotations and prints "error: undefined identifier 'other'".
The error disables Sparse checking for the file.

When we fix the error then the checking is enabled again.  The v1 patch
which changes the annotation is better than the v2 patch because then
it's 9 warnings vs 11 warnings.

The warnings are all false positives.  All old warnings are false
positives.  And again, these are all Sparse warnings, not Smatch.  Smatch
doesn't care about annotations.  Smatch has different bugs completely.
;)

regards,
dan carpenter

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Simon Horman]

On Mon, Feb 17, 2025 at 05:14:14PM +0300, Dan Carpenter wrote:
> On Mon, Feb 17, 2025 at 11:15:15AM +0000, Simon Horman wrote:
> > So, hypothetically, Smatch could be enhanced and there wouldn't be any
> > locking warnings with this patch applied?
> 
> Heh.  No.  What I meant to say was that none of this has anything to do
> with Smatch.  This is all Sparse stuff.  But also I see now that my email
> was wrong...
> 
> What happened is that we changed unix_sk() and that meant Sparse couldn't
> parse the annotations and prints "error: undefined identifier 'other'".
> The error disables Sparse checking for the file.
> 
> When we fix the error then the checking is enabled again.  The v1 patch
> which changes the annotation is better than the v2 patch because then
> it's 9 warnings vs 11 warnings.
> 
> The warnings are all false positives.  All old warnings are false
> positives.  And again, these are all Sparse warnings, not Smatch.  Smatch
> doesn't care about annotations.  Smatch has different bugs completely.
> ;)

Thanks for clarifying :)

Based on the above I'd advocate accepting the code changes in v2 [*].
And live with the warnings.

Which I think is to say that Iwashima-san was right all along.

Reviewed-by: Simon Horman <horms@kernel.org>

[*] Purva, please post a v3 that updates the commit message as per
    Jakub's request elsewhere in this thread:
    https://lore.kernel.org/all/20250212104845.2396abcf@kernel.org/

Re: [PATCH net-next v2] af_unix: Fix undefined 'other' error

[Purva Yeshi]

On 18/02/25 18:51, Simon Horman wrote:
> On Mon, Feb 17, 2025 at 05:14:14PM +0300, Dan Carpenter wrote:
>> On Mon, Feb 17, 2025 at 11:15:15AM +0000, Simon Horman wrote:
>>> So, hypothetically, Smatch could be enhanced and there wouldn't be any
>>> locking warnings with this patch applied?
>>
>> Heh.  No.  What I meant to say was that none of this has anything to do
>> with Smatch.  This is all Sparse stuff.  But also I see now that my email
>> was wrong...
>>
>> What happened is that we changed unix_sk() and that meant Sparse couldn't
>> parse the annotations and prints "error: undefined identifier 'other'".
>> The error disables Sparse checking for the file.
>>
>> When we fix the error then the checking is enabled again.  The v1 patch
>> which changes the annotation is better than the v2 patch because then
>> it's 9 warnings vs 11 warnings.
>>
>> The warnings are all false positives.  All old warnings are false
>> positives.  And again, these are all Sparse warnings, not Smatch.  Smatch
>> doesn't care about annotations.  Smatch has different bugs completely.
>> ;)
> 
> Thanks for clarifying :)
> 
> Based on the above I'd advocate accepting the code changes in v2 [*].
> And live with the warnings.
> 
> Which I think is to say that Iwashima-san was right all along.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> [*] Purva, please post a v3 that updates the commit message as per
>      Jakub's request elsewhere in this thread:
>      https://lore.kernel.org/all/20250212104845.2396abcf@kernel.org/
> 

Thanks for the review and clarification! I'll prepare v3 with no 
trailing double spaces and a more detailed description.

Best regards,
Purva