| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fecfcad0-7a16-42b8-bff2-66ee83a6e5c4@linux.dev>
Date: Mon, 9 Jun 2025 15:09:55 -0700
From: Ihor Solodrai <ihor.solodrai@...ux.dev>
To: Jesper Dangaard Brouer <hawk@...nel.org>, netdev@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>
Cc: bpf@...r.kernel.org, tom@...bertland.com,
Eric Dumazet <eric.dumazet@...il.com>, "David S. Miller"
<davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>,
Toke Høiland-Jørgensen <toke@...e.dk>,
dsahern@...nel.org, makita.toshiaki@....ntt.co.jp,
kernel-team@...udflare.com, phil@....cc
Subject: Re: [PATCH net-next V7 2/2] veth: apply qdisc backpressure on full
ptr_ring to reduce TX drops
On 4/25/25 7:55 AM, Jesper Dangaard Brouer wrote:
> In production, we're seeing TX drops on veth devices when the ptr_ring
> fills up. This can occur when NAPI mode is enabled, though it's
> relatively rare. However, with threaded NAPI - which we use in
> production - the drops become significantly more frequent.
>
> The underlying issue is that with threaded NAPI, the consumer often runs
> on a different CPU than the producer. This increases the likelihood of
> the ring filling up before the consumer gets scheduled, especially under
> load, leading to drops in veth_xmit() (ndo_start_xmit()).
>
> This patch introduces backpressure by returning NETDEV_TX_BUSY when the
> ring is full, signaling the qdisc layer to requeue the packet. The txq
> (netdev queue) is stopped in this condition and restarted once
> veth_poll() drains entries from the ring, ensuring coordination between
> NAPI and qdisc.
>
> Backpressure is only enabled when a qdisc is attached. Without a qdisc,
> the driver retains its original behavior - dropping packets immediately
> when the ring is full. This avoids unexpected behavior changes in setups
> without a configured qdisc.
>
> With a qdisc in place (e.g. fq, sfq) this allows Active Queue Management
> (AQM) to fairly schedule packets across flows and reduce collateral
> damage from elephant flows.
>
> A known limitation of this approach is that the full ring sits in front
> of the qdisc layer, effectively forming a FIFO buffer that introduces
> base latency. While AQM still improves fairness and mitigates flow
> dominance, the latency impact is measurable.
>
> In hardware drivers, this issue is typically addressed using BQL (Byte
> Queue Limits), which tracks in-flight bytes needed based on physical link
> rate. However, for virtual drivers like veth, there is no fixed bandwidth
> constraint - the bottleneck is CPU availability and the scheduler's ability
> to run the NAPI thread. It is unclear how effective BQL would be in this
> context.
>
> This patch serves as a first step toward addressing TX drops. Future work
> may explore adapting a BQL-like mechanism to better suit virtual devices
> like veth.
>
> Reported-by: Yan Zhai <yan@...udflare.com>
> Signed-off-by: Jesper Dangaard Brouer <hawk@...nel.org>
> ---
> drivers/net/veth.c | 57 +++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 7bb53961c0ea..e58a0f1b5c5b 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -307,12 +307,10 @@ static void __veth_xdp_flush(struct veth_rq *rq)
>
> static int veth_xdp_rx(struct veth_rq *rq, struct sk_buff *skb)
> {
> - if (unlikely(ptr_ring_produce(&rq->xdp_ring, skb))) {
> - dev_kfree_skb_any(skb);
> - return NET_RX_DROP;
> - }
> + if (unlikely(ptr_ring_produce(&rq->xdp_ring, skb)))
> + return NETDEV_TX_BUSY; /* signal qdisc layer */
>
> - return NET_RX_SUCCESS;
> + return NET_RX_SUCCESS; /* same as NETDEV_TX_OK */
> }
>
> static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb,
> @@ -346,11 +344,11 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
> struct veth_rq *rq = NULL;
> - int ret = NETDEV_TX_OK;
> + struct netdev_queue *txq;
> struct net_device *rcv;
> int length = skb->len;
> bool use_napi = false;
> - int rxq;
> + int ret, rxq;
>
> rcu_read_lock();
> rcv = rcu_dereference(priv->peer);
> @@ -373,17 +371,45 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
> }
>
> skb_tx_timestamp(skb);
> - if (likely(veth_forward_skb(rcv, skb, rq, use_napi) == NET_RX_SUCCESS)) {
> +
> + ret = veth_forward_skb(rcv, skb, rq, use_napi);
> + switch (ret) {
> + case NET_RX_SUCCESS: /* same as NETDEV_TX_OK */
> if (!use_napi)
> dev_sw_netstats_tx_add(dev, 1, length);
> else
> __veth_xdp_flush(rq);
> - } else {
> + break;
> + case NETDEV_TX_BUSY:
> + /* If a qdisc is attached to our virtual device, returning
> + * NETDEV_TX_BUSY is allowed.
> + */
> + txq = netdev_get_tx_queue(dev, rxq);
> +
> + if (qdisc_txq_has_no_queue(txq)) {
> + dev_kfree_skb_any(skb);
> + goto drop;
> + }
> + /* Restore Eth hdr pulled by dev_forward_skb/eth_type_trans */
> + __skb_push(skb, ETH_HLEN);
> + /* Depend on prior success packets started NAPI consumer via
> + * __veth_xdp_flush(). Cancel TXQ stop if consumer stopped,
> + * paired with empty check in veth_poll().
> + */
> + netif_tx_stop_queue(txq);
> + smp_mb__after_atomic();
> + if (unlikely(__ptr_ring_empty(&rq->xdp_ring)))
> + netif_tx_wake_queue(txq);
> + break;
> + case NET_RX_DROP: /* same as NET_XMIT_DROP */
> drop:
> atomic64_inc(&priv->dropped);
> ret = NET_XMIT_DROP;
> + break;
> + default:
> + net_crit_ratelimited("%s(%s): Invalid return code(%d)",
> + __func__, dev->name, ret);
> }
> -
> rcu_read_unlock();
>
> return ret;
> @@ -874,9 +900,17 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
> struct veth_xdp_tx_bq *bq,
> struct veth_stats *stats)
> {
> + struct veth_priv *priv = netdev_priv(rq->dev);
> + int queue_idx = rq->xdp_rxq.queue_index;
> + struct netdev_queue *peer_txq;
> + struct net_device *peer_dev;
> int i, done = 0, n_xdpf = 0;
> void *xdpf[VETH_XDP_BATCH];
>
> + /* NAPI functions as RCU section */
> + peer_dev = rcu_dereference_check(priv->peer, rcu_read_lock_bh_held());
> + peer_txq = netdev_get_tx_queue(peer_dev, queue_idx);
> +
> for (i = 0; i < budget; i++) {
> void *ptr = __ptr_ring_consume(&rq->xdp_ring);
>
Hi Jesper.
Could you please take a look at the reported call traces and help
understand whether this patch may have introduced a null dereference?
Pasting a snippet, for full logs (2 examples) see the link:
https://lore.kernel.org/bpf/6fd7a5b5-ee26-4cc5-8eb0-449c4e326ccc@linux.dev/
[ 343.217465] BUG: kernel NULL pointer dereference, address:
0000000000000018
[ 343.218173] #PF: supervisor read access in kernel mode
[ 343.218644] #PF: error_code(0x0000) - not-present page
[ 343.219128] PGD 0 P4D 0
[ 343.219379] Oops: Oops: 0000 [#1] SMP NOPTI
[ 343.219768] CPU: 1 UID: 0 PID: 7635 Comm: kworker/1:11 Tainted: G
W OE 6.15.0-g2b36f2252b0a-dirty #7 PREEMPT(full)
[ 343.220844] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 343.221436] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX,
1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 343.222356] Workqueue: mld mld_dad_work
[ 343.222730] RIP: 0010:veth_xdp_rcv.constprop.0+0x6b/0x380
[...]
[ 343.231061] Call Trace:
[ 343.231306] <IRQ>
[ 343.231522] veth_poll+0x7b/0x3a0
[ 343.231856] __napi_poll.constprop.0+0x28/0x1d0
[ 343.232297] net_rx_action+0x199/0x350
[ 343.232682] handle_softirqs+0xd3/0x400
[ 343.233057] ? __dev_queue_xmit+0x27b/0x1250
[ 343.233473] do_softirq+0x43/0x90
[ 343.233804] </IRQ>
[ 343.234016] <TASK>
[ 343.234226] __local_bh_enable_ip+0xb5/0xd0
[ 343.234622] ? __dev_queue_xmit+0x27b/0x1250
[ 343.235035] __dev_queue_xmit+0x290/0x1250
[ 343.235431] ? lock_acquire+0xbe/0x2c0
[ 343.235797] ? ip6_finish_output+0x25e/0x540
[ 343.236210] ? mark_held_locks+0x40/0x70
[ 343.236583] ip6_finish_output2+0x38f/0xb80
[ 343.237002] ? lock_release+0xc6/0x290
[ 343.237364] ip6_finish_output+0x25e/0x540
[ 343.237761] mld_sendpack+0x1c1/0x3a0
[ 343.238123] mld_dad_work+0x3e/0x150
[ 343.238473] process_one_work+0x1f8/0x580
[ 343.238859] worker_thread+0x1ce/0x3c0
[ 343.239224] ? __pfx_worker_thread+0x10/0x10
[ 343.239638] kthread+0x128/0x250
[ 343.239954] ? __pfx_kthread+0x10/0x10
[ 343.240320] ? __pfx_kthread+0x10/0x10
[ 343.240691] ret_from_fork+0x15c/0x1b0
[ 343.241056] ? __pfx_kthread+0x10/0x10
[ 343.241418] ret_from_fork_asm+0x1a/0x30
[ 343.241800] </TASK>
[ 343.242021] Modules linked in: bpf_testmod(OE) [last unloaded:
est_no_cfi(OE)]
[ 343.242737] CR2: 0000000000000018
[ 343.243064] ---[ end trace 0000000000000000 ]---
Thank you.
> @@ -925,6 +959,9 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
> rq->stats.vs.xdp_packets += done;
> u64_stats_update_end(&rq->stats.syncp);
>
> + if (unlikely(netif_tx_queue_stopped(peer_txq)))
> + netif_tx_wake_queue(peer_txq);
> +
> return done;
> }
>
>
>
Powered by blists - more mailing lists