* [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
@ 2023-04-17 11:02 Davide Caratti
2023-04-17 11:16 ` Eric Dumazet
2023-04-18 2:30 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Davide Caratti @ 2023-04-17 11:02 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Eric Dumazet, Christoph Paasch, netdev
if sch_fq is configured with "initial quantum" having values greater than
INT_MAX, the first assignment of "credit" does signed integer overflow to
a very negative value.
In this situation, the syzkaller script provided by Cristoph triggers the
CPU soft-lockup warning even with few sockets. It's not an infinite loop,
but "credit" wasn't probably meant to be minus 2Gb for each new flow.
Capping "initial quantum" to INT_MAX proved to fix the issue.
Reported-by: Christoph Paasch <cpaasch@apple.com>
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/377
Fixes: afe4fd062416 ("pkt_sched: fq: Fair Queue packet scheduler")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/sch_fq.c | 12 ++++++++--
.../tc-testing/tc-tests/qdiscs/fq.json | 22 +++++++++++++++++++
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 48d14fb90ba0..12efbcfc2938 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -842,8 +842,16 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
}
}
- if (tb[TCA_FQ_INITIAL_QUANTUM])
- q->initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
+ if (tb[TCA_FQ_INITIAL_QUANTUM]) {
+ u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
+
+ if (initial_quantum <= INT_MAX) {
+ q->initial_quantum = initial_quantum;
+ } else {
+ NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
+ err = -EINVAL;
+ }
+ }
if (tb[TCA_FQ_FLOW_DEFAULT_RATE])
pr_warn_ratelimited("sch_fq: defrate %u ignored.\n",
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
index 8acb904d1419..3593fb8f79ad 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
@@ -114,6 +114,28 @@
"$IP link del dev $DUMMY type dummy"
]
},
+ {
+ "id": "10f7",
+ "name": "Create FQ with invalid initial_quantum setting",
+ "category": [
+ "qdisc",
+ "fq"
+ ],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "setup": [
+ "$IP link add dev $DUMMY type dummy || /bin/true"
+ ],
+ "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq initial_quantum 0x80000000",
+ "expExitCode": "2",
+ "verifyCmd": "$TC qdisc show dev $DUMMY",
+ "matchPattern": "qdisc fq 1: root.*initial_quantum 2048Mb",
+ "matchCount": "0",
+ "teardown": [
+ "$IP link del dev $DUMMY type dummy"
+ ]
+ },
{
"id": "9398",
"name": "Create FQ with maxrate setting",
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
2023-04-17 11:02 [PATCH net] net/sched: sch_fq: fix integer overflow of "credit" Davide Caratti
@ 2023-04-17 11:16 ` Eric Dumazet
2023-04-18 2:30 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2023-04-17 11:16 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Christoph Paasch, netdev
On Mon, Apr 17, 2023 at 1:02 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> if sch_fq is configured with "initial quantum" having values greater than
> INT_MAX, the first assignment of "credit" does signed integer overflow to
> a very negative value.
> In this situation, the syzkaller script provided by Cristoph triggers the
> CPU soft-lockup warning even with few sockets. It's not an infinite loop,
> but "credit" wasn't probably meant to be minus 2Gb for each new flow.
> Capping "initial quantum" to INT_MAX proved to fix the issue.
>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/377
> Fixes: afe4fd062416 ("pkt_sched: fq: Fair Queue packet scheduler")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
Reviewed-by: Eric Dumazet <edumazet@google.com>
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
2023-04-17 11:02 [PATCH net] net/sched: sch_fq: fix integer overflow of "credit" Davide Caratti
2023-04-17 11:16 ` Eric Dumazet
@ 2023-04-18 2:30 ` Jakub Kicinski
2023-04-18 10:26 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-04-18 2:30 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Eric Dumazet,
Christoph Paasch, netdev
On Mon, 17 Apr 2023 13:02:40 +0200 Davide Caratti wrote:
> + u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
> +
> + if (initial_quantum <= INT_MAX) {
> + q->initial_quantum = initial_quantum;
> + } else {
> + NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
> + err = -EINVAL;
> + }
Please set the right policy in fq_policy[] instead.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
2023-04-18 2:30 ` Jakub Kicinski
@ 2023-04-18 10:26 ` Eric Dumazet
2023-04-18 14:43 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2023-04-18 10:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Davide Caratti, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Christoph Paasch, netdev
On Tue, Apr 18, 2023 at 4:30 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 17 Apr 2023 13:02:40 +0200 Davide Caratti wrote:
> > + u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
> > +
> > + if (initial_quantum <= INT_MAX) {
> > + q->initial_quantum = initial_quantum;
> > + } else {
> > + NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
> > + err = -EINVAL;
> > + }
>
> Please set the right policy in fq_policy[] instead.
Not sure these policies are available for old kernels (sch_fq was
added in linux-3.12) ?
Or have we backported all this infra already on stable kernels ?
commit d06a09b94c618c96ced584dd4611a888c8856b8d
Author: Johannes Berg <johannes.berg@intel.com>
Date: Thu Apr 30 22:13:08 2020 +0200
netlink: extend policy range validation
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
2023-04-18 10:26 ` Eric Dumazet
@ 2023-04-18 14:43 ` Jakub Kicinski
2023-04-18 15:03 ` Davide Caratti
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-04-18 14:43 UTC (permalink / raw)
To: Eric Dumazet, Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Christoph Paasch, netdev
On Tue, 18 Apr 2023 12:26:13 +0200 Eric Dumazet wrote:
> > Please set the right policy in fq_policy[] instead.
>
> Not sure these policies are available for old kernels (sch_fq was
> added in linux-3.12) ?
>
> Or have we backported all this infra already on stable kernels ?
>
> commit d06a09b94c618c96ced584dd4611a888c8856b8d
> Author: Johannes Berg <johannes.berg@intel.com>
> Date: Thu Apr 30 22:13:08 2020 +0200
>
> netlink: extend policy range validation
Good point, Davide, once the policy based fix hits the trees please
send this version of the fix to stable@ for 5.4 and older stable trees.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"
2023-04-18 14:43 ` Jakub Kicinski
@ 2023-04-18 15:03 ` Davide Caratti
0 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2023-04-18 15:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Eric Dumazet, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Christoph Paasch, netdev
hello,
On Tue, Apr 18, 2023 at 4:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 18 Apr 2023 12:26:13 +0200 Eric Dumazet wrote:
> > > Please set the right policy in fq_policy[] instead.
> >
> > Not sure these policies are available for old kernels (sch_fq was
> > added in linux-3.12) ?
[...]
> Good point, Davide, once the policy based fix hits the trees please
> send this version of the fix to stable@ for 5.4 and older stable trees.
Sure, I will do that. And thanks for the review! :)
--
davide
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-18 15:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-17 11:02 [PATCH net] net/sched: sch_fq: fix integer overflow of "credit" Davide Caratti
2023-04-17 11:16 ` Eric Dumazet
2023-04-18 2:30 ` Jakub Kicinski
2023-04-18 10:26 ` Eric Dumazet
2023-04-18 14:43 ` Jakub Kicinski
2023-04-18 15:03 ` Davide Caratti
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.