Re: [PATCH net v3 1/2] octeon_ep: fix skb frags overflow in the RX path

[Jakub Kicinski <kuba@kernel.org>]

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: fix skb frags overflow in the RX path

This change adds a check in __octep_oq_process_rx() so that packets whose
fragment count would exceed MAX_SKB_FRAGS are dropped before build_skb()
via octep_oq_drop_rx(), mirroring the existing build_skb-failure path.

> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> index e6ebc7e44a00..bdbed58c7b2a 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> @@ -453,6 +453,15 @@ static int __octep_oq_process_rx(struct octep_device *oct,
>  
>  		octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
>  
> +		if (buff_info->len > oq->max_single_buffer_size) {
> +			u16 data_len = buff_info->len - oq->max_single_buffer_size;
> +
> +			if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) {
> +				octep_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> +				continue;
> +			}
> +		}
> +

[High]
Before this new check runs, the outer octep_oq_next_pkt() has already
processed the head descriptor:

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

At that point the head page has refcount 1 from dev_alloc_page() and no
struct sk_buff owns it yet, because build_skb() has not been called. resp_hw
is only a virtual pointer, so nothing else holds a reference to the page.

When this new path takes the continue via octep_oq_drop_rx(), the head
page is never freed — octep_oq_drop_rx() only walks fragment descriptors.

Does this drop path leak the head Rx page? The same question applies to
the pre-existing build_skb() failure path just below, but the new path
is device-triggerable in normal operation (packets larger than ~66 KB on
4 KB pages, which the commit message notes is reachable with data_len
close to 65535).

[High]
octep_oq_drop_rx() also appears to reuse the same head buff_info pointer
for every fragment descriptor:

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

The passed-in buff_info is the head slot's buff_info, not
&oq->buff_info[*read_idx] for each fragment. As a result
octep_oq_next_pkt() only clears the head slot's page pointer and never
NULLs the fragment slots' oq->buff_info[i].page.

Does this cause the fragment pages to be leaked on the next
octep_oq_refill(), where those non-NULL page pointers get overwritten
with a fresh dev_alloc_page() reference?

And if octep_oq_free_ring_buffers() runs before refill (netdev close or
reset), does it observe those stale page pointers and call
dma_unmap_page() on descriptor addresses that octep_oq_next_pkt() has
already unmapped inside drop_rx?

This defect predates this patch (it appears to come from the commit
that introduced octep_oq_drop_rx()), but this patch adds a new
device-triggerable call site (fragment count above MAX_SKB_FRAGS) that
would exercise it under normal jumbo Rx.

[Medium]
The new drop path silently discards the packet without updating any
counter:

> +			if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) {
> +				octep_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> +				continue;
> +			}

The commit message states:

    octep_oq_drop_rx() consumes its descriptors like the build_skb failure
    path.

The sibling build_skb() failure path increments oq->stats->alloc_failures,
but this new path does not, and ndo_get_stats64 (octep_get_stats64) is
not updated to bump rx_dropped either.

Should this drop bump a counter visible in ip -s link show or ethtool -S
so the loss is observable?