1. Patchew
  2. linux
  3. [PATCH net-next v2 0/2] net: core: improvements to device lookup by hardware address.
  4. View patch

[PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper

Breno Leitao posted 2 patches 10 months, 1 week ago
There is a newer version of this series
[PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper
Posted by Breno Leitao 10 months, 1 week ago 
Add dedicated helper for finding devices by hardware address when
holding rtnl_lock, similar to existing dev_getbyhwaddr_rcu(). This prevents
PROVE_LOCKING warnings when rtnl_lock is held but RCU read lock is not.

Extract common address comparison logic into dev_comp_addr().

The context about this change could be found in the following
discussion:

Link: https://lore.kernel.org/all/20250206-scarlet-ermine-of-improvement-1fcac5@leitao/

Cc: kuniyu@amazon.com
Cc: ushankar@purestorage.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0deee1313f23a625242678c8e571533e69a05263..6f0f5d327b41bfd5e0ccf9a3e63d6082bdf45d14 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3271,6 +3271,8 @@ static inline struct net_device *first_net_device_rcu(struct net *net)
 }
 
 int netdev_boot_setup_check(struct net_device *dev);
+struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
+				   const char *hwaddr);
 struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
 				       const char *hwaddr);
 struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type);
diff --git a/net/core/dev.c b/net/core/dev.c
index c7e726f81406ece98801441dce3d683c8e0c9d99..2a0fbb319b2ad1b2aae908bc87ef19504cc42909 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1121,6 +1121,12 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
 	return ret;
 }
 
