[PATCH net-next] net: hsr: fix hsr build error when lockdep is not enabled

[PATCH net-next] net: hsr: fix hsr build error when lockdep is not enabled

[Juhee Kang]

In hsr, lockdep_is_held() is needed for rcu_dereference_bh_check().
But if lockdep is not enabled, lockdep_is_held() causes a build error:

    ERROR: modpost: "lockdep_is_held" [net/hsr/hsr.ko] undefined!

Thus, this patch solved by adding lockdep_hsr_is_held(). This helper
function calls the lockdep_is_held() when lockdep is enabled, and returns 1
if not defined.

Fixes: e7f27420681f ("net: hsr: fix suspicious RCU usage warning in hsr_node_get_first()")
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
---
 net/hsr/hsr_framereg.c | 25 +++++++++++++++----------
 net/hsr/hsr_framereg.h |  8 +++++++-
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 62272d76545c..584e21788799 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -20,6 +20,13 @@
 #include "hsr_framereg.h"
 #include "hsr_netlink.h"
 
+#ifdef CONFIG_LOCKDEP
+int lockdep_hsr_is_held(spinlock_t *lock)
+{
+	return lockdep_is_held(lock);
+}
+#endif
+
 u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
 {
 	u32 hash = jhash(addr, ETH_ALEN, hsr->hash_seed);
@@ -27,11 +34,12 @@ u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
 	return reciprocal_scale(hash, hsr->hash_buckets);
 }
 
