Re: [PATCH RFC 1/2] can: rx-offload: can_rx_offload_threaded_irq_flush(): add newfunction to be called from within loop of threaded interrupt handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: stale flush_len causes skb_queue.qlen underflow and permanent packet drops
--

commit eec89aef49a7f405effd6fda359db0f458f6f4df
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: rx-offload: can_rx_offload_threaded_irq_flush(): add newfunction to be called from within loop of threaded interrupt handlers

This commit introduces can_rx_offload_threaded_irq_flush() to prevent
unlimited growth of the IRQ skb queue during heavy load. It partially flushes
skb_irq_queue to the NAPI skb_queue when it reaches 3/4 of its maximum
capacity. A portion of the elements are left behind to accommodate older
out-of-order CAN frames.

> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index 46e7b6db4a1ef..9f494ab7561ef 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
[ ... ]
> @@ -349,6 +351,90 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_finish);
>  
> +/**
> + * __skb_cut_position - cut a skb list into two
[ ... ]
> + */
> +static inline void __skb_cut_position(struct sk_buff_head *list,
> +				      struct sk_buff_head *head,
> +				      struct sk_buff *entry,
> +				      __u32 qlen)
> +{
> +	struct sk_buff *new_first = entry->next;
> +	struct sk_buff *first = head->next;
> +
> +	WRITE_ONCE(list->next, first);
> +	WRITE_ONCE(list->next->prev, (struct sk_buff *)list);
> +	WRITE_ONCE(list->prev, entry);
> +	WRITE_ONCE(entry->next, (struct sk_buff *)list);
> +	list->qlen = qlen;
> +
> +	WRITE_ONCE(head->next, new_first);
> +	WRITE_ONCE(new_first->prev, (struct sk_buff *)head);
> +	head->qlen -= qlen;

[Severity: High]
Could this lead to list->qlen undercounting the elements passed to NAPI?

If an older out-of-order CAN frame is inserted into skb_irq_queue before
offload->flush_skb after flush_len has been cached, the physical list
splice moves more than qlen elements up to entry. Statically updating the
length counters using the stale qlen will cause an eventual underflow in NAPI
dequeue.

> +}
> +
> +/**
> + * can_rx_offload_threaded_irq_flush() - partially flush the rx_offload queue
[ ... ]
> + */
> +void can_rx_offload_threaded_irq_flush(struct can_rx_offload *offload)
> +{
> +	struct sk_buff_head tmp_queue;
> +	unsigned long flags;
> +	u32 irq_queue_len, queue_len;
> +
> +	irq_queue_len = skb_queue_len(&offload->skb_irq_queue);
> +	if (irq_queue_len < offload->skb_queue_len_max / 2)
> +		return;
> +
> +	if (!offload->flush_skb) {
> +		offload->flush_skb = skb_peek_tail(&offload->skb_irq_queue);
> +		offload->flush_len = irq_queue_len;

[Severity: High]
Is it possible for the number of elements preceding the tail to change after
being cached here?

The commit message mentions that older out-of-order CAN frames may arrive from
independent mailboxes and be sorted into skb_irq_queue. If an older frame is
subsequently received, it would be inserted before flush_skb, physically
increasing the number of elements before flush_skb to flush_len + 1.

> +	}
> +
> +	if (irq_queue_len < offload->skb_queue_len_max * 3 / 4)
> +		return;
> +
> +	__skb_cut_position(&tmp_queue, &offload->skb_irq_queue,
> +			   offload->flush_skb, offload->flush_len);

[Severity: High]
Does passing the stale flush_len to the slice operation hide out-of-order
inserted frames from queue length accounting?

When __skb_cut_position is called, it physically moves flush_len + 1
elements into tmp_queue, but assigns tmp_queue.qlen the stale flush_len.
After splicing, skb_queue receives flush_len + 1 elements but its qlen
only increases by flush_len.

When NAPI dequeue consumes all elements, skb_queue.qlen would drop below 0
and underflow to 0xFFFFFFFF. Since this exceeds skb_queue_len_max, would all
future frames be permanently rejected with -ENOBUFS?

> +
> +	spin_lock_irqsave(&offload->skb_queue.lock, flags);
> +	skb_queue_splice_tail_init(&tmp_queue, &offload->skb_queue);
> +	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
> +
> +	offload->flush_skb = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-upstream-can-rx-offload-batching-alternative-v1-0-4c08ccb516ec@pengutronix.de?part=1