| Message ID | 20250210075006.9126-1-purvayeshi550@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] af_unix: Fix undefined 'other' error | expand |
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>
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>
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
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
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
+ 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.
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.
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.
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
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/
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
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;
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(-)