| Message ID | 20250210-arm_fix_selftest-v2-2-ba84b5bc58c8@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: core: improvements to device lookup by hardware address. | expand |
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
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)
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
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
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;
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(-)