[PATCH net] net: add inline annotation to fix the build warning

[PATCH net] net: add inline annotation to fix the build warning

[Moon Yeounsu]

This patch fixes two sparse warnings (`make C=1`):
net/ipv6/icmp.c:103:20: warning: context imbalance in 'icmpv6_xmit_lock' - wrong count at exit
net/ipv6/icmp.c:119:13: warning: context imbalance in 'icmpv6_xmit_unlock' - unexpected unlock

Since `icmp6_xmit_lock()` and `icmp6_xmit_unlock()` are designed as they
are named, entering/returning the function without lock/unlock doesn't
matter.

Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
---
 net/ipv6/icmp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 071b0bc1179d..d8cc3d63c942 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -101,6 +101,7 @@ static const struct inet6_protocol icmpv6_protocol = {
 
 /* Called with BH disabled */
 static struct sock *icmpv6_xmit_lock(struct net *net)
+	__acquires(&sk->sk_lock.slock)
 {
 	struct sock *sk;
 
@@ -117,6 +118,7 @@ static struct sock *icmpv6_xmit_lock(struct net *net)
 }
 
 static void icmpv6_xmit_unlock(struct sock *sk)
+	__releases(&sk->sk_lock.slock)
 {
 	sock_net_set(sk, &init_net);
 	spin_unlock(&sk->sk_lock.slock);
-- 
2.46.1

Re: [PATCH net] net: add inline annotation to fix the build warning

[Simon Horman]

On Thu, Sep 19, 2024 at 11:21:49PM +0900, Moon Yeounsu wrote:
> This patch fixes two sparse warnings (`make C=1`):
> net/ipv6/icmp.c:103:20: warning: context imbalance in 'icmpv6_xmit_lock' - wrong count at exit
> net/ipv6/icmp.c:119:13: warning: context imbalance in 'icmpv6_xmit_unlock' - unexpected unlock
> 
> Since `icmp6_xmit_lock()` and `icmp6_xmit_unlock()` are designed as they
> are named, entering/returning the function without lock/unlock doesn't
> matter.
> 
> Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>

Hi Moon,

Without this patch applied I see the warnings cited above.

However, with this patch applied, I see the following.
So I think this needs more work.

net/ipv6/icmp.c: note: in included file (through include/linux/sched.h, include/linux/percpu.h, arch/x86/include/asm/msr.h, arch/x86/include/asm/tsc.h, arch/x86/include/asm/timex.h, include/linux/timex.h, ...):
./include/linux/spinlock.h:361:16: warning: context imbalance in 'icmpv6_xmit_lock' - different lock contexts for basic block
net/ipv6/icmp.c: note: in included file (through include/linux/spinlock.h, include/linux/sched.h, include/linux/percpu.h, arch/x86/include/asm/msr.h, arch/x86/include/asm/tsc.h, arch/x86/include/asm/timex.h, ...):
./include/linux/bottom_half.h:33:30: warning: context imbalance in 'icmp6_send' - different lock contexts for basic block
./include/linux/bottom_half.h:33:30: warning: context imbalance in 'icmpv6_echo_reply' - different lock contexts for basic block

Also, It is my feeling that addressing warnings of this nature
is not a fix for net, but rather but rather an enhancement for net-next.

net-next is currently closed for the v6.12 merge windows, so non-RFC,
patches should not be posted for net-next until it re-opens once v6.12-rc1
has been released, most likely during the week of 30th September.

-- 
pw-bot: changes-requested

Re: [PATCH net] net: add inline annotation to fix the build warning

[Moon Yeounsu]

On Thu, Sep 19, 2024 at 11:56 PM Simon Horman <horms@kernel.org> wrote:

> Hi Moon,
>
> Without this patch applied I see the warnings cited above.
>
> However, with this patch applied, I see the following.
> So I think this needs more work.
Okay, I'll fix and update. (Of course, patch after release `-rc1`)

> net-next is currently closed for the v6.12 merge windows, so non-RFC,
> patches should not be posted for net-next until it re-opens once v6.12-rc1
> has been released, most likely during the week of 30th September.
Thank you for letting me know : )
During this time, I'll read patches, docs, and code to increase my sense.

I appreciate you reviewing my patch!

[PATCH net] net: add inline annotation to fix the build warning

[Moon Yeounsu]

This patch fixes two `sparse` warnings:
net/core/neighbour.c:453:9: warning: context imbalance in '__neigh_ifdown' - wrong count at exit
net/core/neighbour.c:871:9: warning: context imbalance in 'pneigh_ifdown_and_unlock' - unexpected unlock

You can check it by running:
`make C=1 net/core/neighbour.o`

Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
---
 net/core/neighbour.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 77b819cd995b..6b5ec9a44556 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -441,6 +441,7 @@ EXPORT_SYMBOL(neigh_changeaddr);
 
 static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
 			  bool skip_perm)
