netdev - Re: [PATCH net-next 02/11] net: add helpers for lookup and walking netdevs under netdev

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: <CANn89iKo4k7PaUof+qjiUGT+-25WNed-1+UkWadnASBAMcZ2Bw@mail.gmail.com>
Date: Tue, 14 Jan 2025 14:03:35 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com, 
	andrew+netdev@...n.ch, horms@...nel.org, jdamato@...tly.com
Subject: Re: [PATCH net-next 02/11] net: add helpers for lookup and walking
 netdevs under netdev_lock()

On Tue, Jan 14, 2025 at 4:51 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Add helpers for accessing netdevs under netdev_lock().
> There's some careful handling needed to find the device and lock it
> safely, without it getting unregistered, and without taking rtnl_lock
> (the latter being the whole point of the new locking, after all).
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  net/core/dev.h |  16 +++++++
>  net/core/dev.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>
> diff --git a/net/core/dev.h b/net/core/dev.h
> index d8966847794c..25ae732c0775 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -2,6 +2,7 @@
>  #ifndef _NET_CORE_DEV_H
>  #define _NET_CORE_DEV_H
>
> +#include <linux/cleanup.h>
>  #include <linux/types.h>
>  #include <linux/rwsem.h>
>  #include <linux/netdevice.h>
> @@ -23,8 +24,23 @@ struct sd_flow_limit {
>  extern int netdev_flow_limit_table_len;
>
>  struct napi_struct *netdev_napi_by_id(struct net *net, unsigned int napi_id);
> +struct napi_struct *
> +netdev_napi_by_id_lock(struct net *net, unsigned int napi_id);
>  struct net_device *dev_get_by_napi_id(unsigned int napi_id);
>
> +struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex);
> +struct net_device *__netdev_put_lock(struct net_device *dev);
> +struct net_device *
> +netdev_xa_find_lock(struct net *net, struct net_device *dev,
> +                   unsigned long *index);
> +
> +DEFINE_FREE(netdev_unlock, struct net_device *, if (_T) netdev_unlock(_T));
> +
> +#define for_each_netdev_lock_scoped(net, var_name, ifindex)            \
> +       for (struct net_device *var_name __free(netdev_unlock) = NULL;  \
> +            (var_name = netdev_xa_find_lock(net, var_name, &ifindex)); \
> +            ifindex++)
> +
>  #ifdef CONFIG_PROC_FS
>  int __init dev_proc_init(void);
>  #else
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fda4e1039bf0..5c1e71afbe1c 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -783,6 +783,49 @@ struct napi_struct *netdev_napi_by_id(struct net *net, unsigned int napi_id)
>         return napi;
>  }
>
> +/**
> + *     netdev_napi_by_id_lock() - find a device by NAPI ID and lock it
> + *     @net: the applicable net namespace
> + *     @napi_id: ID of a NAPI of a target device
> + *
> + *     Find a NAPI instance with @napi_id. Lock its device.
> + *     The device must be in %NETREG_REGISTERED state for lookup to succeed.
> + *     netdev_unlock() must be called to release it.
> + *
> + *     Return: pointer to NAPI, its device with lock held, NULL if not found.
> + */
> +struct napi_struct *
> +netdev_napi_by_id_lock(struct net *net, unsigned int napi_id)
> +{
> +       struct napi_struct *napi;
> +       struct net_device *dev;
> +
> +       rcu_read_lock();
> +       napi = netdev_napi_by_id(net, napi_id);
> +       if (!napi || napi->dev->reg_state != NETREG_REGISTERED) {

nit: this should be READ_ONCE(napi->dev->reg_state) != NETREG_REGISTERED

> +               rcu_read_unlock();
> +               return NULL;
> +       }
> +
> +       dev = napi->dev;
> +       dev_hold(dev);
> +       rcu_read_unlock();
> +
> +       dev = __netdev_put_lock(dev);
> +       if (!dev)
> +               return NULL;
> +
> +       rcu_read_lock();
> +       napi = netdev_napi_by_id(net, napi_id);
> +       if (napi && napi->dev != dev)
> +               napi = NULL;
> +       rcu_read_unlock();
> +
> +       if (!napi)
> +               netdev_unlock(dev);
> +       return napi;
> +}
> +
>  /**
>   *     __dev_get_by_name       - find a device by its name
>   *     @net: the applicable net namespace
> @@ -971,6 +1014,73 @@ struct net_device *dev_get_by_napi_id(unsigned int napi_id)
>         return napi ? napi->dev : NULL;
>  }
>
> +/* Release the held reference on the net_device, and if the net_device
> + * is still registered try to lock the instance lock. If device is being
> + * unregistered NULL will be returned (but the reference has been released,
> + * either way!)
> + *
> + * This helper is intended for locking net_device after it has been looked up
> + * using a lockless lookup helper. Lock prevents the instance from going away.
> + */
> +struct net_device *__netdev_put_lock(struct net_device *dev)
> +{
> +       netdev_lock(dev);
> +       if (dev->reg_state > NETREG_REGISTERED) {

I presume the netdev lock will be held at some point in
netdev_run_todo and/or unregister_netdevice_many_notify
so no need for a READ_ONCE() here.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>