Re: [PATCH net v2] qede: Fix NULL pointer dereference in TPA fragment processing

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
qede: Fix NULL pointer dereference in TPA fragment processing

This patch adds NULL page validation in qede_fill_frag_skb(), moves the
TPA error-state check to run before fragment processing in
qede_tpa_end(), and adds a similar check in qede_tpa_cont(), so that
TPA aggregations whose start failed under memory pressure are dropped
instead of crashing.

Link: https://lore.kernel.org/netdev/20260709044704.141507-1-vnagare@xxxxxxxxxx/

> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index c11e0d8f98aa..3de6c5a6056c 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -698,7 +708,10 @@ static int qede_fill_frag_skb(struct qede_dev *edev,
>  
>  out:
>  	tpa_info->state = QEDE_AGG_STATE_ERROR;
> -	qede_recycle_rx_bd_ring(rxq, 1);
> +	if (current_bd->data)
> +		qede_recycle_rx_bd_ring(rxq, 1);
> +	else
> +		qede_rx_bd_ring_consume(rxq);

[Medium]
On the else branch, qede_rx_bd_ring_consume() advances rxq->sw_rx_cons
without a matching produce and without decrementing rxq->filled_buffers.

Every other consume in this driver is paired with either
qede_alloc_rx_buffer() (which increments sw_rx_prod and filled_buffers)
or qede_reuse_page() via qede_recycle_rx_bd_ring() (which produces a
replacement). Here neither happens.

The end-of-NAPI backfill in qede_rx_int() only tops up while
rxq->num_rx_buffers - rxq->filled_buffers is non-zero, so if
filled_buffers is never decremented, the backfill will not replace this
BD.

Does this mean each hit of the NULL-data drop path permanently shrinks
the number of BDs posted to hardware by one, causing the RX ring to
monotonically deplete under the sustained memory pressure this patch is
supposed to make survivable?

Would decrementing rxq->filled_buffers here (or attempting a
replacement allocation) keep the ring in sync with hardware?

>  
>  	return -ENOMEM;
>  }
> @@ -964,8 +977,16 @@ static inline void qede_tpa_cont(struct qede_dev *edev,
>  			     struct qede_rx_queue *rxq,
>  			     struct eth_fast_path_rx_tpa_cont_cqe *cqe)
>  {
> +	struct qede_agg_info *tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
>  	int i;
>  
> +	/* Don't process fragments if TPA start failed */
> +	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START)) {
> +		for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)
> +			qede_recycle_rx_bd_ring(rxq, 1);
> +		return;
> +	}
> +

[High]
The other half of the patch adds a guarded fallback in
qede_fill_frag_skb() with the following justification in the v2
changelog:

  Fixed NULL buffer recycling: check if page is valid before recycling,
  otherwise just consume the BD to prevent NULL from re-entering the
  ring

However this new loop calls qede_recycle_rx_bd_ring() unconditionally on
BDs that belong to the same aggregation whose sw_rx_data may have
data == NULL (that is the exact scenario in the commit message: step 2
says qede_alloc_rx_buffer() returned -ENOMEM leaving sw_rx_data->data
NULL, and step 4 says TPA_CONT is then delivered).

qede_recycle_rx_bd_ring() calls qede_reuse_page():

  static inline void qede_reuse_page(struct qede_rx_queue *rxq,
                                     struct sw_rx_data *curr_cons)
  {
          ...
          *curr_prod = *curr_cons;
          ...
  }

so a NULL data pointer (and possibly a zero mapping) will be copied into
the producer slot and pushed to hardware.

Doesn't this re-introduce exactly the NULL-in-the-ring condition that
the qede_fill_frag_skb() branch above was written to prevent, so the
next TPA aggregation that lands on the recycled slot crashes on the
same NAPI path this commit is trying to protect?

>  	for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)
>  		qede_fill_frag_skb(edev, rxq, cqe->tpa_agg_index,
>  				   le16_to_cpu(cqe->len_list[i]));
> @@ -987,6 +1008,18 @@ static int qede_tpa_end(struct qede_dev *edev,
>  	tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
>  	skb = tpa_info->skb;
>  
> +	/* Drop the packet if TPA start failed */
> +	if (unlikely(tpa_info->state != QEDE_AGG_STATE_START || !skb)) {
> +		/* Clean up: unmap DMA if needed */
> +		if (tpa_info->buffer.page_offset == PAGE_SIZE)
> +			dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
> +				       PAGE_SIZE, rxq->data_direction);
> +		/* Recycle BDs from cqe->len_list to keep ring synchronized */
> +		for (i = 0; cqe->len_list[i] && i < ARRAY_SIZE(cqe->len_list); i++)
> +			qede_recycle_rx_bd_ring(rxq, 1);

[High, High]
The two operands of && are reversed here compared to every other loop
over cqe->len_list in this file (including the sibling loop added just
above in qede_tpa_cont()):

  for (i = 0; cqe->len_list[i] && i < ARRAY_SIZE(cqe->len_list); i++)

Because && evaluates left-to-right, cqe->len_list[i] is read before the
bound check on i.

ETH_TPA_CQE_END_LEN_LIST_SIZE is 4, and in struct
eth_fast_path_rx_tpa_end_cqe the field immediately following len_list[]
is:

  __le16  len_list[ETH_TPA_CQE_END_LEN_LIST_SIZE];
  __le16  reserved3[ETH_TPA_CQE_END_LEN_LIST_SIZE];

If firmware delivers a TPA_END with all four len_list entries non-zero,
does iteration i == 4 read reserved3[0] as if it were len_list[4]?

If reserved3[0] happens to be non-zero, does the loop keep iterating
past the array end and call qede_recycle_rx_bd_ring() extra times,
advancing rxq->sw_rx_cons and producing buffers that firmware never
released, silently desynchronising the RX BD ring?

Would swapping the operands to match the pattern used in
qede_tpa_cont() and the two other loops in qede_tpa_end() fix this?

  for (i = 0; i < ARRAY_SIZE(cqe->len_list) && cqe->len_list[i]; i++)

Separately, this loop shares the same NULL-buffer concern as the one in
qede_tpa_cont() above: qede_recycle_rx_bd_ring() will copy a NULL
sw_rx_data into the producer slot for any BD whose allocation failed,
which is the exact scenario the commit message describes.

> +		goto err;
> +	}
> +
>  	if (tpa_info->buffer.page_offset == PAGE_SIZE)
>  		dma_unmap_page(rxq->dev, tpa_info->buffer.mapping,
>  			       PAGE_SIZE, rxq->data_direction);
-- 
pw-bot: cr