Re: [PATCH net v3] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic
From: Paolo Abeni <pabeni@redhat.com>
Date: 2026-08-18 09:41:38
Also in:
lkml
On 8/13/26 8:02 AM, Jack Wang wrote:
cbs_set_port_rate() and cbs_change() multiply link rates and slope values
by BYTES_PER_KBIT, an unsigned long constant. On 32-bit architectures,
the multiplications therefore take place in 32-bit unsigned arithmetic
before the results are assigned to s64 fields.
For port rates above approximately 34.36 Gbit/s this wraps port_rate.
The same conversion turns a negative sendslope into a large positive
value, reversing the CBS credit adjustment. This affects software CBS;
port_rate is also refreshed on NETDEV_UP and NETDEV_CHANGE notifications.
Cast the first operand of each multiplication to s64 so all intermediate
operations use signed 64-bit arithmetic and preserve the value's sign on
every architecture
Also reject a negative idleslope. A negative idleslope can arm the
watchdog in the past and busy-loop.
Fixes: 585d763af09c ("net/sched: Introduce Credit Based Shaper (CBS) qdisc")
Fixes: 397006ba5d918 ("net/sched: cbs: Fix integer overflow in cbs_set_port_rate()")
Signed-off-by: Jack Wang <redacted>No empty lines in the tag area.
quoted hunk ↗ jump to hunk
--- v3: - Reject only negative idleslope values. - Keep idleslope 0 accepted for compatibility with existing tc-testing defaults, fixing test 1820 regression reported by Victor. - Keep the timediff_to_credits() overflow out of this series as a separate follow-up. v2: - Reject non-positive idleslope values to prevent scheduling the watchdog in the past. v1: https://lore.kernel.org/netdev/20260806155253.50252-1-163wangjack@gmail.com/ (local) net/sched/sch_cbs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c index 1c93469c56e3..8db98d7c98a8 100644 --- a/net/sched/sch_cbs.c +++ b/net/sched/sch_cbs.c@@ -335,7 +335,7 @@ static void cbs_set_port_rate(struct net_device *dev, struct cbs_sched_data *q) speed = ecmd.base.speed; skip: - port_rate = speed * 1000 * BYTES_PER_KBIT; + port_rate = (s64)speed * 1000 * BYTES_PER_KBIT; atomic64_set(&q->port_rate, port_rate); netdev_dbg(dev, "cbs: set %s's port_rate to: %lld, linkspeed: %d\n",@@ -392,6 +392,10 @@ static int cbs_change(struct Qdisc *sch, struct nlattr *opt, } qopt = nla_data(tb[TCA_CBS_PARMS]); + if (qopt->idleslope < 0) { + NL_SET_ERR_MSG(extack, "Idleslope must not be negative");
Sashiko noted this isn't enough to avoid the busy loop timer: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260813060256.28748-1-163wangjack%40gmail.com I understand that a 0 value must be accepted, but I think we need either a better solution or drop entirely this check that does not provide much value per se. I think the first option would be better. Also please note saskiko has more comments. /P