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

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

[Breno Leitao]

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

The second 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 third part coverts arp_req_set_public() to the new helper.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changes in v3:
- Fixed the cover letter (Kuniyuki Iwashima)
- Added a third patch converting arp_req_set_public() to the new helper
  (Kuniyuki Iwashima)
- Link to v2:
  https://lore.kernel.org/r/20250210-arm_fix_selftest-v2-0-ba84b5bc58c8@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 (3):
      net: document return value of dev_getbyhwaddr_rcu()
      net: Add dev_getbyhwaddr_rtnl() helper
      arp: switch to dev_getbyhwaddr() in arp_req_set_public()

 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 38 ++++++++++++++++++++++++++++++++++----
 net/ipv4/arp.c            |  2 +-
 3 files changed, 37 insertions(+), 5 deletions(-)
---
base-commit: 4e41231249f4083a095085ff86e317e29313c2c3
change-id: 20250207-arm_fix_selftest-ee29dbc33a06

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

[PATCH net-next v3 1/3] 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'

Link: https://netdev.bots.linux.dev/static/nipa/931564/13964899/kdoc/summary [1]
Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 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 d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..0b3480a125fcaa6f036ddf219c29fa362ea0cb29 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1134,8 +1134,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 v3 1/3] net: document return value of dev_getbyhwaddr_rcu()

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Wed, 12 Feb 2025 09:47:24 -0800
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..0b3480a125fcaa6f036ddf219c29fa362ea0cb29 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1134,8 +1134,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
>   */

I noticed here we still mention RTNL and it should be removed.

Could you update the comment like this to remove RTNL and fix
mis-aligned notes ?

---8<---
diff --git a/net/core/dev.c b/net/core/dev.c
index d5ab9a4b318e..c0d6017a3840 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1123,17 +1123,17 @@ int netdev_get_name(struct net *net, char *name, int ifindex)
 }
 
 /**
- *	dev_getbyhwaddr_rcu - find a device by its hardware address
- *	@net: the applicable net namespace
- *	@type: media type of device
- *	@ha: hardware address
+ * dev_getbyhwaddr_rcu - find a device by its hardware address
+ * @net: the applicable net namespace
+ * @type: media type of device
+ * @ha: hardware address
  *
- *	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 returned device has not had its ref count increased
- *	and the caller must therefore be careful about locking
+ * Search for an interface by MAC address.  The returned device has
+ * not had its ref count increased and the caller must therefore be
+ * careful about locking
  *
+ * Context: The caller must hold RCU.
+ * Return: pointer to the net_device, or NULL if not found
  */
 
 struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type,
---8<---

Thanks!

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

[Kuniyuki Iwashima]

From: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Thu, 13 Feb 2025 16:36:46 +0900
> From: Breno Leitao <leitao@debian.org>
> Date: Wed, 12 Feb 2025 09:47:24 -0800
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..0b3480a125fcaa6f036ddf219c29fa362ea0cb29 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1134,8 +1134,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
> >   */
> 
> I noticed here we still mention RTNL and it should be removed.

I missed this part is removed in patch 2, but the Return: part
is still duplicate.

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

[Breno Leitao]

Hello Kuniyuki,

On Thu, Feb 13, 2025 at 04:47:48PM +0900, Kuniyuki Iwashima wrote:
> From: Kuniyuki Iwashima <kuniyu@amazon.com>
> Date: Thu, 13 Feb 2025 16:36:46 +0900
> > From: Breno Leitao <leitao@debian.org>
> > Date: Wed, 12 Feb 2025 09:47:24 -0800
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..0b3480a125fcaa6f036ddf219c29fa362ea0cb29 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -1134,8 +1134,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
> > >   */
> > 
I am a bit confused about what you are saying.
> > I noticed here we still mention RTNL and it should be removed.

I have no mention RTNL in this patch at all:

	# git log -n1 --oneline HEAD~2
	6d34fd4700231 net: document return value of dev_getbyhwaddr_rcu()
	# git show  HEAD~2  | grep -i rtnl

> I missed this part is removed in patch 2, but the Return: part
> is still duplicate.

This part is also unclear to me. What do you mean the "Return:" part is
still duplicated?

