[PATCH RFC 0/2] can: rx-offload: can_rx_offload_threaded_irq_flush(): partly flush skb_irq_queue

[PATCH RFC 0/2] can: rx-offload: can_rx_offload_threaded_irq_flush(): partly flush skb_irq_queue

[Marc Kleine-Budde]

This is a proof of conecpt series to partly flush the skb_irq_queue. It
should avoid issues pointed out by the netdev sashiko bot in Ciprian
Costea's series.

When the system is under heavy load, flushing is required to prevent an RX
starvation.

Partial flushing is required to prevent out of order RX errors when
combining multiple RX FIFOs.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (2):
      can: rx-offload: can_rx_offload_threaded_irq_flush(): add newfunction to be called from within loop of threaded interrupt handlers
      can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

 drivers/net/can/dev/rx-offload.c               | 88 ++++++++++++++++++++++++++
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c |  1 +
 include/linux/can/rx-offload.h                 |  4 ++
 3 files changed, 93 insertions(+)
---
base-commit: cfb7793d1bc0f7d90571611979654cf1b3886b29
change-id: 20261001-upstream-can-rx-offload-batching-alternative-c2a1d0b85a63

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[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

[Marc Kleine-Budde]

CAN drivers that use threaded IRQ handlers usually have a while loop that
runs until all IRQs have been processed. In this loop, received CAN frames
are placed in the offload->skb_irq_queue.

At the end of the IRQ handler, the offload->skb_irq_queue is spliced into
the offload->skb_queue and NAPI is started, which then pushes the CAN
frames into the network stack.

Under certain load situations, the threaded IRQ handler will not exit the
while loop, resulting in an unlimited growth of the offload->skb_irq_queue.

In order to avoid this situation add a new function
can_rx_offload_threaded_irq_flush(). It's meant to be called from within
the threaded IRQ while loop.

Check in can_rx_offload_threaded_irq_flush() whether
offload->skb_irq_queue exceeds 1/2 of the maximum queue length
(can_rx_offload->skb_queue_len_max) and save the last element of the queue.
If the queue exceeds 3/4 of the maximum queue length, spliced up to the
previously saved element into offload->skb_queue and start NAPI.

Why is not the whole offload->skb_irq_queue flushed to NAPI?

Some CAN-IP cores use more than one FIFO or even independent mailboxes. In
situations where reception from the CAN bus and reading of the
FIFOs/mailboxes take place simultaneously, older CAN frames may be present
in the chip at the end of the current IRQ handler loop than in the
offload->skb_irq_queue.

As the order of the CAN frames is decisive for most CAN protocols, leave
about 1/4 of the maximum queue length in offload->skb_irq_queue so that
older CAN frames can be added to the queue at the correct position in the
next loop of the IRQ handler.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/rx-offload.c | 88 ++++++++++++++++++++++++++++++++++++++++
 include/linux/can/rx-offload.h   |  4 ++
 2 files changed, 92 insertions(+)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 46e7b6db4a1e..9f494ab7561e 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -338,6 +338,8 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
 	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
 	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
 
+	offload->flush_skb = NULL;
+
 	queue_len = skb_queue_len(&offload->skb_queue);
 	if (queue_len > offload->skb_queue_len_max / 8)
 		netdev_dbg(offload->dev, "%s: queue_len=%d\n",
@@ -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
+ * @list: a new list to add all removed entries
+ * @head: a list with entries
+ * @entry: an entry within head, could be the head itself
+ *	and if so we won't cut the list
+ * @qlen: length up to @entry in @head
+ *
+ * This helper moves the initial part of @head, up to and including
+ * @entry, from @head to @list. You should pass on @entry an element
+ * you know is on @head and @qlen being the position of that element
+ * on @head. @list should be an empty list or a list you do not care
+ * about losing its data.
+ *
+ */
+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;
+}
+
+/**
+ * can_rx_offload_threaded_irq_flush() - partially flush the rx_offload queue
+ * @offload: pointer to rx_offload context
+ *
+ * If can_rx_offload->skb_irq_queue exceeds 3/4 of the maximal queue
+ * length (can_rx_offload->skb_queue_len_max) flush about 1/2 of the
+ * maximal queue length to NAPI.
+ *
+ * This function is intended to be called within the loop of threaded
+ * IRQ handlers, which run until all IRQs have been served.
+ *
+ */
+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;
+	}
+
+	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);
+
+	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;
+
+	queue_len = skb_queue_len(&offload->skb_queue);
+	if (queue_len > offload->skb_queue_len_max / 8)
+		netdev_dbg(offload->dev, "%s: queue_len=%d\n",
+			   __func__, queue_len);
+
+	local_bh_disable();
+	napi_schedule(&offload->napi);
+	local_bh_enable();
+}
+EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_flush);
+
 static int can_rx_offload_init_queue(struct net_device *dev,
 				     struct can_rx_offload *offload,
 				     unsigned int weight)
