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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220221204731.1229f987@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Mon, 21 Feb 2022 20:47:31 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Juhee Kang <claudiajkang@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        eric.dumazet@...il.com, ennoerlangen@...il.com,
        george.mccollister@...il.com, olteanv@...il.com,
        marco.wenzel@...berle.de
Subject: Re: [PATCH net-next] net: hsr: fix hsr build error when lockdep is
 not enabled

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@...il.com>
> Signed-off-by: Juhee Kang <claudiajkang@...il.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!