Re: [PATCH net v3 2/2] octeon_ep_vf: fix skb frags overflow in the RX path
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-20 23:49:57
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- octeon_ep_vf: fix skb frags overflow in the RX path This patch adds a MAX_SKB_FRAGS check in __octep_vf_oq_process_rx() so packets that would require more fragments than the skb can hold are dropped, and factors the descriptor-drain loop into a new octep_vf_oq_drop_rx() helper that is shared with the napi_build_skb() failure path.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c index d98247408242..aa77b673ae3e 100644 --- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c +++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c@@ -357,6 +357,29 @@ static inline u32 octep_vf_oq_next_idx(struct octep_vf_oq *oq, u32 idx) return (idx + 1 == oq->max_count) ? 0 : idx + 1; } +static void octep_vf_oq_drop_rx(struct octep_vf_oq *oq, + struct octep_vf_rx_buffer *buff_info, + u32 *read_idx, u32 *desc_used) +{ + u16 data_len = buff_info->len - oq->max_single_buffer_size; + + (*desc_used)++; + *read_idx = octep_vf_oq_next_idx(oq, *read_idx); + while (data_len) { + dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr, + PAGE_SIZE, DMA_FROM_DEVICE); + buff_info = (struct octep_vf_rx_buffer *) + &oq->buff_info[*read_idx]; + buff_info->page = NULL; + if (data_len < oq->buffer_size) + data_len = 0; + else + data_len -= oq->buffer_size; + (*desc_used)++; + *read_idx = octep_vf_oq_next_idx(oq, *read_idx); + } +}
[High]
Does octep_vf_oq_drop_rx() leak the pages backing the dropped
descriptors?
The helper dma_unmap_page()s each fragment descriptor and then just sets
buff_info->page = NULL, without ever calling put_page() on the page that
was allocated by dev_alloc_page() in octep_vf_oq_fill_ring_buffers() or
octep_vf_oq_refill().
Compare with octep_vf_oq_free_ring_buffers() in the same file, which
appears to be the intended release pattern:
for (i = 0; i < oq->max_count; i++) {
if (oq->buff_info[i].page) {
dma_unmap_page(oq->dev, desc_ring[i].buffer_ptr,
PAGE_SIZE, DMA_FROM_DEVICE);
put_page(oq->buff_info[i].page);
oq->buff_info[i].page = NULL;
desc_ring[i].buffer_ptr = 0;
}
}
After octep_vf_oq_drop_rx() returns, octep_vf_oq_refill() will overwrite
buff_info[refill_idx].page with a freshly allocated page from
dev_alloc_page(), so the previously-mapped page pointer is lost without
a matching put_page(), isn't it?
The head descriptor of the dropped packet looks affected too. The caller
in __octep_vf_oq_process_rx() has already stashed the head page's kernel
address into resp_hw and cleared buff_info->page:
resp_hw = page_address(buff_info->page);
buff_info->page = NULL;
On the two paths that now call octep_vf_oq_drop_rx() (the new
MAX_SKB_FRAGS-exceeded branch and the napi_build_skb() failure branch),
napi_build_skb() is either never called or returned NULL, so no skb has
taken ownership of resp_hw. Is the head page also leaked in these cases?
quoted hunk ↗ jump to hunk
@@ -431,25 +454,16 @@ static int __octep_vf_oq_process_rx(struct octep_vf_device *oct, struct skb_shared_info *shinfo; u16 data_len; + data_len = buff_info->len - oq->max_single_buffer_size; + if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) { + octep_vf_oq_drop_rx(oq, buff_info, &read_idx, &desc_used); + continue; + } + skb = napi_build_skb((void *)resp_hw, PAGE_SIZE); if (!skb) { oq->stats->alloc_failures++; - desc_used++; - read_idx = octep_vf_oq_next_idx(oq, read_idx); - 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_vf_rx_buffer *) - &oq->buff_info[read_idx]; - buff_info->page = NULL; - if (data_len < oq->buffer_size) - data_len = 0; - else - data_len -= oq->buffer_size; - desc_used++; - read_idx = octep_vf_oq_next_idx(oq, read_idx); - } + octep_vf_oq_drop_rx(oq, buff_info, &read_idx, &desc_used); continue; }
[High] The napi_build_skb() failure path had the same missing put_page() before this patch, so that part is a pre-existing issue. The newly added MAX_SKB_FRAGS check reaches octep_vf_oq_drop_rx() on a device-triggered path (buff_info->len comes from the response header), so a misbehaving or malicious device that repeatedly sends packets whose fragment count exceeds MAX_SKB_FRAGS would drop roughly 18 pages per packet with no accounting. Would it be reasonable to have octep_vf_oq_drop_rx() put_page() the head page (or have the caller do so before invoking it) and put_page() each fragment page after dma_unmap_page(), mirroring the release sequence in octep_vf_oq_free_ring_buffers()?