Re: [PATCH net] net/sched: re-cap skb->queue_mapping before indexing dev->_tx[]

[netdev-bot+sashiko@kernel.org]

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

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

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] qdisc_restart() caps queue_mapping only on the head skb that
  dequeue_skb() returns, but dequeue_skb() can return a list linked…

--

net/sched: re-cap skb->queue_mapping before indexing dev->_tx[]

This commit adds a netdev_cap_txqueue() call in qdisc_restart(). It
clamps skb->queue_mapping before skb_get_tx_queue() indexes dev->_tx[],
because a tc BPF program on a transmit qdisc can store an out-of-range
queue_mapping.

> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0d..c326d12942e52 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -432,6 +432,13 @@ static inline bool qdisc_restart(struct Qdisc *q, int *packets, int budget)
>  		root_lock = qdisc_lock(q);
>  
>  	dev = qdisc_dev(q);
> +	/* A tc BPF program attached to a transmit qdisc can store an
> +	 * arbitrary skb->queue_mapping after the tx queue was picked, so
> +	 * the value reaching here is not necessarily one the device can
> +	 * serve.  Re-cap it before it indexes dev->_tx[].
> +	 */
> +	skb_set_queue_mapping(skb,
> +			      netdev_cap_txqueue(dev, skb_get_queue_mapping(skb)));

[Severity: High]
Is this cap early enough? dequeue_skb() runs before it, and it can index
dev->_tx[] with a BPF-written mapping in two places in this file.

First, take a qdisc without TCQ_F_ONETXQUEUE, such as prio as root of a
multiqueue device. There, try_bulk_dequeue_skb_slow() parks an skb whose
mapping differs from the head's on skb_bad_txq, and leaves the mapping
unchanged:

net/sched/sch_generic.c:try_bulk_dequeue_skb_slow() {
    ...
		if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
			qdisc_enqueue_skb_bad_txq(q, nskb);
			break;
		}
    ...
}

On the next qdisc_restart(), the path
dequeue_skb()->qdisc_dequeue_skb_bad_txq()->__skb_dequeue_bad_txq()
does:

	skb = skb_peek(&q->skb_bad_txq);
	if (skb) {
		/* check the reason of requeuing without tx lock first */
		txq = skb_get_tx_queue(txq->dev, skb);
		if (!netif_xmit_frozen_or_stopped(txq)) {

Say the classifier writes 0 on one packet and 1000 on the next. Wouldn't
this read &dev->_tx[1000].state before the new cap runs?

Second, dev_requeue_skb() puts every unsent skb of a returned list on
q->gso_skb without changing it. Only the head of a bulk list is capped
here. So on NETDEV_TX_BUSY, on a stopped queue partway through the list,
or on the XFRM "again" path, the tail skbs are requeued with their
original mapping. The gso_skb branch of dequeue_skb() then does:

		/* check the reason of requeuing without tx lock first */
		txq = skb_get_tx_queue(txq->dev, skb);
		if (!netif_xmit_frozen_or_stopped(txq)) {

using that uncapped value.

The queue_mapping store rewrite in bpf_convert_ctx_access() only drops
stores >= NO_QUEUE_MAPPING. Any value below 0xffff can therefore reach
both paths, so the new comment's "Re-cap it before it indexes dev->_tx[]"
doesn't hold for them.

The existing cap in netdev_tx_queue_mapping() in net/core/dev.c is
applied where the value is produced, not at one consumer. Would capping
after the egress qdisc's classifier runs during enqueue, or in the ctx
store rewrite, cover all of these consumers instead of adding a cap in
qdisc_restart()?

>  	txq = skb_get_tx_queue(dev, skb);
>  
>  	return sch_direct_xmit(skb, q, dev, txq, root_lock, validate);

[Severity: High]
This caps only the head skb, but dequeue_skb() can return a list linked
through skb->next. try_bulk_dequeue_skb_slow() chains up to 8 skbs that
share the head's original mapping:

	int mapping = skb_get_queue_mapping(skb);
	...
		skb->next = nskb;
		skb = nskb;
	} while (++cnt < 8);

try_bulk_dequeue_skb() is used for TCQ_F_ONETXQUEUE qdiscs such as
mq/mqprio children. It chains skbs without checking their mapping at all.

Suppose a cls_bpf program writes 100 on every packet. After this change
the head skb maps to 0, but every other skb in the list still carries
100. sch_direct_xmit()->dev_hard_start_xmit() then walks the whole list:

	while (skb) {
		struct sk_buff *next = skb->next;

		skb_mark_not_on_list(skb);
		rc = xmit_one(skb, dev, txq, next != NULL);

xmit_one() passes each skb to ndo_start_xmit. Drivers such as virtio_net
index their per-queue arrays with the raw mapping:

drivers/net/virtio_net.c:start_xmit() {
	int qnum = skb_get_queue_mapping(skb);
	struct send_queue *sq = &vi->sq[qnum];
	int err;
	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
    ...
}

Can this still overrun vi->sq[] and dev->_tx[] for every packet after the
first in a bulk batch?

The commit message says:

    The same stale value is then handed to dev_hard_start_xmit(), which
    reports the same fault.

That path still looks reachable for the tail skbs. These tail skbs can
also be requeued to gso_skb, where they hit the uncapped lookup in
dequeue_skb() described above.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155450.66934-1-phx0fer%40gmail.com