Re: [PATCH net v3 2/2] octeon_ep_vf: 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_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.

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

> @@ -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()?