@@ -360,6 +446,8 @@ static int can_rx_offload_init_queue(struct net_device *dev,
 	offload->skb_queue_len_max *= 4;
 	skb_queue_head_init(&offload->skb_queue);
 	__skb_queue_head_init(&offload->skb_irq_queue);
+	offload->flush_skb = NULL;
+	offload->flush_len = 0;
 
 	netif_napi_add_weight(dev, &offload->napi, can_rx_offload_napi_poll,
 			      weight);
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index d29bb4521947..18dcced516a5 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -23,6 +23,9 @@ struct can_rx_offload {
 	struct sk_buff_head skb_irq_queue;
 	u32 skb_queue_len_max;
 
+	struct sk_buff *flush_skb;
+	u32 flush_len;
+
 	unsigned int mb_first;
 	unsigned int mb_last;
 
@@ -54,6 +57,7 @@ unsigned int can_rx_offload_get_echo_skb_queue_tail(struct can_rx_offload *offlo
 						    unsigned int *frame_len_ptr);
 void can_rx_offload_irq_finish(struct can_rx_offload *offload);
 void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload);
+void can_rx_offload_threaded_irq_flush(struct can_rx_offload *offload);
 void can_rx_offload_del(struct can_rx_offload *offload);
 void can_rx_offload_enable(struct can_rx_offload *offload);
 

-- 
2.53.0

[PATCH RFC 2/2] can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

[Marc Kleine-Budde]

Under certain load situations, the threaded IRQ handler will not exit
the while loop, resulting in unlimited growth of the
offload->skb_irq_queue.

Call into the newly added function can_rx_offload_threaded_irq_flush()
to flush parts of the offload->skb_irq_queue.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index f441f2265299..208595c0cc46 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -1615,6 +1615,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id)
 			}
 		}
 
+		can_rx_offload_threaded_irq_flush(&priv->offload);
 		handled = IRQ_HANDLED;
 	} while (1);
 

-- 
2.53.0

Re: [PATCH RFC 2/2] can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: mcp251xfd: crash in __skb_cut_position() due to out-of-order timestamps
--

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

can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

Under certain load situations, the threaded IRQ handler will not exit
the while loop, resulting in unlimited growth of the skb_irq_queue.
This commit adds a call into the newly added function
can_rx_offload_threaded_irq_flush() to flush parts of the queue.

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index f441f2265299e..208595c0cc469 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -1615,6 +1615,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id)
>  			}
>  		}
>  
> +		can_rx_offload_threaded_irq_flush(&priv->offload);

[Severity: Critical]
Does this new loop expose a regression in the flushing logic when dealing with
out-of-order timestamps?

The mcp251xfd_irq threaded handler iterates over multiple RX FIFOs
sequentially. Packets from different FIFOs can overlap chronologically,
meaning a newly read packet from FIFO 1 may have an older timestamp than
a previously read packet from FIFO 0.

When can_rx_offload_queue_timestamp() is called, __skb_queue_add_sort()
inserts this older packet before the newer one in the skb_irq_queue:

drivers/net/can/dev/rx-offload.c:__skb_queue_add_sort() {
    ...
    __skb_queue_after(head, insert, new);
    ...
}

However, can_rx_offload_threaded_irq_flush() caches flush_skb and flush_len
as a snapshot:

drivers/net/can/dev/rx-offload.c:can_rx_offload_threaded_irq_flush() {
    ...
    if (!offload->flush_skb) {
        offload->flush_skb = skb_peek_tail(&offload->skb_irq_queue);
        offload->flush_len = irq_queue_len;
    }
    ...
}

If an older packet is subsequently inserted before flush_skb, the physical
number of elements from the head to flush_skb will exceed the cached
flush_len.

When __skb_cut_position() slices the queue, it uses the stale flush_len:

drivers/net/can/dev/rx-offload.c:__skb_cut_position() {
    ...
    list->qlen = qlen;

    WRITE_ONCE(head->next, new_first);
    WRITE_ONCE(new_first->prev, (struct sk_buff *)head);
    head->qlen -= qlen;
    ...
}

This leaves skb_irq_queue.qlen under-decremented. Over multiple iterations
of this new while loop, qlen grows artificially large.

