Re: [PATCH net-next v3 3/4] net/sched/sch_cake: widen buffer_used to u64

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> This is a follow-up to commit 4c660ee8c809 ("net: sched: fix 32-bit
> backlog wrap in gred, bfifo and plug enqueue"), which promoted the
> backlog + length admission sums in gred, bfifo and plug to u64.  CAKE's
> own memory accounting has the same 32-bit wrap.
>
> The issue:
> cake_sched_data.buffer_used accumulates skb->truesize and is compared
> against memory_limit:
>
> 	q->buffer_used += skb->truesize;
> 	if (q->buffer_used <= q->buffer_limit)
> 		return NET_XMIT_SUCCESS;
> 	while (q->buffer_used > q->buffer_limit)
> 		drop...
>
> buffer_used is u32, so once the resident truesize passes 2^32 it wraps to
> a small value, the <= test passes and the drop loop is skipped even
> though far more than the configured limit is queued.  memory_limit is a
> u32 attribute (TCA_CAKE_MEMORY), so buffer_limit can be as large as
> U32_MAX and never caps buffer_used below the wrap point; with a large
> memory_limit the queue then grows until the machine is out of memory.
>
> The fix:
> Widen buffer_used and buffer_max_used to u64, the same shape as the
> gred/bfifo/plug fix.  buffer_limit stays u32; the comparison promotes it
> to u64, so the bounded queue stops at the configured limit below 2^32 and
> the counter can no longer wrap.  buffer_used is subtracted on dequeue and
> drop, so the widened type flows through unchanged.  The ACK-filter
> replacement applied the net delta as one expression,
> q->buffer_used += skb->truesize - ack->truesize.  Both operands are
> unsigned int, so the subtraction is evaluated at u32 and a larger removed
> ACK wraps the delta; the old u32 accumulator folded that back to the
> correct net value, but the widened u64 accumulator would persist it as a
> roughly 4 GiB over-count.  Apply the replacement as two exact operations
> on the widened counter instead; the queue already accounts ack->truesize,
> so the subtraction cannot underflow.
>
> Widening the counters alone is incomplete because cake_dump_stats() still
> exports buffer_max_used through PUT_STAT_U32(MEMORY_USED, ...), and
> tc.yaml declares memory-used as u32.  buffer_max_used only ever rises, so
> once the widened counter passes 2^32 the export reports peak - 2^32 for
> the rest of the qdisc's life.  Clamp the exported value to U32_MAX; the
> counter itself is unaffected.
>
> The dump path runs without the qdisc lock: tc_fill_qdisc() passes NULL to
> gnet_stats_start_copy_compat(), so the reader races with the enqueue-side
> update.  READ_ONCE() is not an atomic 64-bit load on 32-bit CPUs, so a
> reader can observe a torn high-water mark and report a bogus MEMORY_USED.
> Publish the counter as u64_stats_t under the qdisc lock, which the enqueue
> path already holds, and take a seqcount-protected snapshot in the dump.
> The counter stays u64; only the exported value is bounded.
>
> Conditions to recreate the bug:
> CAP_NET_ADMIN (root or a user namespace, the qdisc attach is gated by
> netlink_net_capable(CAP_NET_ADMIN)).
>
> A TUN device is needed so the root qdisc can park packets instead of
> dropping them.  Open /dev/net/tun, request TUNSETIFF with
> IFF_TUN | IFF_NO_PI | IFF_BACKPRESSURE and keep the fd open without
> reading it (run it in the background and stop it at teardown): with
> IFF_BACKPRESSURE the full tx ring calls netif_tx_stop_queue() rather than
> dropping, so the queue behind the qdisc can grow past 4 GiB of resident
> truesize.  A plain 'ip tuntap add' attaches no fd and does not request
> IFF_BACKPRESSURE, so it cannot park the queue this way.
>
>   ip link set tun0 txqueuelen 32 up
>   ip addr add 10.99.0.1/24 dev tun0
>   tc qdisc add dev tun0 root handle 1: cake memlimit 4294967295
>
>   # Then push more than 4 GiB of resident truesize through the qdisc: six
>   # UDP senders using large SO_SNDBUFFORCE buffers send 1400-byte
>   # datagrams (the reported OOM witness used 6 workers for 100 s on a
>   # 12 GiB guest).  On the unfixed kernel buffer_used wraps, the admission
>   # test passes, the drop loop is bypassed and the guest runs out of
>   # memory; with the fix the counter passes the limit without wrapping and
>   # the excess is dropped.
>
>   # The ACK-filter over-count is a separate corner: it needs ack-filter
>   # enabled and a queued pure ACK whose truesize is larger than the
>   # replacement ACK.  Use a fresh interface (or 'tc qdisc change') rather
>   # than a second root 'tc qdisc add' on an interface that already has a
>   # qdisc:
>   tc qdisc change dev tun0 root cake ack-filter
>
> CONFIG_NET_SCH_CAKE=y and CONFIG_TUN=y are required, and the
> memory-residency recipe needs a root-run sender envelope with enough
> guest RAM to hold the queued data.
>
> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/20260818095927.15901-1-jhs@mojatatu.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>