Re: [PATCH net] net/sched: clamp quantum and psched_mtu in change paths and missed siblings

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

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

> This is a followup to commit 709f34f7c28d ("net/sched: fq: add
> overflow bounds to quantum and initial quantum").
> The quantum_backlog_overflow series and the five siblings that followed
> clamped the init-path quantum to in fq, fq_codel, fq_pie, hhf, sfq.
> The change() paths with the same pattern, same writer of q->quantum,
> same privilege level (CAP_NET_ADMIN in a user namespace) were not clamped.
> A user can override the init clamp via tc qdisc change, restoring the
> small-quantum deficit spin that the init clamp was meant to prevent.
>
> This follow-up also covers two siblings that were missed entirely by
> the original series: sch_dualpi2 and sch_pie call psched_mtu() without
> any clamp at all. With a crafted size table qdisc_pkt_len reaches ~2
> GiB, so quantum=1 (or a zero psched_mtu on a headerless device) makes
> the deficit-refill loop spin ~2^31 times under the qdisc lock (a soft
> lockup / denial of service).
>
> Fixes based on review of 709f34f7c28d:
>
> 1. fq_pie_change() accepts quantum=1 (NLA policy fq_pie_q_range.min=1).
>    Add max(256U, ...) matching fq_codel_change()
>    (Sashiko nipa gpt-5-6-sol-3-8 and gpt-5-6-sol-6-7)
>
> 2. sfq_change() accepts any non-negative quantum (only rejects
>    (int)ctl->quantum < 0). Add max(256U, ...) matching fq_codel_change().
>    Reject quantum > 1<<20 with -EINVAL, matching fq_codel_change() and
>    the init clamp.
>    (Internal review noticing same pattern)
>
> 3. hhf_change() accepts quantum=1 (only checks non_hh_quantum product).
>    Add max(256U, ...) matching fq_codel_change()
>    (Sashiko nipa v1 review + vega@nebusec.ai independent bug)
>
> 4. fq_change() accepts TCA_FQ_INITIAL_QUANTUM up to INT_MAX (iq_range.max
>    = INT_MAX) while fq_init() now clamps to 1<<20.  Narrow iq_range.max
>    to 1<<20, rejecting at parse time.  (Eric Dumazet)
>
> 5. sch_dualpi2: dualpi2_calculate_c_protection() and get_memory_limit()
>    call psched_mtu() with no clamp. A huge MTU makes (s32)psched_mtu()
>    overflow in the signed multiply for c_protection_init, and 2 *
>    psched_mtu() wraps in get_memory_limit(). Clamp to [1, 1<<20] at
>    all three call sites. (Sashiko nipa main-6-4)
>
> 6. sch_pie: pie_drop_early() calls psched_mtu() with no clamp.  With
>    mtu=0x80000000 the bytemode divide silently zeroes the drop
>    probability, disabling AQM. Clamp to [1, 1<<20] (Sashiko gemini)
>
> 7. sch_drr: drr_change_class() rejects explicit quantum==0 but falls
>    back to psched_mtu() with no floor. Add max(256U, ...) after the
>    zero reject and on the fallback path
>   (vega@nebusec.ai independent bug)
>
> 8. sch_ets: ets_qdisc_change() falls back to psched_mtu() with no floor
>    for bands without an explicit quantum.  Add max(256U, ...) on the
>    fallback path (vega@nebusec.ai independent bug)
>
> The init paths of fq_pie, sfq, and hhf delegate to their _change() when
> opt is present, so the floor covers tc qdisc add ... quantum 1 as well
> as change. The zero-quantum-from-psched_mtu case on a headerless device
> (mtu==0) is also covered by the 256 floor in drr and ets; the
> explicit-zero reject in drr_change_class() is preserved.
>
> The sfq_change() silent clamp is user-visible: sfq_dump() reports the
> clamped quantum, so a previously accepted quantum < 256 now reads back
> as 256. Idempotent config managers that read back and compare will
> see drift. fq_codel_change() made the same trade, so this is consistent.
>
> Conditions to recreate the bug: create a fq_pie, sfq, hhf, dualpi2, or
> pie qdisc (or a drr class / ets band), then tc qdisc change ... quantum 1
> with a STAB size table inflating qdisc_pkt_len, e.g.:
>
>   tc qdisc add dev dummy0 root fq_pie
>   tc qdisc change dev dummy0 root fq_pie quantum 1 \
>     stab data 32768 size_log 15 cell_log 0
>
> Requires CAP_NET_ADMIN in a user namespace (unshare -Urn).
>
> Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Fixes: e4650d7ae425 ("net_sched: sch_sfq: handle bigger packets")
> Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
> Fixes: 320d031ad6e4 ("sched: Struct definition and parsing of dualpi2 qdisc")
> Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")
> Fixes: 13d2a1d2b032 ("pkt_sched: add DRR scheduler")
> Reported-by: vega@nebusec.ai
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>