[v1] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic

[PATCH] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic

Posted by Jack Wang 1 month, 3 weeks ago

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.

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 <163wangjack@gmail.com>
---
 net/sched/sch_cbs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index 1c93469c56e3..9ef31ecdaa05 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",
@@ -405,8 +405,8 @@ static int cbs_change(struct Qdisc *sch, struct nlattr *opt,
 	/* Everything went OK, save the parameters used. */
 	WRITE_ONCE(q->hicredit, qopt->hicredit);
 	WRITE_ONCE(q->locredit, qopt->locredit);
-	WRITE_ONCE(q->idleslope, qopt->idleslope * BYTES_PER_KBIT);
-	WRITE_ONCE(q->sendslope, qopt->sendslope * BYTES_PER_KBIT);
+	WRITE_ONCE(q->idleslope, (s64)qopt->idleslope * BYTES_PER_KBIT);
+	WRITE_ONCE(q->sendslope, (s64)qopt->sendslope * BYTES_PER_KBIT);
 	WRITE_ONCE(q->offload, qopt->offload);
 
 	return 0;
-- 
2.53.0

Re: [PATCH] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic

Posted by Jamal Hadi Salim 1 month, 3 weeks ago

On Thu, Aug 6, 2026 at 11:53 AM Jack Wang <163wangjack@gmail.com> 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.
>

Check what the sashikos are saying please  - i agree with their view.
I believe we need a v2:
I'd suggest a v2 that addresses both (and optionally validates the
idleslope sign and a negative idleslope can arm the watchdog in the
past and busy-loop).

For completeness, the pre-existing timediff_to_credits() s64 overflow
that gemini sashiko flagged is a separate follow-up, not part of this
patch.

cheers,
jamal

> 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 <163wangjack@gmail.com>
> ---
>  net/sched/sch_cbs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
> index 1c93469c56e3..9ef31ecdaa05 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",
> @@ -405,8 +405,8 @@ static int cbs_change(struct Qdisc *sch, struct nlattr *opt,
>         /* Everything went OK, save the parameters used. */
>         WRITE_ONCE(q->hicredit, qopt->hicredit);
>         WRITE_ONCE(q->locredit, qopt->locredit);
> -       WRITE_ONCE(q->idleslope, qopt->idleslope * BYTES_PER_KBIT);
> -       WRITE_ONCE(q->sendslope, qopt->sendslope * BYTES_PER_KBIT);
> +       WRITE_ONCE(q->idleslope, (s64)qopt->idleslope * BYTES_PER_KBIT);
> +       WRITE_ONCE(q->sendslope, (s64)qopt->sendslope * BYTES_PER_KBIT);
>         WRITE_ONCE(q->offload, qopt->offload);
>
>         return 0;
> --
> 2.53.0
>

Re: [PATCH] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic

Posted by Vinicius Costa Gomes 1 month, 3 weeks ago

Jack Wang <163wangjack@gmail.com> writes:

> 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.
>
> 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 <163wangjack@gmail.com>
> ---

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius

[PATCH net v2] net/sched: cbs: perform rate conversions in signed 64-bit arithmetic

Posted by Jack Wang 1 month, 2 weeks ago

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 non-positive 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 <163wangjack@gmail.com>
---
v2:
  - Reject a non-positive idleslope to prevent scheduling the watchdog in
    the past.
  - Leave the pre-existing timediff_to_credits() overflow for a separate
    follow-up.

  v1: https://lore.kernel.org/netdev/20260806155253.50252-1-163wangjack@gmail.com/

 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..e960c8e01ef4 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 be greater than zero");
+		return -EINVAL;
+	}
 
 	if (!qopt->offload) {
 		cbs_set_port_rate(dev, q);
@@ -405,8 +409,8 @@ static int cbs_change(struct Qdisc *sch, struct nlattr *opt,
 	/* Everything went OK, save the parameters used. */
 	WRITE_ONCE(q->hicredit, qopt->hicredit);
 	WRITE_ONCE(q->locredit, qopt->locredit);
-	WRITE_ONCE(q->idleslope, qopt->idleslope * BYTES_PER_KBIT);
-	WRITE_ONCE(q->sendslope, qopt->sendslope * BYTES_PER_KBIT);
+	WRITE_ONCE(q->idleslope, (s64)qopt->idleslope * BYTES_PER_KBIT);
+	WRITE_ONCE(q->sendslope, (s64)qopt->sendslope * BYTES_PER_KBIT);
 	WRITE_ONCE(q->offload, qopt->offload);
 
 	return 0;

base-commit: 7b53449540502cb21b32bca62a6258e22cd97bbe
-- 
2.53.0