linux-kernel - Re: [PATCH net v2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process

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: <b9ae8575-f903-425f-aa42-0c2a7605aa94@redhat.com>
Date: Tue, 24 Sep 2024 10:45:59 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Aleksandr Mishin <amishin@...rgos.ru>,
 Veerasenareddy Burru <vburru@...vell.com>
Cc: Sathesh Edara <sedara@...vell.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Abhijit Ayarekar <aayarekar@...vell.com>,
 Satananda Burla <sburla@...vell.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH net v2] octeon_ep: Add SKB allocation failures handling in
 __octep_oq_process_rx()

On 9/16/24 08:02, Aleksandr Mishin wrote:
> build_skb() returns NULL in case of a memory allocation failure so handle
> it inside __octep_oq_process_rx() to avoid NULL pointer dereference.
> 
> __octep_oq_process_rx() is called during NAPI polling by the driver. If
> skb allocation fails, keep on pulling packets out of the Rx DMA queue: we
> shouldn't break the polling immediately and thus falsely indicate to the
> octep_napi_poll() that the Rx pressure is going down. As there is no
> associated skb in this case, don't process the packets and don't push them
> up the network stack - they are skipped.
> 
> The common code with skb and some index manipulations is extracted to make
> the fix more readable and avoid code duplication. Also helper function is
> implemented to unmmap/flush all the fragment buffers used by the dropped
> packet. 'alloc_failures' counter is incremented to mark the skb allocation
> error in driver statistics.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Suggested-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Aleksandr Mishin <amishin@...rgos.ru>
> ---
> A similar situation is present in the __octep_vf_oq_process_rx() of the
> Octeon VF driver. First we want to try the fix on __octep_oq_process_rx().
> 
> There are some doubts about increasing the 'rx_bytes'. On the one hand,
> the data has not been processed, therefore, the counter does not need to
> be increased. On the other hand, this counter is used to estimate the
> bandwidth at the card's input.
> In octeon_droq_fast_process_packet() from the Liquidio driver in
> 'droq->stats.bytes_received += total_len' everything that was received
> from the device is considered.
> /* Output Queue statistics. Each output queue has four stats fields. */
> struct octep_oq_stats {
> 	/* Number of packets received from the Device. */
> 	u64 packets;
> 	/* Number of bytes received from the Device. */
> 	u64 bytes;
> 	/* Number of times failed to allocate buffers. */
> 	u64 alloc_failures;
> };
> 
> Compile tested only.
> 
> v2:
>    - Implement helper instead of adding multiple checks for '!skb' and
>      remove 'rx_bytes' increasing in case of packet dropping as suggested
>      by Paolo
>      (https://lore.kernel.org/all/ba514498-3706-413b-a09f-f577861eef28@redhat.com/)
> v1: https://lore.kernel.org/all/20240906063907.9591-1-amishin@t-argos.ru/
> 
>   .../net/ethernet/marvell/octeon_ep/octep_rx.c | 80 +++++++++++++++----
>   1 file changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> index 4746a6b258f0..6b665263b9be 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> @@ -336,6 +336,51 @@ static int octep_oq_check_hw_for_pkts(struct octep_device *oct,
>   	return new_pkts;
>   }
>   
> +/**
> + * octep_oq_drop_rx() - Free the resources associated with a packet.
> + *
> + * @oq: Octeon Rx queue data structure.
> + * @buff_info: Current packet buffer info.
> + * @read_idx: Current packet index in the ring.
> + * @desc_used: Current packet descriptor number.
> + *
> + */
> +static void octep_oq_drop_rx(struct octep_oq *oq,
> +			     struct octep_rx_buffer *buff_info,
> +			     u32 *read_idx, u32 *desc_used)
> +{
> +	dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr,
> +		       PAGE_SIZE, DMA_FROM_DEVICE);
> +	buff_info->page = NULL;
> +	(*read_idx)++;
> +	(*desc_used)++;
> +	if (*read_idx == oq->max_count)
> +		*read_idx = 0;
> +
> +	if (buff_info->len > oq->max_single_buffer_size) {
> +		u16 data_len;
> +		/* Head fragment includes response header(s);
> +		 * subsequent fragments contains only data.
> +		 */
> +		data_len = buff_info->len - oq->max_single_buffer_size;
> +		while (data_len) {
> +			dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr,
> +				       PAGE_SIZE, DMA_FROM_DEVICE);
> +			buff_info = (struct octep_rx_buffer *)
> +				     &oq->buff_info[*read_idx];
> +			if (data_len < oq->buffer_size)
> +				data_len = 0;
> +			else
> +				data_len -= oq->buffer_size;
> +			buff_info->page = NULL;
> +			(*read_idx)++;
> +			(*desc_used)++;
> +			if (*read_idx == oq->max_count)
> +				*read_idx = 0;
> +		}
> +	}
> +}

I *think* you can simplify this helper always looping:

	int data_len = buff_info->len - oq->max_single_buffer_size;

	do {
		dma_unmap_page(oq->dev,
			oq->desc_ring[*read_idx].buffer_ptr,
			PAGE_SIZE, DMA_FROM_DEVICE);
		buff_info = (struct octep_rx_buffer *)
			     &oq->buff_info[*read_idx];
		data_len -= oq->buffer_size;
		buff_info->page = NULL;
		(*read_idx)++;
		(*desc_used)++;
		if (*read_idx == oq->max_count)
			*read_idx = 0;
	} while (data_len > 0);

Thanks,

Paolo