This is how the documentation looks like, after the patch applied:

	/**
	*      dev_getbyhwaddr_rcu - find a device by its hardware address
	*      @net: the applicable net namespace
	*      @type: media type of device
	*      @ha: hardware address
	*
	*      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.
	*      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)
	{
		<snip>
	}

	/**
	*      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)
	{
		<snip>
	}

Where is the Return: part duplicated?

Thanks for the review
--breno

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

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Thu, 13 Feb 2025 02:16:36 -0800
> Hello Kuniyuki,
> 
> On Thu, Feb 13, 2025 at 04:47:48PM +0900, Kuniyuki Iwashima wrote:
> > From: Kuniyuki Iwashima <kuniyu@amazon.com>
> > Date: Thu, 13 Feb 2025 16:36:46 +0900
> > > From: Breno Leitao <leitao@debian.org>
> > > Date: Wed, 12 Feb 2025 09:47:24 -0800
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index d5ab9a4b318ea4926c200ef20dae01eaafa18c6b..0b3480a125fcaa6f036ddf219c29fa362ea0cb29 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -1134,8 +1134,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
> > > >   */
> > > 
> I am a bit confused about what you are saying.
> > > I noticed here we still mention RTNL and it should be removed.
> 
> I have no mention RTNL in this patch at all:

Yes, and patch 2 removes the part from kernel-doc of
dev_getbyhwaddr_rcu().