Eventually, irq_queue_len exceeds the flush threshold even when the queue is
physically empty, causing skb_peek_tail() to return NULL for flush_skb.
__skb_cut_position() then dereferences entry->next where entry is NULL:

drivers/net/can/dev/rx-offload.c:__skb_cut_position() {
    ...
    struct sk_buff *new_first = entry->next;
    ...
}

Can a remote attacker trigger this kernel panic by flooding the CAN bus
with traffic to distribute packets across multiple FIFOs, creating the
out-of-order timestamp scenario?

>  		handled = IRQ_HANDLED;
>  	} while (1);
>

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

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]

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4807 bytes --]

On 05.10.2026 10:53:08, sashiko-bot@kernel.org wrote:
> 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?

I see 2 options:
- update __skb_queue_add_sort to update offload->flush_len if needed
- skip __skb_cut_position() + skb_queue_splice_tail_init() altogether
  and move/sort one element by one from the skb_irq_queue to the
  skb_queue?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC 2/2] can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [High] This patch adds the first and only caller of
  can_rx_offload_threaded_irq_flush(), in a driver whose producers all…
- [Low] The commit fixes a user-visible defect (unbounded growth of
  offload->skb_irq_queue in the mcp251xfd threaded IRQ handler under bus…
- [Low] The kernel-doc of the new __skb_cut_position()
  (drivers/net/can/dev/rx-offload.c:354-368, added in the preceding…
- [Low] The kernel-doc of can_rx_offload_threaded_irq_flush() (rx-
  offload.c:388-399) says that once skb_irq_queue exceeds 3/4 of…

--

can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

This patch adds a call to can_rx_offload_threaded_irq_flush() at the end
of each pass through the main status-register loop in mcp251xfd_irq().
The goal is to stop offload->skb_irq_queue from growing without bound when
the threaded IRQ handler never leaves its loop under load.

> Under certain load situations, the threaded IRQ handler will not exit
> the while loop, resulting in unlimited growth of the
> offload->skb_irq_queue.

[ ... ]

> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

[Severity: Low]
This fixes a user-visible problem, so should it carry a Fixes: tag and
possibly a stable annotation?

The skb_irq_queue design and the threaded finish call in mcp251xfd_irq()
come from these commits:

c757096ea103 ("can: rx-offload: add skb queue for use during ISR")
30bfec4fec59 ("can: rx-offload: can_rx_offload_threaded_irq_finish(): add new function to be called from threaded interrupt")

Either one looks like a reasonable Fixes: target.

This patch also depends on the preceding commit, which adds
can_rx_offload_threaded_irq_flush() and the flush_skb/flush_len fields.
Neither commit records that dependency, and this patch will not build if
it is backported on its own.

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index f441f2265299e..208595c0cc469 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -1615,6 +1615,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id)
>  			}
>  		}
>  
> +		can_rx_offload_threaded_irq_flush(&priv->offload);

[Severity: High]
Does this also cover the RX-INT loop at the top of mcp251xfd_irq()?

The new flush runs only in the second loop, the one that reads the
status registers. When the microchip,rx-int GPIO is present and
rx_ring_num == 1, the first loop keeps running for as long as the RX-INT
pin is asserted:

mcp251xfd_irq() {
	if (priv->rx_int)
		do {
			rx_pending = gpiod_get_value_cansleep(priv->rx_int);
			if (!rx_pending)
				break;
			...
			err = mcp251xfd_handle(priv, rxif);
			...
		} while (priv->rx_ring_num == 1);
	...
}

Each pass goes through
mcp251xfd_handle_rxif_one()->can_rx_offload_queue_timestamp(), which adds
the skb to offload->skb_irq_queue. The only admission check there looks
at skb_queue, not at skb_irq_queue:

can_rx_offload_queue_timestamp() {
	if (skb_queue_len(&offload->skb_queue) >
	    offload->skb_queue_len_max) {
		dev_kfree_skb_any(skb);
		return -ENOBUFS;
	}
	...
	__skb_queue_add_sort(&offload->skb_irq_queue, skb,
			     can_rx_offload_compare);
}

Nothing in this loop moves skbs to skb_queue or schedules NAPI, so the
check never fires.

rx_ring_num is 1 whenever rx_obj_num <= MCP251XFD_FIFO_DEPTH, for example
after "ethtool -G rx 32". If back-to-back frames on the bus keep the RX
FIFO non-empty, can skb_irq_queue still grow without bound here? That is
the problem the commit message describes.

