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

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