[PATCH net-next v2 0/2] net: core: improvements to device lookup by hardware address.

[PATCH net-next v2 0/2] net: core: improvements to device lookup by hardware address.

[Breno Leitao]

The first patch adds a new dev_getbyhwaddr() helper function for
finding devices by hardware address when the RTNL lock is held. This
prevents PROVE_LOCKING warnings that occurred when RTNL was held but the
RCU read lock wasn't. The common address comparison logic is extracted
into dev_comp_addr() to avoid code duplication.

The second patch adds missing documentation for the return value of
dev_getbyhwaddr_rcu(), fixing a warning reported by NIPA. The kdoc
comment now properly specifies that the function returns either a
pointer to net_device or NULL when no matching device is found.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changes in v2:
- Fixed the documentation (Jakub)
- Renamed the function from dev_getbyhwaddr_rtnl() to dev_getbyhwaddr()
  (Jakub)
- Exported the function in the header (Jakub)
- Link to v1: https://lore.kernel.org/r/20250207-arm_fix_selftest-v1-1-487518d2fd1c@debian.org

---
Breno Leitao (2):
      net: document return value of dev_getbyhwaddr_rcu()
      net: Add dev_getbyhwaddr_rtnl() helper

 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 38 ++++++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)
---
base-commit: 0d5248724ed8bc68c867c4c65dda625277f68fbc
change-id: 20250207-arm_fix_selftest-ee29dbc33a06

Best regards,
-- 
Breno Leitao <leitao@debian.org>

[PATCH net-next v2 1/2] net: document return value of dev_getbyhwaddr_rcu()

[Breno Leitao]

Add missing return value documentation in the kernel-doc comment for
dev_getbyhwaddr_rcu(), clarifying that it returns either a pointer to
net_device or NULL if no matching device is found.

This fix a warning found in NIPA[1]:

	net/core/dev.c:1141: warning: No description found for return value of 'dev_getbyhwaddr_rcu'

[1] Link: https://netdev.bots.linux.dev/static/nipa/931564/13964899/kdoc/summary
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c41d1e1cbf62e0c5778c472cdb947b6f140f6064..c7e726f81406ece98801441dce3d683c8e0c9d99 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1133,8 +1133,8 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
  *	The returned device has not had its ref count increased
  *	and the caller must therefore be careful about locking
  *
+ *	Return: pointer to the net_device, or NULL if not found
  */
-
 struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
 				       const char *ha)
 {

-- 
2.43.5

Re: [PATCH net-next v2 1/2] net: document return value of dev_getbyhwaddr_rcu()

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Mon, 10 Feb 2025 03:56:13 -0800
> Add missing return value documentation in the kernel-doc comment for
> dev_getbyhwaddr_rcu(), clarifying that it returns either a pointer to
> net_device or NULL if no matching device is found.
> 
> This fix a warning found in NIPA[1]:
> 
> 	net/core/dev.c:1141: warning: No description found for return value of 'dev_getbyhwaddr_rcu'
> 
> [1] Link: https://netdev.bots.linux.dev/static/nipa/931564/13964899/kdoc/summary

nit: We usually use

  Link: https://.... [1]

, a bare space (not %20) indicates the end of URL.


> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

[Breno Leitao]

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

[Kuniyuki Iwashima]

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

[Breno Leitao]

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

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

[Uday Shankar]

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

[Breno Leitao]

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 0/2] net: core: improvements to device lookup by hardware address.

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Mon, 10 Feb 2025 03:56:12 -0800
> The first patch adds a new dev_getbyhwaddr() helper function for

nit: second

> finding devices by hardware address when the RTNL lock is held. This
> prevents PROVE_LOCKING warnings that occurred when RTNL was held but the

Same comment for patch 2, this itself doens't fix the warning.
Also, patch 2 & 3 should be net.git materials ?  Maybe squash
the two and add a Fixes tag then.


> RCU read lock wasn't. The common address comparison logic is extracted
> into dev_comp_addr() to avoid code duplication.
> 
> The second patch adds missing documentation for the return value of

nit: first


> dev_getbyhwaddr_rcu(), fixing a warning reported by NIPA. The kdoc
> comment now properly specifies that the function returns either a
> pointer to net_device or NULL when no matching device is found.

Re: [PATCH net-next v2 0/2] net: core: improvements to device lookup by hardware address.

[Breno Leitao]

On Tue, Feb 11, 2025 at 10:09:27AM +0900, Kuniyuki Iwashima wrote:
> From: Breno Leitao <leitao@debian.org>
> Date: Mon, 10 Feb 2025 03:56:12 -0800
> > The first patch adds a new dev_getbyhwaddr() helper function for
> 
> nit: second
> 
> > finding devices by hardware address when the RTNL lock is held. This
> > prevents PROVE_LOCKING warnings that occurred when RTNL was held but the
> 
> Same comment for patch 2, this itself doens't fix the warning.
> Also, patch 2 & 3 should be net.git materials ?  Maybe squash
> the two and add a Fixes tag then.

I am not sure this should be against net, since the main user of it will
be targeting net-next.

Also, I don't see it as a fix itself, but a new API that users can
leverage once it is landed, thus, focusing on net-next.

Thanks for the review
--breno