Re: [PATCH net] net: sfc: fix using uninitialized xdp tx_queue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
>
> Acked-by: Martin Habets <habetsm.xilinx@xxxxxxxxx>
>

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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux