netdev - Re: for_each_netdev_rcu() protected by RTNL and CONFIG_PROVE_RCU

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: <CANn89iJ0UdSpuA9gMEDeZ1UU+_VwjvD=bdQPeEA0kWfKMBwC8g@mail.gmail.com>
Date: Fri, 7 Feb 2025 11:56:53 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Kuniyuki Iwashima <kuniyu@...zon.com>, andrew+netdev@...n.ch, kernel-team@...a.com, 
	kuba@...nel.org, netdev@...r.kernel.org, ushankar@...estorage.com
Subject: Re: for_each_netdev_rcu() protected by RTNL and CONFIG_PROVE_RCU_LIST

On Fri, Feb 7, 2025 at 11:46 AM Breno Leitao <leitao@...ian.org> wrote:
>
> Hello Kuniyuki,
>
> On Fri, Feb 07, 2025 at 12:38:22PM +0900, Kuniyuki Iwashima wrote:
> > From: Breno Leitao <leitao@...ian.org>
> > Date: Thu, 6 Feb 2025 07:51:55 -0800
>
> > > Are there better approaches to silence these warnings when RTNL is held?
> > > Any suggestions would be appreciated.
> >
> > We can't use lockdep_rtnl_net_is_held() there yet because most users are
> > not converted to per-netns RTNL, so it will complain loudly.
>
> Right, so, I understand the best approach is to leverage
> lockdep_rtnl_is_held() only right now. Something as:
>
>         diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>         index 1dcc76af75203..0deee1313f23a 100644
>         --- a/include/linux/netdevice.h
>         +++ b/include/linux/netdevice.h
>         @@ -3217,7 +3217,8 @@ int call_netdevice_notifiers_info(unsigned long val,
>         #define for_each_netdev_reverse(net, d)        \
>                         list_for_each_entry_reverse(d, &(net)->dev_base_head, dev_list)
>         #define for_each_netdev_rcu(net, d)            \
>         -               list_for_each_entry_rcu(d, &(net)->dev_base_head, dev_list)
>         +               list_for_each_entry_rcu(d, &(net)->dev_base_head, dev_list, \
>         +                                       lockdep_rtnl_is_held())
>         #define for_each_netdev_safe(net, d, n)        \
>                         list_for_each_entry_safe(d, n, &(net)->dev_base_head, dev_list)
>         #define for_each_netdev_continue(net, d)               \
>
> Which brings another problem:
>
> lockdep_rtnl_is_held() is defined in include/linux/rtnetlink.h, so,
> we'll need to include 'linux/rtnetlink.h' in linux/netdevice.h, which
> doesn't seem correct (!?).
>
> Otherwise drivers using for_each_netdev_rcu() will not be able to find
> lockdep_rtnl_is_held().
>
> I suppose we will need to move some of definitions around, but, I am
> confident in which way.

Note that we have different accessors like rtnl_dereference() and
rcu_dereference_rtnl()
It helps to differentiate expectations, and as self describing code.

I would not change  for_each_netdev_rcu(), and instead add a new
dev_getbyhwaddr_rtnl()
function for contexts holding RTNL.

Alternatively, add one rcu_read_lock()/rcu_read_unlock() pair as some
dev_getbyhwaddr_rcu() callers already do.