netdev - Re: [PATCH net-next 2/3] net: devmem: don't call queue stop / start when the interface is down

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: <CAHS8izNgVd_bPDCiFD5mN=TgkcaKmQK1RcLgw_051GRHcLXHvw@mail.gmail.com>
Date: Wed, 5 Feb 2025 12:35:30 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org
Subject: Re: [PATCH net-next 2/3] net: devmem: don't call queue stop / start
 when the interface is down

On Wed, Feb 5, 2025 at 11:02 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> We seem to be missing a netif_running() check from the devmem
> installation path. Starting a queue on a stopped device makes
> no sense. We still want to be able to allocate the memory, just
> to test that the device is indeed setting up the page pools
> in a memory provider compatible way.
>
> This is not a bug fix, because existing drivers check if
> the interface is down as part of the ops. But new drivers
> shouldn't have to do this, as long as they can correctly
> alloc/free while down.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  include/net/netdev_queues.h |  4 ++++
>  net/core/netdev_rx_queue.c  | 16 ++++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index b02bb9f109d5..73d3401261a6 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -117,6 +117,10 @@ struct netdev_stat_ops {
>   *
>   * @ndo_queue_stop:    Stop the RX queue at the specified index. The stopped
>   *                     queue's memory is written at the specified address.
> + *
> + * Note that @ndo_queue_mem_alloc and @ndo_queue_mem_free may be called while
> + * the interface is closed. @ndo_queue_start and @ndo_queue_stop will only
> + * be called for an interface which is open.
>   */
>  struct netdev_queue_mgmt_ops {
>         size_t                  ndo_queue_mem_size;
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index a5813d50e058..5352e0c1f37e 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -37,13 +37,17 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
>         if (err)
>                 goto err_free_new_queue_mem;
>
> -       err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);
> -       if (err)
> -               goto err_free_new_queue_mem;
> +       if (netif_running(dev)) {
> +               err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);
> +               if (err)
> +                       goto err_free_new_queue_mem;
>
> -       err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
> -       if (err)
> -               goto err_start_queue;
> +               err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
> +               if (err)
> +                       goto err_start_queue;
> +       } else {
> +               swap(new_mem, old_mem);
> +       }

Why not return an error if !netif_running(), and change the call site
in net_devmem_unbind_dmabuf() to not call into this if
!netif_running()? Is that a bit cleaner? It feels a bit weird to have
netdev_rx_queue_restart() do a bunch of allocations and driver calls
unnecessarily when it's really not going to do anything, no?

-- 
Thanks,
Mina