Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: 2026-08-25 16:08:16
Also in:
stable
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Mon, Aug 24, 2026 at 8:37 AM Jamal Hadi Salim [off-list ref] wrote:
On Sat, Aug 22, 2026 at 7:02 PM Jakub Kicinski [off-list ref] wrote:quoted
On Sat, 22 Aug 2026 18:14:49 -0400 Jamal Hadi Salim wrote:quoted
quoted
On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:quoted
+ if (qlen > 65535) { + pr_warn_ratelimited("pfifo_fast: capping ring size %u to 65535 for dev %s\n", + qlen, qdisc_dev(qdisc)->name); + qlen = 65535; + }Why not a hard cap?sorry, where to put the hard cap and what would it be? Or maybe you misspoke and intended to say "why put a hardcap"? Or maybe in this case probably safer to just reject the change if > hard cap?The latter, I'm asking why not: if (qlen > 64k) return -EINVAL; To be clear I didn't dig into the code, just read the commit msg, so maybe you have a reason. But the "normal behavior" for out of range params should be to return an error rather than silently clamp() into the allowed range?No real concreate reason other than the thought that it's a device param as opposed to a pfifo one. I am going to send a v2. Will switch to similar to what virtio does: if (qlen > S16) { //extack here return -E2BIG; } The followup as identified by sashikos is on tun/tap - i have a rough patch, untested but there are others ahead of that i need to send out first.
Although i have submitted v2 - i may have a small change of heart. I was trying to document sashiko's famous "pre-existing issue" rant for this bug so we can followup later; sashiko correctly pointed out that tun, tap/macvtap have the same issue and i was going to do those as followups. Although i had this approach below as an option earlier - i didnt want to go with it because the goal was to isolate the pfifo changes to net/sched. The option was to make this change:
diff --git a/net/core/dev.c b/net/core/dev.c
index 38336858c168..ab6b44b67be2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -9985,6 +9985,9 @@ int netif_change_tx_queue_len(struct net_device*dev, unsigned long new_len)
if (new_len != (unsigned int)new_len)
return -ERANGE;
+ if (new_len > S16_MAX)
+ return -ERANGE;
+
if (new_len != orig_len) {
WRITE_ONCE(dev->tx_queue_len, new_len);
res = call_netdevice_notifiers(NETDEV_CHANGE_TX_QUEUE_LEN, dev);
This is a bit controversial with S16_MAX as the upper bound, although
no device i could find had anything that caps other than virtio (u16
max: 32768).
I have tested this with all 3 issues(pfifo, tun and tap) and it fixes all 3.
If this is acceptable as a fix i am going to withdraw v2
(I dont see much of a downside even for buffer bloat needy folks like
satellite links and given virtio there is precedence)
cheers,
jamal