[PATCH net v2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()

[PATCH net v2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()

[Aleksandr Mishin]

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@redhat.com>
Signed-off-by: Aleksandr Mishin <amishin@t-argos.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;
+		}
+	}
+}
+
 /**
  * __octep_oq_process_rx() - Process hardware Rx queue and push to stack.
  *
@@ -367,10 +412,7 @@ static int __octep_oq_process_rx(struct octep_device *oct,
 	desc_used = 0;
 	for (pkt = 0; pkt < pkts_to_process; pkt++) {
 		buff_info = (struct octep_rx_buffer *)&oq->buff_info[read_idx];
-		dma_unmap_page(oq->dev, oq->desc_ring[read_idx].buffer_ptr,
-			       PAGE_SIZE, DMA_FROM_DEVICE);
 		resp_hw = page_address(buff_info->page);
-		buff_info->page = NULL;
 
 		/* Swap the length field that is in Big-Endian to CPU */
 		buff_info->len = be64_to_cpu(resp_hw->length);
@@ -394,31 +436,37 @@ static int __octep_oq_process_rx(struct octep_device *oct,
 			data_offset = OCTEP_OQ_RESP_HW_SIZE;
 			rx_ol_flags = 0;
 		}
+
+		skb = build_skb((void *)resp_hw, PAGE_SIZE);
+		if (!skb) {
+			octep_oq_drop_rx(oq, buff_info,
+					 &read_idx, &desc_used);
+			oq->stats.alloc_failures++;
+			continue;
+		}
+		skb_reserve(skb, data_offset);
+
+		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;
+
 		rx_bytes += buff_info->len;
 
 		if (buff_info->len <= oq->max_single_buffer_size) {
-			skb = build_skb((void *)resp_hw, PAGE_SIZE);
-			skb_reserve(skb, data_offset);
 			skb_put(skb, buff_info->len);
-			read_idx++;
-			desc_used++;
-			if (read_idx == oq->max_count)
-				read_idx = 0;
 		} else {
 			struct skb_shared_info *shinfo;
 			u16 data_len;
 
-			skb = build_skb((void *)resp_hw, PAGE_SIZE);
-			skb_reserve(skb, data_offset);
 			/* Head fragment includes response header(s);
 			 * subsequent fragments contains only data.
 			 */
 			skb_put(skb, oq->max_single_buffer_size);
-			read_idx++;
-			desc_used++;
-			if (read_idx == oq->max_count)
-				read_idx = 0;
-
 			shinfo = skb_shinfo(skb);
 			data_len = buff_info->len - oq->max_single_buffer_size;
 			while (data_len) {
-- 
2.30.2

Re: [PATCH net v2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()

[Simon Horman]

On Mon, Sep 16, 2024 at 09:02:12AM +0300, 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@redhat.com>
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.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.

Hi Veerasenareddy, Sathesh, and the team at Marvell,

This change looks correct to me, but I am wondering if
it could be exercised in order increase confidence in
it's run-time behaviour.

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

Hi Aleksandr,

Maybe this is taking things to far, but I notice that
the above patter appears several times in this patch.
And I am wondering if it makes sense to have a helper for it.

...

Re: [PATCH net v2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()

[Paolo Abeni]

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@redhat.com>
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.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