netdev - Re: [PATCH net-next 08/11] net: protect threaded status of NAPI with 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: <CANn89iLrzg0hGgNS0PRxaJKvoi4_-siv439x6erpDzb4az3X9Q@mail.gmail.com>
Date: Tue, 14 Jan 2025 14:19:38 +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, 
	leitao@...ian.org
Subject: Re: [PATCH net-next 08/11] net: protect threaded status of NAPI with netdev_lock()

On Tue, Jan 14, 2025 at 4:51 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Now that NAPI instances can't come and go without holding
> netdev->lock we can trivially switch from rtnl_lock() to
> netdev_lock() for setting netdev->threaded via sysfs.
>
> Note that since we do not lock netdev_lock around sysfs
> calls in the core we don't have to "trylock" like we do
> with rtnl_lock.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: leitao@...ian.org
> ---
>  include/linux/netdevice.h | 13 +++++++++++--
>  net/core/dev.c            |  2 ++
>  net/core/net-sysfs.c      | 32 +++++++++++++++++++++++++++++++-
>  3 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d3108a12e562..75c30404657b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -384,7 +384,7 @@ struct napi_struct {
>         int                     rx_count; /* length of rx_list */
>         unsigned int            napi_id; /* protected by netdev_lock */
>         struct hrtimer          timer;
> -       struct task_struct      *thread;
> +       struct task_struct      *thread; /* protected by netdev_lock */
>         unsigned long           gro_flush_timeout;
>         unsigned long           irq_suspend_timeout;
>         u32                     defer_hard_irqs;
> @@ -2451,10 +2451,12 @@ struct net_device {
>          * Drivers are free to use it for other protection.
>          *
>          * Protects:
> -        *      @napi_list, @net_shaper_hierarchy, @reg_state
> +        *      @napi_list, @net_shaper_hierarchy, @reg_state, @threaded
>          * Partially protects (readers hold either @lock or rtnl_lock,
>          * writers must hold both for registered devices):
>          *      @up
> +        * Also protects some fields in struct napi_struct.
> +        *
>          * Ordering: take after rtnl_lock.
>          */
>         struct mutex            lock;
> @@ -2696,6 +2698,13 @@ static inline void netdev_assert_locked(struct net_device *dev)
>         lockdep_assert_held(&dev->lock);
>  }
>
> +static inline void netdev_assert_locked_or_invisible(struct net_device *dev)
> +{
> +       if (dev->reg_state == NETREG_REGISTERED ||
> +           dev->reg_state == NETREG_UNREGISTERING)
> +               netdev_assert_locked(dev);
> +}
> +
>  static inline void netif_napi_set_irq(struct napi_struct *napi, int irq)
>  {
>         napi->irq = irq;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1151baaedf4d..5872f0797cc3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6784,6 +6784,8 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
>         struct napi_struct *napi;
>         int err = 0;
>
> +       netdev_assert_locked_or_invisible(dev);
> +
>         if (dev->threaded == threaded)
>                 return 0;
>
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 2d9afc6e2161..5602a3c12e9a 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -108,6 +108,36 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
>         return ret;
>  }
>
> +/* Same as netdev_store() but takes netdev_lock() instead of rtnl_lock() */
> +static ssize_t
> +netdev_lock_store(struct device *dev, struct device_attribute *attr,
> +                 const char *buf, size_t len,
> +                 int (*set)(struct net_device *, unsigned long))
> +{
> +       struct net_device *netdev = to_net_dev(dev);
> +       struct net *net = dev_net(netdev);
> +       unsigned long new;
> +       int ret;
> +
> +       if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +               return -EPERM;
> +
> +       ret = kstrtoul(buf, 0, &new);
> +       if (ret)
> +               return ret;
> +
> +       netdev_lock(netdev);
> +
> +       if (dev_isalive(netdev)) {

nit: dev_isalive() is supposed to be called with RCU or RTNL, I guess
we should update its comment:

/* Caller holds RTNL or RCU */

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