* [PATCH net v5 1/2] octeon_ep: Implement helper for iterating packets in Rx queue
2024-10-17 10:06 [PATCH net v5 0/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() Aleksandr Mishin
@ 2024-10-17 10:06 ` Aleksandr Mishin
2024-10-17 18:08 ` Jacob Keller
2024-10-17 10:06 ` [PATCH net v5 2/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() Aleksandr Mishin
2024-10-20 14:50 ` [PATCH net v5 0/2] " patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Aleksandr Mishin @ 2024-10-17 10:06 UTC (permalink / raw)
To: Veerasenareddy Burru, Abhijit Ayarekar, Satananda Burla, Sathesh Edara
Cc: Aleksandr Mishin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel, lvc-project, Simon Horman
The common code with some packet and index manipulations is extracted and
moved to newly implemented helper to make the code more readable and avoid
duplication. This is a preparation for skb allocation failure handling.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Suggested-by: Simon Horman <horms@kernel.org>
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
Compile tested only.
v5:
- Unmap paged buffer before the skb creation as suggested by Paolo
(https://lore.kernel.org/all/cf656975-69b4-427e-8769-d16575774bba@redhat.com/)
v4: https://lore.kernel.org/all/20241012094950.9438-1-amishin@t-argos.ru/
- Split patch up as suggested by Jakub
(https://lore.kernel.org/all/20241004073311.223efca4@kernel.org/)
v3: https://lore.kernel.org/all/20240930053328.9618-1-amishin@t-argos.ru/
- Implement helper which frees current packet resources and increase
index and descriptor as suggested by Simon
(https://lore.kernel.org/all/20240919134812.GB1571683@kernel.org/)
- v3 has been reviewed-by Simon Horman
(https://lore.kernel.org/all/20240930162622.GF1310185@kernel.org/)
v1: https://lore.kernel.org/all/20240906063907.9591-1-amishin@t-argos.ru/
.../net/ethernet/marvell/octeon_ep/octep_rx.c | 55 +++++++++++--------
1 file changed, 32 insertions(+), 23 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..a889c1510518 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
@@ -336,6 +336,30 @@ static int octep_oq_check_hw_for_pkts(struct octep_device *oct,
return new_pkts;
}
+/**
+ * octep_oq_next_pkt() - Move to the next packet in Rx queue.
+ *
+ * @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.
+ *
+ * Free the resources associated with a packet.
+ * Increment packet index in the ring and packet descriptor number.
+ */
+static void octep_oq_next_pkt(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;
+}
+
/**
* __octep_oq_process_rx() - Process hardware Rx queue and push to stack.
*
@@ -367,10 +391,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,36 +415,27 @@ static int __octep_oq_process_rx(struct octep_device *oct,
data_offset = OCTEP_OQ_RESP_HW_SIZE;
rx_ol_flags = 0;
}
+
+ octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
+
+ skb = build_skb((void *)resp_hw, PAGE_SIZE);
+ skb_reserve(skb, data_offset);
+
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) {
- 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) {
@@ -438,11 +450,8 @@ static int __octep_oq_process_rx(struct octep_device *oct,
buff_info->page, 0,
buff_info->len,
buff_info->len);
- buff_info->page = NULL;
- read_idx++;
- desc_used++;
- if (read_idx == oq->max_count)
- read_idx = 0;
+
+ octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
}
}
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v5 2/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()
2024-10-17 10:06 [PATCH net v5 0/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() Aleksandr Mishin
2024-10-17 10:06 ` [PATCH net v5 1/2] octeon_ep: Implement helper for iterating packets in Rx queue Aleksandr Mishin
@ 2024-10-17 10:06 ` Aleksandr Mishin
2024-10-17 18:09 ` Jacob Keller
2024-10-20 14:50 ` [PATCH net v5 0/2] " patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Aleksandr Mishin @ 2024-10-17 10:06 UTC (permalink / raw)
To: Veerasenareddy Burru, Abhijit Ayarekar, Satananda Burla, Sathesh Edara
Cc: Aleksandr Mishin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel, lvc-project
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.
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>
---
Compile tested only.
v5:
- Update helper as suggested by Paolo
(https://lore.kernel.org/all/cf656975-69b4-427e-8769-d16575774bba@redhat.com/)
v4: https://lore.kernel.org/all/20241012094950.9438-1-amishin@t-argos.ru/
- Split patch up as suggested by Jakub
(https://lore.kernel.org/all/20241004073311.223efca4@kernel.org/)
v3: https://lore.kernel.org/all/20240930053328.9618-1-amishin@t-argos.ru/
- Optimize helper as suggested by Paolo
(https://lore.kernel.org/all/b9ae8575-f903-425f-aa42-0c2a7605aa94@redhat.com/)
- v3 has been reviewed-by Simon Horman
(https://lore.kernel.org/all/20240930162622.GF1310185@kernel.org/)
v2: https://lore.kernel.org/all/20240916060212.12393-1-amishin@t-argos.ru/
- 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 | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
index a889c1510518..8af75cb37c3e 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
@@ -360,6 +360,27 @@ static void octep_oq_next_pkt(struct octep_oq *oq,
*read_idx = 0;
}
+/**
+ * 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)
+{
+ int data_len = buff_info->len - oq->max_single_buffer_size;
+
+ while (data_len > 0) {
+ octep_oq_next_pkt(oq, buff_info, read_idx, desc_used);
+ data_len -= oq->buffer_size;
+ };
+}
+
/**
* __octep_oq_process_rx() - Process hardware Rx queue and push to stack.
*
@@ -419,6 +440,12 @@ static int __octep_oq_process_rx(struct octep_device *oct,
octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
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);
rx_bytes += buff_info->len;
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net v5 0/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()
2024-10-17 10:06 [PATCH net v5 0/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() Aleksandr Mishin
2024-10-17 10:06 ` [PATCH net v5 1/2] octeon_ep: Implement helper for iterating packets in Rx queue Aleksandr Mishin
2024-10-17 10:06 ` [PATCH net v5 2/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() Aleksandr Mishin
@ 2024-10-20 14:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-10-20 14:50 UTC (permalink / raw)
To: Aleksandr Mishin
Cc: vburru, aayarekar, sburla, sedara, davem, edumazet, kuba, pabeni,
netdev, linux-kernel, lvc-project
Hello:
This series was applied to netdev/net.git (main)
by Andrew Lunn <andrew@lunn.ch>:
On Thu, 17 Oct 2024 13:06:49 +0300 you wrote:
> __octep_oq_process_rx() is called during NAPI polling by the driver and
> calls build_skb() which may return NULL as skb pointer in case of memory
> allocation error. This pointer is dereferenced later without checking for
> NULL.
>
> In this series, we introduce two helpers to make the fix more readable and
> avoid code duplication. Also we handle build_skb() errors inside
> __octep_oq_process_rx() to avoid NULL pointer dereference.
>
> [...]
Here is the summary with links:
- [net,v5,1/2] octeon_ep: Implement helper for iterating packets in Rx queue
https://git.kernel.org/netdev/net/c/bd28df26197b
- [net,v5,2/2] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx()
https://git.kernel.org/netdev/net/c/eb592008f79b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread