[PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jamal Hadi Salim]

pfifo_fast_init() and pfifo_fast_change_tx_queue_len() allocate skb
ring arrays sized by dev->tx_queue_len with GFP_KERNEL and no upper
bound. An unprivileged user (via unshare -Urn) can set a huge
tx_queue_len and attach many pfifo_fast qdiscs to exhaust global
memory, causing a system-wide OOM.

Cap the ring size to 65535 (default tx_queue_len is 1000) and use
GFP_KERNEL_ACCOUNT so the allocations are charged to the allocating
process's memory cgroup. A pr_warn_ratelimited is emitted when the
ring size is capped so the user is not silently clamped.

Conditions to recreate the bug:
- CONFIG_NET_SCHED=y, CONFIG_VETH=y, CONFIG_USER_NS=y, CONFIG_NET_NS=y.
- Unprivileged user in a fresh user+net namespace (unshare -Urn).
- Create a veth pair, set tx_queue_len to a huge value (e.g. 500000)
  while the devices are down.
- Attach mq at root, then replace each child queue with pfifo_fast:
    tc qdisc replace dev veth0 root handle 1: mq
    tc qdisc replace dev veth0 parent 1:1 pfifo_fast
    tc qdisc replace dev veth0 parent 1:2 pfifo_fast ...
- Repeat across many veth pairs. Each pfifo_fast allocates 3 skb_array
  rings of tx_queue_len entries (~12MB per qdisc at QLEN=500000).
- On the unfixed kernel this exhausts global memory in ~28 iterations
  on a 2GB guest -> OOM panic. On the fixed kernel the ring is capped
  to 65535 entries (~1.5MB per qdisc) and charged to the memcg.

Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
Reported-by: vega@nebusec.ai
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_generic.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ef2b4bf51564..e98fd236afa0 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -910,11 +910,17 @@ static int pfifo_fast_init(struct Qdisc *qdisc, struct nlattr *opt,
 	if (!qlen)
 		return -EINVAL;
 
+	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;
+	}
+
 	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
 		struct skb_array *q = band2list(priv, prio);
 		int err;
 
-		err = skb_array_init(q, qlen, GFP_KERNEL);
+		err = skb_array_init(q, qlen, GFP_KERNEL_ACCOUNT);
 		if (err)
 			return -ENOMEM;
 	}
@@ -957,8 +963,14 @@ static int pfifo_fast_change_tx_queue_len(struct Qdisc *sch,
 		bands[prio] = q;
 	}
 
+	if (new_len > 65535) {
+		pr_warn_ratelimited("pfifo_fast: capping ring size %u to 65535 for dev %s\n",
+				    new_len, qdisc_dev(sch)->name);
+		new_len = 65535;
+	}
+
 	return skb_array_resize_multiple_bh(bands, PFIFO_FAST_BANDS, new_len,
-					    GFP_KERNEL);
+					    GFP_KERNEL_ACCOUNT);
 }
 
 struct Qdisc_ops pfifo_fast_ops __read_mostly = {
-- 
2.43.0

[PATCH net 2/2] selftests: tc-testing: add pfifo_fast ring cap regression test

[Jamal Hadi Salim]

Add a tdc test case (0249) that sets a huge tx_queue_len (100000000)
on a dummy device and attaches pfifo_fast. On the fixed kernel the
ring is internally capped to 65535 entries, so the qdisc is created
successfully (exit 0). On the unfixed kernel the 100M-entry ring
allocation (~2.4GB) fails, causing the qdisc add to fail (exit 2).

Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 .../tc-tests/qdiscs/pfifo_fast.json           | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/pfifo_fast.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/pfifo_fast.json
index 30da27fe8806..2cbe9738d699 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/pfifo_fast.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/pfifo_fast.json
@@ -105,5 +105,28 @@
         "teardown": [
             "$TC qdisc del dev $DUMMY handle 1: root"
         ]
+    },
+    {
+        "id": "0249",
+        "name": "Create pfifo_fast with huge tx_queue_len (ring cap regression)",
+        "category": [
+            "qdisc",
+            "pfifo_fast"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY txqueuelen 100000000"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root pfifo_fast",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc pfifo_fast 1: root",
+        "matchCount": "1",
+        "teardown": [
+            "$IP link set dev $DUMMY txqueuelen 1000",
+            "$TC qdisc del dev $DUMMY handle 1: root"
+        ]
     }
 ]
-- 
2.43.0

Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jakub Kicinski]

On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:
> +	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? We do that all over the place, once in a release we
pick a value too small and someone quickly sends us a fix up to bump
the number. No problem.

Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jamal Hadi Salim]

On Sat, Aug 22, 2026 at 4:44 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:
> > +     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?

cheers,
jamal

> We do that all over the place, once in a release we
> pick a value too small and someone quickly sends us a fix up to bump
> the number. No problem.

Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jakub Kicinski]

On Sat, 22 Aug 2026 18:14:49 -0400 Jamal Hadi Salim wrote:
> > On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:  
> > > +     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?

Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jamal Hadi Salim]

On Sat, Aug 22, 2026 at 7:02 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 22 Aug 2026 18:14:49 -0400 Jamal Hadi Salim wrote:
> > > On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:
> > > > +     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.

cheers,
jamal

Re: [PATCH net 1/2] net/sched: pfifo_fast: cap ring size and account to memcg

[Jamal Hadi Salim]

On Mon, Aug 24, 2026 at 8:37 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sat, Aug 22, 2026 at 7:02 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Sat, 22 Aug 2026 18:14:49 -0400 Jamal Hadi Salim wrote:
> > > > On Thu, 20 Aug 2026 05:57:41 -0400 Jamal Hadi Salim wrote:
> > > > > +     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