netdev - Re: [PATCH net-next v2 3/8] net: s/dev_get_mac_address/netif_get_mac

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: <686430091cc2_20bfeb294fc@willemb.c.googlers.com.notmuch>
Date: Tue, 01 Jul 2025 14:59:21 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Stanislav Fomichev <stfomichev@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com
Subject: Re: [PATCH net-next v2 3/8] net:
 s/dev_get_mac_address/netif_get_mac_address/

Stanislav Fomichev wrote:
> On 06/30, Willem de Bruijn wrote:
> > Stanislav Fomichev wrote:
> > > Commit cc34acd577f1 ("docs: net: document new locking reality")
> > > introduced netif_ vs dev_ function semantics: the former expects locked
> > > netdev, the latter takes care of the locking. We don't strictly
> > > follow this semantics on either side, but there are more dev_xxx handlers
> > > now that don't fit. Rename them to netif_xxx where appropriate.
> > > 
> > > netif_get_mac_address is used only by tun/tap, so move it into
> > > NETDEV_INTERNAL namespace.
> > > 
> > > Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> > > ---
> > >  drivers/net/tap.c         | 6 ++++--
> > >  drivers/net/tun.c         | 4 +++-
> > >  include/linux/netdevice.h | 2 +-
> > >  net/core/dev.c            | 4 ++--
> > >  net/core/dev_ioctl.c      | 3 ++-
> > >  net/core/net-sysfs.c      | 2 +-
> > >  6 files changed, 13 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > > index bdf0788d8e66..4c85770c809b 100644
> > > --- a/drivers/net/tap.c
> > > +++ b/drivers/net/tap.c
> > > @@ -28,6 +28,8 @@
> > >  
> > >  #include "tun_vnet.h"
> > >  
> > > +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> > > +
> > >  #define TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
> > >  
> > >  static struct proto tap_proto = {
> > > @@ -1000,8 +1002,8 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> > >  			return -ENOLINK;
> > >  		}
> > >  		ret = 0;
> > > -		dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> > > -				    tap->dev->name);
> > > +		netif_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> > > +				      tap->dev->name);
> > >  		if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
> > >  		    copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
> > >  			ret = -EFAULT;
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index f8c5e2fd04df..4509ae68decf 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -85,6 +85,8 @@
> > >  
> > >  #include "tun_vnet.h"
> > >  
> > > +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> > > +
> > 
> > Thanks for giving this a go. Now that you've implemented it, does the
> > risk (of overlooking callers, mainly) indeed seem acceptable?
> > 
> > Documentation/core-api/symbol-namespaces.rst says
> > 
> >   It is advisable to add the MODULE_IMPORT_NS() statement close to other module
> >   metadata definitions like MODULE_AUTHOR() or MODULE_LICENSE().
> > 
> > No need to respin just for this from me. Something to consider,
> > especially if anything else comes up.
> 
> I put it at the top because it was at the top in bnxt. But it is
> at the top in bnxt is because the MODULE_LICENSE is there :-(
> Thanks for pointing it out, I'll definitely address that to be
> consistent.
>
> > Just curious, did you use the modpost and make nsdeps, or was it
> > sufficient to find the callers with tools like cscope and grep?
> 
> Only grep. I'm hoping the build bots will tell me if missed something.

SG.

One tradeoff with this series is that renaming and refactoring always
adds code churn that makes backports (e.g., to stable) more complex.
I trust that you weighted the pros and cons. We just need to be
careful to not encourage renaming series in general. Hence calling
that out right here :)

And, it's not trivial to review that the now netif_.. callees indeed
are holding the netdev locked (or RTNL). Does it make sense to add
lockdep_rtnl_is_held (or equivalent netdev lock) checks as part of
this series or follow-up? And the inverse for the dev_.. variants.

Aside from this high level points, overall series LGTM, thanks.