Would it make sense to call can_rx_offload_threaded_irq_flush() inside
the RX-INT loop as well, or to count skb_irq_queue in the admission check?

[Severity: High]
This is the first caller of can_rx_offload_threaded_irq_flush(), and every
producer in this driver inserts into skb_irq_queue in timestamp order.
Can the saved flush_skb/flush_len pair go stale between the 1/2 call and
the 3/4 call?

can_rx_offload_threaded_irq_flush() {
	...
	if (!offload->flush_skb) {
		offload->flush_skb = skb_peek_tail(&offload->skb_irq_queue);
		offload->flush_len = irq_queue_len;
	}
	...
	__skb_cut_position(&tmp_queue, &offload->skb_irq_queue,
			   offload->flush_skb, offload->flush_len);
	...
}

__skb_cut_position() cuts by pointer, but it takes both qlen values from
the caller and never counts the list:

	list->qlen = qlen;
	...
	head->qlen -= qlen;

Between the two calls, __skb_queue_add_sort() puts a new skb in front of
flush_skb whenever its timestamp is older. In mcp251xfd this seems to
happen routinely:

- mcp251xfd_handle_rxovif() queues an error skb stamped with a live TBC
  read from mcp251xfd_alloc_can_err_skb(). On the next pass, the frames
  still in the full RX FIFO sort in front of it.
- With TX IRQ coalescing, TEF echo skbs from mcp251xfd_handle_tefif_one()
  carry older TX timestamps.
- With multiple RX FIFOs, frames from another ring can be older than the
  saved tail.

The preceding commit's own description expects older frames to be "added
to the queue at the correct position in the next loop".

If k skbs land in front of flush_skb, the cut moves flush_len + k skbs
but counts only flush_len. skb_irq_queue.qlen ends up k too high and
skb_queue.qlen k too low. Once NAPI drains skb_queue with skb_dequeue(),
skb_queue.qlen wraps to about 2^32 - k.

>From then on, can_rx_offload_queue_timestamp() sees
skb_queue_len(&offload->skb_queue) > skb_queue_len_max and frees every new
RX, TEF echo and error skb, even though skb_queue is actually empty.

If the cut left the skb_irq_queue list empty,
can_rx_offload_threaded_irq_finish() returns early:

	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
		return;

skb_queue_empty_lockless() checks only the pointers, so the wrong qlen is
never handed back. Nothing else resets qlen, because
can_rx_offload_enable() only calls napi_enable().

Would this leave the interface unable to receive until the driver is
unbound?

There is also a possible NULL dereference. If a stale skb_irq_queue.qlen
on an empty list ever reached 3/4 of the max, skb_peek_tail() would
return NULL and __skb_cut_position() would dereference entry->next. That
needs k >= 384, which the FIFO depths make unlikely.

[Severity: Low]
This isn't a bug, but the kernel-doc of __skb_cut_position(), added in
the preceding commit, doesn't match the function body:

 * @entry: an entry within head, could be the head itself
 *	and if so we won't cut the list

The body has no entry == head check. If entry == (struct sk_buff *)head,
@list is left with list->next pointing at the first element and
list->prev pointing at head, while list->qlen is 0. skb_queue_empty(list)
then returns false for a list that isn't valid. list_cut_position(), where
this comment was copied from, handles that case with INIT_LIST_HEAD(list).

The only caller passes skb_peek_tail(), so this can't happen today. Could
the comment be corrected, or the check added?

[Severity: Low]
The kernel-doc of can_rx_offload_threaded_irq_flush() says that once
skb_irq_queue exceeds 3/4 of the maximum it will "flush about 1/2 of the
maximal queue length". The code instead flushes whatever was queued when
flush_skb was first saved:

	if (!offload->flush_skb) {
		offload->flush_skb = skb_peek_tail(&offload->skb_irq_queue);
		offload->flush_len = irq_queue_len;
	}

	if (irq_queue_len < offload->skb_queue_len_max * 3 / 4)
		return;

If the first call with qlen >= 1/2 max already sees qlen >= 3/4 max,
flush_skb is the current tail and the whole queue goes to NAPI.

One pass of the main loop can't cross both thresholds. It adds at most
about 112 skbs, and the gap between 256 and 384 is 128.

The RX-INT loop with rx_ring_num == 1, however, can queue any number of
skbs without flushing. The first flush from this call site in
mcp251xfd_irq() can then send everything to NAPI.

Would older TEF echo or error frames handled after that point then be
delivered after newer RX frames?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-upstream-can-rx-offload-batching-alternative-v1-0-4c08ccb516ec%40pengutronix.de