+	__acquires(&tbl->lock)
 {
 	write_lock_bh(&tbl->lock);
 	neigh_flush_dev(tbl, dev, skip_perm);
@@ -453,6 +454,7 @@ static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
 }
 
 int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev)
+	__acquires(&tbl->lock)
 {
 	__neigh_ifdown(tbl, dev, true);
 	return 0;
@@ -460,6 +462,7 @@ int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev)
 EXPORT_SYMBOL(neigh_carrier_down);
 
 int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
+	__acquires(&tbl->lock)
 {
 	__neigh_ifdown(tbl, dev, false);
 	return 0;
@@ -848,6 +851,7 @@ int pneigh_delete(struct neigh_table *tbl, struct net *net, const void *pkey,
 
 static int pneigh_ifdown_and_unlock(struct neigh_table *tbl,
 				    struct net_device *dev)
+	__releases(&tbl->lock)
 {
 	struct pneigh_entry *n, **np, *freelist = NULL;
 	u32 h;
-- 
2.46.1

Re: [PATCH net] net: add inline annotation to fix the build warning

[Eric Dumazet]

On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote:
>
> Moon is stupid. He doesn't understand what's going on. It makes me upset.
>
> https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/
>
> Simon did the best effort for him, but he didn't remember that.
>
> Please don't reply to this careless patch.
>
> Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart.
>
> Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`.
>
> I'm very very disappointed to myself :(

LOCKDEP is more powerful than sparse, I would not bother with this at all.

Re: [PATCH net] net: add inline annotation to fix the build warning

[Moon Yeounsu]

On Wed, Oct 2, 2024 at 11:41 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote:
> >
> > Moon is stupid. He doesn't understand what's going on. It makes me upset.
> >
> > https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/
> >
> > Simon did the best effort for him, but he didn't remember that.
> >
> > Please don't reply to this careless patch.
> >
> > Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart.
> >
> > Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`.
> >
> > I'm very very disappointed to myself :(
>
> LOCKDEP is more powerful than sparse, I would not bother with this at all.

Totally agree with that. `Sparse` has a lot of problems derived from its nature.
And It is too annoying to silence the warning message. I know that
this patch just fixes for a fix. (What a trivial?)
But, even though `LOCKDEP` is more powerful than `Sparse`, that can't
be the reason to ignore the warning message.

It is only my opinion and this topic may be outside of the net
subsystem. Please don't be offended by my words and ignorance. I don't
want to make a problem, rather want to fix a problem.
If there's no reason to use `Sparse`, then, how about just removing it
from the kernel? If It can't, we have to make Sparse more useful at
least make to have to care about this warning message.

> LOCKDEP is more powerful than sparse, I would not bother with this at all.

Leastways, This sentence is irrational in my view. Let me know, world!

Re: [PATCH net] net: add inline annotation to fix the build warning

[Eric Dumazet]

On Thu, Oct 3, 2024 at 3:57 PM Moon Yeounsu <yyyynoom@gmail.com> wrote:
>
> On Wed, Oct 2, 2024 at 11:41 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote:
> > >
> > > Moon is stupid. He doesn't understand what's going on. It makes me upset.
> > >
> > > https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/
> > >
> > > Simon did the best effort for him, but he didn't remember that.
> > >
> > > Please don't reply to this careless patch.
> > >
> > > Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart.
> > >
> > > Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`.
> > >
> > > I'm very very disappointed to myself :(
> >
> > LOCKDEP is more powerful than sparse, I would not bother with this at all.
>
> Totally agree with that. `Sparse` has a lot of problems derived from its nature.
> And It is too annoying to silence the warning message. I know that
> this patch just fixes for a fix. (What a trivial?)
> But, even though `LOCKDEP` is more powerful than `Sparse`, that can't
> be the reason to ignore the warning message.
>
> It is only my opinion and this topic may be outside of the net
> subsystem. Please don't be offended by my words and ignorance. I don't
> want to make a problem, rather want to fix a problem.
> If there's no reason to use `Sparse`, then, how about just removing it
> from the kernel? If It can't, we have to make Sparse more useful at
> least make to have to care about this warning message.

sparse is not in the kernel. Feel free to remove it from your hosts.

Anyway, the __acquires(XXX) annotations means nothing, XXX is
completely ignored.

$ diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 09e31757e96c7472af2a9dfff7a731d4d076aa11..50fc48c6d0c99d91f5a8eb15c4e3dd0304a83e0b
100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2888,7 +2888,7 @@ static struct key_vector
*fib_route_get_idx(struct fib_route_iter *iter,
 }

 static void *fib_route_seq_start(struct seq_file *seq, loff_t *pos)
-       __acquires(RCU)
+       __acquires(some_random_stuff)
 {
        struct fib_route_iter *iter = seq->private;
        struct fib_table *tb;


$ make C=1 net/ipv4/fib_trie.o
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  DESCEND bpf/resolve_btfids
  INSTALL libsubcmd_headers
  CC      net/ipv4/fib_trie.o
  CHECK   net/ipv4/fib_trie.c

No error at all.

It also does not know about conditional locking, it is quite useless.

Re: [PATCH net] net: add inline annotation to fix the build warning

[Moon Yeounsu]

> sparse is not in the kernel. Feel free to remove it from your hosts.
Oh... I see. Yes, you are right. Sparse is just a program like other
tools like gcc.

>
> $ diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 09e31757e96c7472af2a9dfff7a731d4d076aa11..50fc48c6d0c99d91f5a8eb15c4e3dd0304a83e0b
> 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2888,7 +2888,7 @@ static struct key_vector
> *fib_route_get_idx(struct fib_route_iter *iter,
>  }
>
>  static void *fib_route_seq_start(struct seq_file *seq, loff_t *pos)
> -       __acquires(RCU)
> +       __acquires(some_random_stuff)
>  {
>         struct fib_route_iter *iter = seq->private;
>         struct fib_table *tb;
>
>
> $ make C=1 net/ipv4/fib_trie.o
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   DESCEND bpf/resolve_btfids
>   INSTALL libsubcmd_headers
>   CC      net/ipv4/fib_trie.o
>   CHECK   net/ipv4/fib_trie.c
>
> No error at all.
> It also does not know about conditional locking, it is quite useless.
Yes, exactly. And It makes me crazy.
`net/ipv6/icmp.c` was written to use the conditional lock as you mentioned.
This is not a problem and can easily be verified intuitively, but
Sparse can't sense it.
To refactor the code to silent `Sparse` is putting the cart before the
horse. NON-SENSE.

So... What do you think about who wants to send the patch to silence
the Sparse's warning message, nevertheless?
I know him who was just about to write the next patch by correcting
mistakes (Seems like he wrote the subject prefix to `net`, not a
`net-next`, what a foolish one).
Is he wasting his life and taking other people's invaluable time? What
do you think about it?

Re: [PATCH net] net: add inline annotation to fix the build warning

[Edward Cree]

On 03/10/2024 16:33, Moon Yeounsu wrote:
> On 03/10/2024 15:19, Eric Dumazet wrote:
>> It also does not know about conditional locking, it is quite useless.
> So... What do you think about who wants to send the patch to silence
> the Sparse's warning message, nevertheless?

Fwiw, my experience is that if I can't explain the locking to sparse
 that's usually a sign that the code is too complex and needs to be
 rewritten.
In general I'm in favour of patches to fix sparse warnings.  In this
 case it looks like what's needed is __cond_acquires, but the patch
 to implement that in sparse[1] doesn't seem to have gotten anywhere
 near Luc's tree.  (Yet it's present and occasionally used in the
 kernel...)  CCing the sparse ML to find out why.

-ed

[1]: https://lore.kernel.org/all/CAHk-=wjZfO9hGqJ2_hGQG3U_XzSh9_XaXze=HgPdvJbgrvASfA@mail.gmail.com/#t

Re: [PATCH net] net: add inline annotation to fix the build warning

[Stephen Hemminger]

On Thu, 3 Oct 2024 17:11:26 +0100
Edward Cree <ecree.xilinx@gmail.com> wrote:

> On 03/10/2024 16:33, Moon Yeounsu wrote:
> > On 03/10/2024 15:19, Eric Dumazet wrote:  
> >> It also does not know about conditional locking, it is quite useless.  
> > So... What do you think about who wants to send the patch to silence
> > the Sparse's warning message, nevertheless?  

In my experience, conditional locking is often a cause of bugs.