Re: [PATCH net v2 1/2] net/sched: pfifo_fast: reject oversized ring and account to memcg

[Paolo Abeni <pabeni@redhat.com>]

On 8/27/26 7:39 PM, Jamal Hadi Salim wrote:
> On Thu, Aug 27, 2026 at 7:26 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 8/25/26 10:17 AM, Jamal Hadi Salim wrote:
>>> 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.
>>>
>>> Reject tx_queue_len values exceeding S16_MAX (32767) with -ERANGE
>>> in both pfifo_fast_init() and pfifo_fast_change_tx_queue_len().
>>> Note: For the init path, NL_SET_ERR_MSG_FMT_MOD reports the error
>>> via extack whereas for the resize path, the error propagates to
>>> netif_change_tx_queue_len() which rolls back dev->tx_queue_len to
>>> the original value. Use GFP_KERNEL_ACCOUNT so the ring allocations
>>> are charged to the allocating process's memory cgroup.
>>>
>>> S16_MAX is the virtio virtqueue size limit: the virtio specification
>>> stores the queue size as a u16 with a maximum of 32768, so 32767 is
>>> the largest tx_queue_len any in-tree driver can meaningfully use.
>>>
>>> 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 oversized
>>>   tx_queue_len is rejected with -ERANGE.
>>>
>>> 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>
>>> ---
>>> v1 -> v2:
>>> - Replaced silent clamp + pr_warn_ratelimited with reject (-ERANGE) (Jakub)
>>> - Changed cap from 65535 to S16_MAX (32767), matching virtio's
>>>   virtio16 ring size limit.
>>> - Dropped the doubled module prefix in extack (NL_SET_ERR_MSG_FMT_MOD
>>>   already prepends KBUILD_MODNAME).
>>> - Added resize-path tdc test case (Sashiko nipa gpt-5-6-sol-1-2).
>>> - Fixed tdc teardown to use JSON list form for acceptable exit codes.
>>> ---
>>>  net/sched/sch_generic.c | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>>> index ef2b4bf51564..eb5c0d3f67c2 100644
>>> --- a/net/sched/sch_generic.c
>>> +++ b/net/sched/sch_generic.c
>>> @@ -910,11 +910,18 @@ static int pfifo_fast_init(struct Qdisc *qdisc, struct nlattr *opt,
>>>       if (!qlen)
>>>               return -EINVAL;
>>>
>>> +     if (qlen > S16_MAX) {
>>> +             NL_SET_ERR_MSG_FMT_MOD(extack,
>>> +                                    "ring size %u too large (max %d)",
>>> +                                    qlen, S16_MAX);
>>> +             return -ERANGE;
>>
>> Sashiko noted that setting a large tx_queue_len to a down
>> interface gives an inconsistent behavior:
>>
>> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260825081751.134086-1-jhs%40mojatatu.com
>>
>> (all other comments are IMHO noise and should be ignored)
>>
>> I don't see and effective way to avoid that, short of falling
>> back to v1, WDYT?
> 
> I changed my mind on this. See this thread and let me know what you think:
> https://lore.kernel.org/netdev/CAM0EoMk8fAACt11fe1pUsFeG9np3SdsV0YUNmEevLu=i=b+xKg@mail.gmail.com/
Whoops, I missed the last message in such a thread. I think it makes
sense and will cover all the sashiko-reported concerns. You can probably
also additionally get rid of the old test:

	 if (new_len != (unsigned int)new_len)
                return -ERANGE;

/P