netdev - Re: [PATCH] sfc: adjust efx->xdp_tx_queue_count with the real number of initialized queues

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]

Date:   Thu, 29 Apr 2021 15:49:45 +0100
From:   Ignat Korchagin <ignat@...udflare.com>
To:     Edward Cree <ecree.xilinx@...il.com>
Cc:     habetsm.xilinx@...il.com, "David S. Miller" <davem@...emloft.net>,
        kuba@...nel.org, netdev@...r.kernel.org,
        kernel-team <kernel-team@...udflare.com>, stable@...r.kernel.org
Subject: Re: [PATCH] sfc: adjust efx->xdp_tx_queue_count with the real number
 of initialized queues

On Thu, Apr 29, 2021 at 3:22 PM Edward Cree <ecree.xilinx@...il.com> wrote:
>
> On 27/04/2021 22:09, Ignat Korchagin wrote:
> > efx->xdp_tx_queue_count is initially initialized to num_possible_cpus() and is
> > later used to allocate and traverse efx->xdp_tx_queues lookup array. However,
> > we may end up not initializing all the array slots with real queues during
> > probing. This results, for example, in a NULL pointer dereference, when running
> > "# ethtool -S <iface>", similar to below
> ...
> > diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> > index 1bfeee283ea9..a3ca406a3561 100644
> > --- a/drivers/net/ethernet/sfc/efx_channels.c
> > +++ b/drivers/net/ethernet/sfc/efx_channels.c
> > @@ -914,6 +914,8 @@ int efx_set_channels(struct efx_nic *efx)
> >                       }
> >               }
> >       }
> > +     if (xdp_queue_number)
> Wait, why is this guard condition needed?
> What happens if we had nonzero efx->xdp_tx_queue_count initially, but we end up
>  with no TXQs available for XDP at all (so xdp_queue_number == 0)?
>
> -ed

My thoughts were: efx->xdp_tx_queue_count is originally used to
allocate efx->xdp_tx_queues.
So, if xdp_queue_number ends up being 0, we should keep
efx->xdp_tx_queue_count positive not
to forget to release efx->xdp_tx_queues (because most checks are
efx->xdp_tx_queue_count && efx->xdp_tx_queues).

I'm not familiar enough with SFC internals to definitely say if it is
even possible to have
xdp_queue_number == 0 while having efx->xdp_tx_queue_count > 0, but my
understanding is that
it should not be possible due to the checks in the driver init path,
when we actually determine the number
of queues, channels, events per channel etc.

Ignat

> > +             efx->xdp_tx_queue_count = xdp_queue_number;
> >
> >       rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
> >       if (rc)
> >
>