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] [day] [month] [year] [list]

Message-ID: <f308136f-0744-0a78-d882-bfd926fe1ea2@gmail.com>
Date:   Tue, 5 Apr 2022 17:41:20 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     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

On 4/5/22 17:20, Martin Habets wrote:

Hi Martin,
Thank you so much for your review!

 > 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>
 >

Thanks, I will send a v2 patch to change fixes tag and it will contain 
your Acked tag.

Thanks a lot,
Taehee Yoo

 >> ---
 >>   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