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: <aGVRUCTYNt_aMkQz@mini-arch>
Date: Wed, 2 Jul 2025 08:33:36 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: 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/

On 07/01, Willem de Bruijn wrote:
> 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 :)

Yeah, agreed, that why I'm only targeting the core<>driver api boundary.
Which still might be, arguably, too much :-)

> 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.

Ack, let me double check whether adding more lockdep calls makes sense.
We already have a bunch of them deep down in the call stack,
that might be enough, not sure.

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

Thank you for the review and feedback!