Thread (7 messages) flat view 7 messages, 2 authors, 7h ago

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help