netdev - Re: [PATCH net] net: sfc: fix using uninitialized xdp tx

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:   Tue, 5 Apr 2022 09:20:09 +0100
From:   Martin Habets <habetsm.xilinx@...il.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, ecree.xilinx@...il.com, ast@...nel.org,
        daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com,
        cmclachlan@...arflare.com, bpf@...r.kernel.org
Subject: Re: [PATCH net] net: sfc: fix using uninitialized xdp tx_queue

Hi Taehee,

On Tue, Apr 05, 2022 at 05:00:19AM +0000, Taehee Yoo wrote:
> In some cases, xdp tx_queue can get used before initialization.
> 1. interface up/down
> 2. ring buffer size change
> 
> When CPU cores are lower than maximum number of channels of sfc driver,
> it creates new channels only for XDP.
> 
> When an interface is up or ring buffer size is changed, all channels
> are initialized.
> But xdp channels are always initialized later.
> So, the below scenario is possible.
> Packets are received to rx queue of normal channels and it is acted
> XDP_TX and tx_queue of xdp channels get used.
> But these tx_queues are not initialized yet.
> If so, TX DMA or queue error occurs.
> 
> In order to avoid this problem
> 1. initializes xdp tx_queues earlier than other rx_queue in
> efx_start_channels().
> 2. checks whether tx_queue is initialized or not in efx_xdp_tx_buffers().
> 
> Splat looks like:
>    sfc 0000:08:00.1 enp8s0f1np1: TX queue 10 spurious TX completion id 250
>    sfc 0000:08:00.1 enp8s0f1np1: resetting (RECOVER_OR_ALL)
>    sfc 0000:08:00.1 enp8s0f1np1: MC command 0x80 inlen 100 failed rc=-22
>    (raw=22) arg=789
>    sfc 0000:08:00.1 enp8s0f1np1: has been disabled
> 
> Fixes: dfe44c1f52ee ("sfc: handle XDP_TX outcomes of XDP eBPF programs")

A better fixes tag for this might be
f28100cb9c96 ("sfc: fix lack of XDP TX queues - error XDP TX failed (-22)")
as it enabled XDP in more situations.

> Signed-off-by: Taehee Yoo <ap420073@...il.com>

Acked-by: Martin Habets <habetsm.xilinx@...il.com>

> ---
>  drivers/net/ethernet/sfc/efx_channels.c | 2 +-
>  drivers/net/ethernet/sfc/tx.c           | 3 +++
>  drivers/net/ethernet/sfc/tx_common.c    | 2 ++
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index 83e27231fbe6..377df8b7f015 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -1140,7 +1140,7 @@ void efx_start_channels(struct efx_nic *efx)
>  	struct efx_rx_queue *rx_queue;
>  	struct efx_channel *channel;
>  
> -	efx_for_each_channel(channel, efx) {
> +	efx_for_each_channel_rev(channel, efx) {
>  		efx_for_each_channel_tx_queue(tx_queue, channel) {
>  			efx_init_tx_queue(tx_queue);
>  			atomic_inc(&efx->active_queues);
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index d16e031e95f4..6983799e1c05 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -443,6 +443,9 @@ int efx_xdp_tx_buffers(struct efx_nic *efx, int n, struct xdp_frame **xdpfs,
>  	if (unlikely(!tx_queue))
>  		return -EINVAL;
>  
> +	if (!tx_queue->initialised)
> +		return -EINVAL;
> +
>  	if (efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED)
>  		HARD_TX_LOCK(efx->net_dev, tx_queue->core_txq, cpu);
>  
> diff --git a/drivers/net/ethernet/sfc/tx_common.c b/drivers/net/ethernet/sfc/tx_common.c
> index d530cde2b864..9bc8281b7f5b 100644
> --- a/drivers/net/ethernet/sfc/tx_common.c
> +++ b/drivers/net/ethernet/sfc/tx_common.c
> @@ -101,6 +101,8 @@ void efx_fini_tx_queue(struct efx_tx_queue *tx_queue)
>  	netif_dbg(tx_queue->efx, drv, tx_queue->efx->net_dev,
>  		  "shutting down TX queue %d\n", tx_queue->queue);
>  
> +	tx_queue->initialised = false;
> +
>  	if (!tx_queue->buffer)
>  		return;
>  
> -- 
> 2.17.1