+static bool dev_comp_addr(struct net_device *dev, unsigned short type,
+			  const char *ha)
+{
+	return dev->type == type && !memcmp(dev->dev_addr, ha, dev->addr_len);
+}
+
 /**
  *	dev_getbyhwaddr_rcu - find a device by its hardware address
  *	@net: the applicable net namespace
@@ -1129,7 +1135,7 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
  *
  *	Search for an interface by MAC address. Returns NULL if the device
  *	is not found or a pointer to the device.
- *	The caller must hold RCU or RTNL.
+ *	The caller must hold RCU.
  *	The returned device has not had its ref count increased
  *	and the caller must therefore be careful about locking
  *
@@ -1141,14 +1147,38 @@ struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
 	struct net_device *dev;
 
 	for_each_netdev_rcu(net, dev)
-		if (dev->type == type &&
-		    !memcmp(dev->dev_addr, ha, dev->addr_len))
+		if (dev_comp_addr(dev, type, ha))
 			return dev;
 
 	return NULL;
 }
 EXPORT_SYMBOL(dev_getbyhwaddr_rcu);
 
+/**
+ *	dev_getbyhwaddr - find a device by its hardware address
+ *	@net: the applicable net namespace
+ *	@type: media type of device
+ *	@ha: hardware address
+ *
+ *	Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold
+ *	rtnl_lock.
+ *
+ *	Return: pointer to the net_device, or NULL if not found
+ */
+struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
+				   const char *ha)
+{
+	struct net_device *dev;
+
+	ASSERT_RTNL();
+	for_each_netdev(net, dev)
+		if (dev_comp_addr(dev, type, ha))
+			return dev;
+
+	return NULL;
+}
+EXPORT_SYMBOL(dev_getbyhwaddr);
+
 struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
 {
 	struct net_device *dev, *ret = NULL;

-- 
2.43.5
Re: [PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper
Posted by Uday Shankar 10 months, 1 week ago 
On Mon, Feb 10, 2025 at 03:56:14AM -0800, Breno Leitao wrote:
> +/**
> + *	dev_getbyhwaddr - find a device by its hardware address
> + *	@net: the applicable net namespace
> + *	@type: media type of device
> + *	@ha: hardware address
> + *
> + *	Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold
> + *	rtnl_lock.
> + *
> + *	Return: pointer to the net_device, or NULL if not found
> + */
> +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
> +				   const char *ha)
> +{
> +	struct net_device *dev;
> +
> +	ASSERT_RTNL();
> +	for_each_netdev(net, dev)
> +		if (dev_comp_addr(dev, type, ha))
> +			return dev;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(dev_getbyhwaddr);

Commit title should change to reflect the new function name in v2.

Separately - how should I combine this with
https://lore.kernel.org/netdev/20250205-netconsole-v3-0-132a31f17199@purestorage.com/?
I see three options:
- combine the two series into one
- wait for your series to land before mine
- figure out how to use take and use RCU correctly to avoid the warning,
  then revert those changes and use your new helper in your series
  (would want to avoid this, as it's more work for everyone)
Re: [PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper
Posted by Breno Leitao 10 months, 1 week ago 
Hello Uday,

On Tue, Feb 11, 2025 at 01:10:01AM -0700, Uday Shankar wrote:
> On Mon, Feb 10, 2025 at 03:56:14AM -0800, Breno Leitao wrote:
> > +/**
> > + *	dev_getbyhwaddr - find a device by its hardware address
> > + *	@net: the applicable net namespace
> > + *	@type: media type of device
> > + *	@ha: hardware address
> > + *
> > + *	Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold
> > + *	rtnl_lock.
> > + *
> > + *	Return: pointer to the net_device, or NULL if not found
> > + */
> > +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
> > +				   const char *ha)
> > +{
> > +	struct net_device *dev;
> > +
> > +	ASSERT_RTNL();
> > +	for_each_netdev(net, dev)
> > +		if (dev_comp_addr(dev, type, ha))
> > +			return dev;
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL(dev_getbyhwaddr);
> 
> Commit title should change to reflect the new function name in v2.
> 
> Separately - how should I combine this with
> https://lore.kernel.org/netdev/20250205-netconsole-v3-0-132a31f17199@purestorage.com/?
> I see three options:
> - combine the two series into one

I would suggest you combine the two series into one.

I will send a v3 today adjusting the comments, and you can integrated
them into your patchset.

Thanks
--breno
Re: [PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper
Posted by Kuniyuki Iwashima 10 months, 1 week ago 
From: Breno Leitao <leitao@debian.org>
Date: Mon, 10 Feb 2025 03:56:14 -0800
> Add dedicated helper for finding devices by hardware address when
> holding rtnl_lock, similar to existing dev_getbyhwaddr_rcu(). This prevents
> PROVE_LOCKING warnings when rtnl_lock is held but RCU read lock is not.

No one uses dev_getbyhwaddr() yet, so this patch itself doens't fix
the warninig.

You are missing patch 3 to convert arp_req_set_public().  Other call
sites are under RCU.


> 
> Extract common address comparison logic into dev_comp_addr().
> 
> The context about this change could be found in the following
> discussion:
> 
> Link: https://lore.kernel.org/all/20250206-scarlet-ermine-of-improvement-1fcac5@leitao/
> 
> Cc: kuniyu@amazon.com
> Cc: ushankar@purestorage.com
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  include/linux/netdevice.h |  2 ++
>  net/core/dev.c            | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0deee1313f23a625242678c8e571533e69a05263..6f0f5d327b41bfd5e0ccf9a3e63d6082bdf45d14 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3271,6 +3271,8 @@ static inline struct net_device *first_net_device_rcu(struct net *net)
>  }
>  
>  int netdev_boot_setup_check(struct net_device *dev);
> +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
> +				   const char *hwaddr);
>  struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
>  				       const char *hwaddr);
>  struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c7e726f81406ece98801441dce3d683c8e0c9d99..2a0fbb319b2ad1b2aae908bc87ef19504cc42909 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1121,6 +1121,12 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
>  	return ret;
>  }
>  
> +static bool dev_comp_addr(struct net_device *dev, unsigned short type,
> +			  const char *ha)
> +{
> +	return dev->type == type && !memcmp(dev->dev_addr, ha, dev->addr_len);
> +}
> +
>  /**
>   *	dev_getbyhwaddr_rcu - find a device by its hardware address
>   *	@net: the applicable net namespace
> @@ -1129,7 +1135,7 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
>   *
>   *	Search for an interface by MAC address. Returns NULL if the device
>   *	is not found or a pointer to the device.
> - *	The caller must hold RCU or RTNL.
> + *	The caller must hold RCU.
>   *	The returned device has not had its ref count increased
>   *	and the caller must therefore be careful about locking
>   *
> @@ -1141,14 +1147,38 @@ struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
>  	struct net_device *dev;
>  
>  	for_each_netdev_rcu(net, dev)
> -		if (dev->type == type &&
> -		    !memcmp(dev->dev_addr, ha, dev->addr_len))
> +		if (dev_comp_addr(dev, type, ha))
>  			return dev;
>  
>  	return NULL;
>  }
>  EXPORT_SYMBOL(dev_getbyhwaddr_rcu);
>  
> +/**
> + *	dev_getbyhwaddr - find a device by its hardware address
> + *	@net: the applicable net namespace
> + *	@type: media type of device
> + *	@ha: hardware address
> + *
> + *	Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold
> + *	rtnl_lock.
> + *
> + *	Return: pointer to the net_device, or NULL if not found
> + */
> +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type,
> +				   const char *ha)
> +{
> +	struct net_device *dev;
> +
> +	ASSERT_RTNL();
> +	for_each_netdev(net, dev)
> +		if (dev_comp_addr(dev, type, ha))
> +			return dev;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(dev_getbyhwaddr);
> +
>  struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
>  {
>  	struct net_device *dev, *ret = NULL;
> 
> -- 
> 2.43.5
Re: [PATCH net-next v2 2/2] net: Add dev_getbyhwaddr_rtnl() helper
Posted by Breno Leitao 10 months, 1 week ago 
On Tue, Feb 11, 2025 at 10:03:00AM +0900, Kuniyuki Iwashima wrote:
> From: Breno Leitao <leitao@debian.org>
> Date: Mon, 10 Feb 2025 03:56:14 -0800
> > Add dedicated helper for finding devices by hardware address when
> > holding rtnl_lock, similar to existing dev_getbyhwaddr_rcu(). This prevents
> > PROVE_LOCKING warnings when rtnl_lock is held but RCU read lock is not.
> 
> No one uses dev_getbyhwaddr() yet, so this patch itself doens't fix
> the warninig.
> 
> You are missing patch 3 to convert arp_req_set_public().  Other call
> sites are under RCU.

That will come later. For now, the goal is to solve the current
netconsole patch by Uday.

So, my suggestion is that Uday's patchset merges this fix, and fix their
own curent problem. Later we can convert dev_getbyhwaddr_rcu() users
back to dev_getbyhwaddr()

how does it sound?

Thanks
--breno

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.