Re: [PATCH RFC net-next v4 05/14] xsk: add direct xmit in batch function

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Packet validation executed inside hardware transmit lock, causing severe lock contention
- [High] Missing transmit recursion protection introduces deadlock vectors
- [High] Missing device state validation allows transmission to stopped or suspended devices
- [Medium] Hardware doorbell batching is defeated by hardcoded 'more' parameter
--

commit b98984346f64d24e3ecd521b219464ccbe9fba91
Author: Jason Xing <kernelxing@tencent.com>

xsk: add direct xmit in batch function

This commit adds a batch transmission logic for AF_XDP. It introduces a
new direct transmit function that disables bottom halves, grabs the lock
once, and sends all aggregated packets in a loop.

> diff --git a/net/core/dev.c b/net/core/dev.c
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> +int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
> +{
> +	u16 queue_id = xs->queue_id;
> +	struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_id);
> +	struct sk_buff_head *send_queue = &xs->batch.send_queue;
> +	int ret = NETDEV_TX_BUSY;
> +	struct sk_buff *skb;
> +

Should there be a check for the device state before attempting to transmit?
Direct transmit paths that bypass the Qdisc layer, like __dev_direct_xmit(),
typically verify that the device is running and the carrier is up:

	if (unlikely(!netif_running(dev) ||
		     !netif_carrier_ok(dev)))
		goto drop;

Could invoking transmission on a stopped or suspended device cause issues?

> +	local_bh_disable();

Are we missing transmit recursion protection here? Standard paths wrap the
hardware transmit lock with recursion tracking to protect against stack
overflows and deadlocks when routing through virtual devices:

	dev_xmit_recursion_inc();
	HARD_TX_LOCK(dev, txq, smp_processor_id());

> +	HARD_TX_LOCK(dev, txq, smp_processor_id());
> +	while ((skb = __skb_dequeue(send_queue)) != NULL) {
> +		struct sk_buff *orig_skb = skb;
> +		bool again = false;
> +
> +		skb = validate_xmit_skb_list(skb, dev, &again);

Will executing validate_xmit_skb_list() inside the hardware transmit lock
cause lock contention? This function can perform computationally expensive
operations such as GSO segmentation, software checksum fallbacks, and
subsystem hooks. Other transmit paths execute this validation strictly
before acquiring the hardware lock.

> +		if (skb != orig_skb) {
> +			dev_core_stats_tx_dropped_inc(dev);
> +			kfree_skb_list(skb);
> +			ret = NET_XMIT_DROP;
> +			break;
> +		}
> +
> +		if (netif_xmit_frozen_or_drv_stopped(txq)) {
> +			__skb_queue_head(send_queue, skb);
> +			break;
> +		}
> +		skb_set_queue_mapping(skb, queue_id);
> +		ret = netdev_start_xmit(skb, dev, txq, false);

Does passing false for the more parameter defeat the purpose of hardware
doorbell batching? It instructs the driver to ring the hardware doorbell for
every single packet instead of delaying it until the end of the batch.

It looks like this might be fixed later in the patch series by commit
6d7c8f2ad056e ("xsk: support dynamic xmit.more control for batch xmit").

> +		if (ret != NETDEV_TX_OK) {
> +			if (ret == NETDEV_TX_BUSY)
> +				__skb_queue_head(send_queue, skb);
> +			break;
> +		}
> +	}
> +	HARD_TX_UNLOCK(dev, txq);

Also, we would need to pair any added recursion increment with a decrement
here:

	dev_xmit_recursion_dec();

> +	local_bh_enable();
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260415082654.21026-1-kerneljasonxing@gmail.com?part=5