> 
> 	# git log -n1 --oneline HEAD~2
> 	6d34fd4700231 net: document return value of dev_getbyhwaddr_rcu()
> 	# git show  HEAD~2  | grep -i rtnl
> 
> > I missed this part is removed in patch 2, but the Return: part
> > is still duplicate.
> 
> This part is also unclear to me. What do you mean the "Return:" part is
> still duplicated?
> 
> This is how the documentation looks like, after the patch applied:
> 
> 	/**
> 	*      dev_getbyhwaddr_rcu - find a device by its hardware address
> 	*      @net: the applicable net namespace
> 	*      @type: media type of device
> 	*      @ha: hardware address
> 	*
> 	*      Search for an interface by MAC address. Returns NULL if the device
                                                       ^^^^^^^
Sorry, I meant this part is redundant as we have Return: now.


> 	*      is not found or a pointer to the device.
> 	*      The caller must hold RCU.
> 	*      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)
> 	{
> 		<snip>
> 	}

[PATCH net-next v3 2/3] 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 5429581f22995bff639e6962a317adbd0ce30cff..641091c73710f8c4229e76c66f40ede9c235c221 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3270,6 +3270,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 0b3480a125fcaa6f036ddf219c29fa362ea0cb29..fd15f96cbd8a99da0aa686f22e3e955d179ffc99 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1122,6 +1122,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
@@ -1130,7 +1136,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
  *
@@ -1142,14 +1148,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 v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper

[Kuniyuki Iwashima]

> Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper

s/_rtnl//

looks like Uday's comment was missed due to the lore issue.


From: Breno Leitao <leitao@debian.org>
Date: Wed, 12 Feb 2025 09:47:25 -0800
> +/**
> + *	dev_getbyhwaddr - find a device by its hardware address

While at it, could you replace '\t' after '*' to a single '\s'
for all kernel-doc comment lines below ?


> + *	@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.

Otherwise the text here is mis-aligned.

  $ ./scripts/kernel-doc -man net/core/dev.c | \
    scripts/split-man.pl /tmp/man && \
    man /tmp/man/dev_getbyhwaddr.9

Also, the latter part should be in Context:

Context: rtnl_lock() must be held.

See https://docs.kernel.org/doc-guide/kernel-doc.html

> + *
> + *	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)

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

[Breno Leitao]

Hello Kuniyuki,

On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote:
> > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper
> 
> s/_rtnl//

Ack!

> looks like Uday's comment was missed due to the lore issue.

hmm, I haven't seen Uday's comment. Didn't it reach lore?

> From: Breno Leitao <leitao@debian.org>
> Date: Wed, 12 Feb 2025 09:47:25 -0800
> > +/**
> > + *	dev_getbyhwaddr - find a device by its hardware address
> 
> While at it, could you replace '\t' after '*' to a single '\s'
> for all kernel-doc comment lines below ?
> 
> 
> > + *	@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.
> 
> Otherwise the text here is mis-aligned.

Sorry, what is misaligned specifically? I generated the documentation,
and I can't see it misaligned.

This is what I see when generating the document (full log at
https://pastebin.mozilla.org/YkotEoHh#L250,271)


	dev_getbyhwaddr_rcu(9)                                                         Kernel Hacker's Manual                                                         dev_getbyhwaddr_rcu(9)

	NAME
	dev_getbyhwaddr_rcu - find a device by its hardware address

	SYNOPSIS
	struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha );

	ARGUMENTS
	net         the applicable net namespace

	type        media type of device

	ha          hardware address

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

	dev_getbyhwaddr(9)                                                             Kernel Hacker's Manual                                                             dev_getbyhwaddr(9)

	NAME
	dev_getbyhwaddr - find a device by its hardware address

	SYNOPSIS
	struct net_device * dev_getbyhwaddr (struct net *net , unsigned short type , const char *ha );

	ARGUMENTS
	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


>   $ ./scripts/kernel-doc -man net/core/dev.c | \
>     scripts/split-man.pl /tmp/man && \
>     man /tmp/man/dev_getbyhwaddr.9
> 
> Also, the latter part should be in Context:
> 
> Context: rtnl_lock() must be held.

Sure. Should I do something similar for _rcu function as well?

	Context: caller must hold rcu_read_lock

Thanks for the review
--breno

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

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Thu, 13 Feb 2025 02:29:38 -0800
> Hello Kuniyuki,
> 
> On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote:
> > > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper
> > 
> > s/_rtnl//
> 
> Ack!
> 
> > looks like Uday's comment was missed due to the lore issue.
> 
> hmm, I haven't seen Uday's comment. Didn't it reach lore?

I saw the reply and my another one on lore but somehow they
were removed :)


> 
> > From: Breno Leitao <leitao@debian.org>
> > Date: Wed, 12 Feb 2025 09:47:25 -0800
> > > +/**
> > > + *	dev_getbyhwaddr - find a device by its hardware address
> > 
> > While at it, could you replace '\t' after '*' to a single '\s'
> > for all kernel-doc comment lines below ?
> > 
> > 
> > > + *	@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.
> > 
> > Otherwise the text here is mis-aligned.
> 
> Sorry, what is misaligned specifically? I generated the documentation,
> and I can't see it misaligned.
> 
> This is what I see when generating the document (full log at
> https://pastebin.mozilla.org/YkotEoHh#L250,271)
> 
> 
> 	dev_getbyhwaddr_rcu(9)                                                         Kernel Hacker's Manual                                                         dev_getbyhwaddr_rcu(9)
> 
> 	NAME
> 	dev_getbyhwaddr_rcu - find a device by its hardware address
> 
> 	SYNOPSIS
> 	struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha );
> 
> 	ARGUMENTS
> 	net         the applicable net namespace
> 
> 	type        media type of device
> 
> 	ha          hardware address
> 
> 			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.  The returned  device  has
                        ^^^^^^
This scentence starts from a weird position when we use '\t' after '*'.
You will see it start from the head if '\s' follows '*'.


> 			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
> 
> 	dev_getbyhwaddr(9)                                                             Kernel Hacker's Manual                                                             dev_getbyhwaddr(9)
> 
> 	NAME
> 	dev_getbyhwaddr - find a device by its hardware address
> 
> 	SYNOPSIS
> 	struct net_device * dev_getbyhwaddr (struct net *net , unsigned short type , const char *ha );
> 
> 	ARGUMENTS
> 	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
> 
> 
> >   $ ./scripts/kernel-doc -man net/core/dev.c | \
> >     scripts/split-man.pl /tmp/man && \
> >     man /tmp/man/dev_getbyhwaddr.9
> > 
> > Also, the latter part should be in Context:
> > 
> > Context: rtnl_lock() must be held.
> 
> Sure. Should I do something similar for _rcu function as well?
> 
> 	Context: caller must hold rcu_read_lock

Yes, that would be nice.

https://lore.kernel.org/netdev/20250213073646.14847-1-kuniyu@amazon.com/

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

[Breno Leitao]

On Thu, Feb 13, 2025 at 07:52:17PM +0900, Kuniyuki Iwashima wrote:
> From: Breno Leitao <leitao@debian.org>
> Date: Thu, 13 Feb 2025 02:29:38 -0800
> > Hello Kuniyuki,
> > 
> > On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote:
> > > > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper
> > > 
> > > s/_rtnl//
> > 
> > Ack!
> > 
> > > looks like Uday's comment was missed due to the lore issue.
> > 
> > hmm, I haven't seen Uday's comment. Didn't it reach lore?
> 
> I saw the reply and my another one on lore but somehow they
> were removed :)
> 
> 
> > 
> > > From: Breno Leitao <leitao@debian.org>
> > > Date: Wed, 12 Feb 2025 09:47:25 -0800
> > > > +/**
> > > > + *	dev_getbyhwaddr - find a device by its hardware address
> > > 
> > > While at it, could you replace '\t' after '*' to a single '\s'
> > > for all kernel-doc comment lines below ?
> > > 
> > > 
> > > > + *	@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.
> > > 
> > > Otherwise the text here is mis-aligned.
> > 
> > Sorry, what is misaligned specifically? I generated the documentation,
> > and I can't see it misaligned.
> > 
> > This is what I see when generating the document (full log at
> > https://pastebin.mozilla.org/YkotEoHh#L250,271)
> > 
> > 
> > 	dev_getbyhwaddr_rcu(9)                                                         Kernel Hacker's Manual                                                         dev_getbyhwaddr_rcu(9)
> > 
> > 	NAME
> > 	dev_getbyhwaddr_rcu - find a device by its hardware address
> > 
> > 	SYNOPSIS
> > 	struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha );
> > 
> > 	ARGUMENTS
> > 	net         the applicable net namespace
> > 
> > 	type        media type of device
> > 
> > 	ha          hardware address
> > 
> > 			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.  The returned  device  has
>                         ^^^^^^
> This scentence starts from a weird position when we use '\t' after '*'.
> You will see it start from the head if '\s' follows '*'.
>

Makes sense. All of these functions on that file is misaligned.

Having it aligned will add the section under "DESCRIPTION" manpage
section.

[PATCH net-next v3 3/3] arp: switch to dev_getbyhwaddr() in arp_req_set_public()

[Breno Leitao]

The arp_req_set_public() function is called with the RTNL lock held,
which provides enough synchronization protection. This makes the RCU
variant of dev_getbyhwaddr() unnecessary. Switch to using the simpler
dev_getbyhwaddr() function since we already have the required rtnl
locking.

This change helps maintain consistency in the networking code by using
the appropriate helper function for the existing locking context.

Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/ipv4/arp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index cb9a7ed8abd3ab17403f226ea7e31ea2bae52a9f..1867de1cd156fa91bb3ed4a2c12cafd69d11468a 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1075,7 +1075,7 @@ static int arp_req_set_public(struct net *net, struct arpreq *r,
 	__be32 mask = ((struct sockaddr_in *)&r->arp_netmask)->sin_addr.s_addr;
 
 	if (!dev && (r->arp_flags & ATF_COM)) {
-		dev = dev_getbyhwaddr_rcu(net, r->arp_ha.sa_family,
+		dev = dev_getbyhwaddr(net, r->arp_ha.sa_family,
 				      r->arp_ha.sa_data);
 		if (!dev)
 			return -ENODEV;

-- 
2.43.5

Re: [PATCH net-next v3 3/3] arp: switch to dev_getbyhwaddr() in arp_req_set_public()

[Kuniyuki Iwashima]

From: Breno Leitao <leitao@debian.org>
Date: Wed, 12 Feb 2025 09:47:26 -0800
> The arp_req_set_public() function is called with the RTNL lock held,
> which provides enough synchronization protection. This makes the RCU
> variant of dev_getbyhwaddr() unnecessary. Switch to using the simpler
> dev_getbyhwaddr() function since we already have the required rtnl
> locking.
> 
> This change helps maintain consistency in the networking code by using
> the appropriate helper function for the existing locking context.
> 
> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>

Fixes: 941666c2e3e0 ("net: RCU conversion of dev_getbyhwaddr() and arp_ioctl()")
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

I still think patch 2 & 3 should be posted to net separately.

Documentation/process/maintainer-netdev.rst

---8<---
the ``net`` tree is for fixes to existing code already in the
mainline tree from Linus
---8<---