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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 22 Feb 2022 19:27:00 +0900
From:   Juhee Kang <claudiajkang@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Networking <netdev@...r.kernel.org>, davem@...emloft.net,
        Eric Dumazet <eric.dumazet@...il.com>, ennoerlangen@...il.com,
        george.mccollister@...il.com, Vladimir Oltean <olteanv@...il.com>,
        marco.wenzel@...berle.de
Subject: Re: [PATCH net-next] net: hsr: fix hsr build error when lockdep is
 not enabled

On Tue, Feb 22, 2022 at 1:47 PM Jakub Kicinski <kuba@...nel.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@...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!

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