-struct hsr_node *hsr_node_get_first(struct hlist_head *head, int cond)
+struct hsr_node *hsr_node_get_first(struct hlist_head *head, spinlock_t *lock)
 {
 	struct hlist_node *first;
 
-	first = rcu_dereference_bh_check(hlist_first_rcu(head), cond);
+	first = rcu_dereference_bh_check(hlist_first_rcu(head),
+					 lockdep_hsr_is_held(lock));
 	if (first)
 		return hlist_entry(first, struct hsr_node, mac_list);
 
@@ -59,8 +67,7 @@ bool hsr_addr_is_self(struct hsr_priv *hsr, unsigned char *addr)
 {
 	struct hsr_node *node;
 
-	node = hsr_node_get_first(&hsr->self_node_db,
-				  lockdep_is_held(&hsr->list_lock));
+	node = hsr_node_get_first(&hsr->self_node_db, &hsr->list_lock);
 	if (!node) {
 		WARN_ONCE(1, "HSR: No self node\n");
 		return false;
@@ -107,8 +114,7 @@ int hsr_create_self_node(struct hsr_priv *hsr,
 	ether_addr_copy(node->macaddress_B, addr_b);
 
 	spin_lock_bh(&hsr->list_lock);
-	oldnode = hsr_node_get_first(self_node_db,
-				     lockdep_is_held(&hsr->list_lock));
+	oldnode = hsr_node_get_first(self_node_db, &hsr->list_lock);
 	if (oldnode) {
 		hlist_replace_rcu(&oldnode->mac_list, &node->mac_list);
 		spin_unlock_bh(&hsr->list_lock);
@@ -127,8 +133,7 @@ void hsr_del_self_node(struct hsr_priv *hsr)
 	struct hsr_node *node;
 
 	spin_lock_bh(&hsr->list_lock);
-	node = hsr_node_get_first(self_node_db,
-				  lockdep_is_held(&hsr->list_lock));
+	node = hsr_node_get_first(self_node_db, &hsr->list_lock);
 	if (node) {
 		hlist_del_rcu(&node->mac_list);
 		kfree_rcu(node, rcu_head);
@@ -194,7 +199,7 @@ static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
 
 	spin_lock_bh(&hsr->list_lock);
 	hlist_for_each_entry_rcu(node, node_db, mac_list,
-				 lockdep_is_held(&hsr->list_lock)) {
+				 lockdep_hsr_is_held(&hsr->list_lock)) {
 		if (ether_addr_equal(node->macaddress_A, addr))
 			goto out;
 		if (ether_addr_equal(node->macaddress_B, addr))
@@ -601,7 +606,7 @@ void *hsr_get_next_node(struct hsr_priv *hsr, void *_pos,
 
 	if (!_pos) {
 		node = hsr_node_get_first(&hsr->node_db[hash],
-					  lockdep_is_held(&hsr->list_lock));
+					  &hsr->list_lock);
 		if (node)
 			ether_addr_copy(addr, node->macaddress_A);
 		return node;
diff --git a/net/hsr/hsr_framereg.h b/net/hsr/hsr_framereg.h
index 2efd03fb3465..f3762e9e42b5 100644
--- a/net/hsr/hsr_framereg.h
+++ b/net/hsr/hsr_framereg.h
@@ -28,8 +28,14 @@ struct hsr_frame_info {
 	bool is_from_san;
 };
 
+#ifdef CONFIG_LOCKDEP
+int lockdep_hsr_is_held(spinlock_t *lock);
+#else
+#define lockdep_hsr_is_held(lock)	1
+#endif
+
 u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr);
-struct hsr_node *hsr_node_get_first(struct hlist_head *head, int cond);
+struct hsr_node *hsr_node_get_first(struct hlist_head *head, spinlock_t *lock);
 void hsr_del_self_node(struct hsr_priv *hsr);
 void hsr_del_nodes(struct hlist_head *node_db);
 struct hsr_node *hsr_get_node(struct hsr_port *port, struct hlist_head *node_db,
-- 
2.25.1

Re: [PATCH net-next] net: hsr: fix hsr build error when lockdep is not enabled

[Jakub Kicinski]

On Sun, 20 Feb 2022 15:32:50 +0000 Juhee Kang wrote:
> In hsr, lockdep_is_held() is needed for rcu_dereference_bh_check().
> But if lockdep is not enabled, lockdep_is_held() causes a build error:
> 
>     ERROR: modpost: "lockdep_is_held" [net/hsr/hsr.ko] undefined!
> 
> Thus, this patch solved by adding lockdep_hsr_is_held(). This helper
> function calls the lockdep_is_held() when lockdep is enabled, and returns 1
> if not defined.
> 
> Fixes: e7f27420681f ("net: hsr: fix suspicious RCU usage warning in hsr_node_get_first()")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
> ---
>  net/hsr/hsr_framereg.c | 25 +++++++++++++++----------
>  net/hsr/hsr_framereg.h |  8 +++++++-
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index 62272d76545c..584e21788799 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -20,6 +20,13 @@
>  #include "hsr_framereg.h"
>  #include "hsr_netlink.h"
>  
> +#ifdef CONFIG_LOCKDEP
> +int lockdep_hsr_is_held(spinlock_t *lock)
> +{
> +	return lockdep_is_held(lock);
> +}
> +#endif

Let me apply the patch, so that people don't hit this problem,
but please investigate if this helper is needed..

>  u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
>  {
>  	u32 hash = jhash(addr, ETH_ALEN, hsr->hash_seed);
> @@ -27,11 +34,12 @@ u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
>  	return reciprocal_scale(hash, hsr->hash_buckets);
>  }
>  
> -struct hsr_node *hsr_node_get_first(struct hlist_head *head, int cond)
> +struct hsr_node *hsr_node_get_first(struct hlist_head *head, spinlock_t *lock)
>  {
>  	struct hlist_node *first;
>  
> -	first = rcu_dereference_bh_check(hlist_first_rcu(head), cond);
> +	first = rcu_dereference_bh_check(hlist_first_rcu(head),
> +					 lockdep_hsr_is_held(lock));

.. since you moved the lockdep check inside rcu_dereference() I think
the build problem should go away. rcu_deref..() will only execute the
last argument if PROVE_LOCKING is set, so it should be safe to pass
lockdep_is_held(lock) in directly there.

Please double check and send another follow up if I'm correct, thanks!

Re: [PATCH net-next] net: hsr: fix hsr build error when lockdep is not enabled

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 20 Feb 2022 15:32:50 +0000 you wrote:
> In hsr, lockdep_is_held() is needed for rcu_dereference_bh_check().
> But if lockdep is not enabled, lockdep_is_held() causes a build error:
> 
>     ERROR: modpost: "lockdep_is_held" [net/hsr/hsr.ko] undefined!
> 
> Thus, this patch solved by adding lockdep_hsr_is_held(). This helper
> function calls the lockdep_is_held() when lockdep is enabled, and returns 1
> if not defined.
> 
> [...]

Here is the summary with links:
  - [net-next] net: hsr: fix hsr build error when lockdep is not enabled
    https://git.kernel.org/netdev/net-next/c/a0b92e0514bb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: hsr: fix hsr build error when lockdep is not enabled

[Juhee Kang]

On Tue, Feb 22, 2022 at 1:47 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 20 Feb 2022 15:32:50 +0000 Juhee Kang wrote:
> > In hsr, lockdep_is_held() is needed for rcu_dereference_bh_check().
> > But if lockdep is not enabled, lockdep_is_held() causes a build error:
> >
> >     ERROR: modpost: "lockdep_is_held" [net/hsr/hsr.ko] undefined!
> >
> > Thus, this patch solved by adding lockdep_hsr_is_held(). This helper
> > function calls the lockdep_is_held() when lockdep is enabled, and returns 1
> > if not defined.
> >
> > Fixes: e7f27420681f ("net: hsr: fix suspicious RCU usage warning in hsr_node_get_first()")
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
> > ---
> >  net/hsr/hsr_framereg.c | 25 +++++++++++++++----------
> >  net/hsr/hsr_framereg.h |  8 +++++++-
> >  2 files changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> > index 62272d76545c..584e21788799 100644
> > --- a/net/hsr/hsr_framereg.c
> > +++ b/net/hsr/hsr_framereg.c
> > @@ -20,6 +20,13 @@
> >  #include "hsr_framereg.h"
> >  #include "hsr_netlink.h"
> >
> > +#ifdef CONFIG_LOCKDEP
> > +int lockdep_hsr_is_held(spinlock_t *lock)
> > +{
> > +     return lockdep_is_held(lock);
> > +}
> > +#endif
>
> Let me apply the patch, so that people don't hit this problem,
> but please investigate if this helper is needed..
>
> >  u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
> >  {
> >       u32 hash = jhash(addr, ETH_ALEN, hsr->hash_seed);
> > @@ -27,11 +34,12 @@ u32 hsr_mac_hash(struct hsr_priv *hsr, const unsigned char *addr)
> >       return reciprocal_scale(hash, hsr->hash_buckets);
> >  }
> >
> > -struct hsr_node *hsr_node_get_first(struct hlist_head *head, int cond)
> > +struct hsr_node *hsr_node_get_first(struct hlist_head *head, spinlock_t *lock)
> >  {
> >       struct hlist_node *first;
> >
> > -     first = rcu_dereference_bh_check(hlist_first_rcu(head), cond);
> > +     first = rcu_dereference_bh_check(hlist_first_rcu(head),
> > +                                      lockdep_hsr_is_held(lock));
>
> .. since you moved the lockdep check inside rcu_dereference() I think
> the build problem should go away. rcu_deref..() will only execute the
> last argument if PROVE_LOCKING is set, so it should be safe to pass
> lockdep_is_held(lock) in directly there.
>
> Please double check and send another follow up if I'm correct, thanks!

Hi Jakub,
Sorry for the noise. I keep that in mind.

And thank you so much for the review.
I will apply for your review and send a new patch after some tests.

Thanks a lot

-- 

Best regards,
Juhee Kang