| Message ID | 7b3a3c7e36d03068707a021760a194a8eb5ad41a.1682002300.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7041101ff6c3073fd8f2e99920f535b111c929cb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net/sched: sch_fq: fix integer overflow of "credit" | expand |
On 20/04/2023 11:59, Davide Caratti 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. > > v2: validation of "initial quantum" is done in fq_policy, instead of open > coding in fq_change() _ suggested by Jakub Kicinski > > 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") > Reviewed-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/sched/sch_fq.c | 6 ++++- > .../tc-testing/tc-tests/qdiscs/fq.json | 22 +++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c > index 48d14fb90ba0..f59a2cb2c803 100644 > --- a/net/sched/sch_fq.c > +++ b/net/sched/sch_fq.c > @@ -779,13 +779,17 @@ static int fq_resize(struct Qdisc *sch, u32 log) > return 0; > } > > +static struct netlink_range_validation iq_range = { > + .max = INT_MAX, > +}; > + > static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = { > [TCA_FQ_UNSPEC] = { .strict_start_type = TCA_FQ_TIMER_SLACK }, > > [TCA_FQ_PLIMIT] = { .type = NLA_U32 }, > [TCA_FQ_FLOW_PLIMIT] = { .type = NLA_U32 }, > [TCA_FQ_QUANTUM] = { .type = NLA_U32 }, > - [TCA_FQ_INITIAL_QUANTUM] = { .type = NLA_U32 }, > + [TCA_FQ_INITIAL_QUANTUM] = NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range), > [TCA_FQ_RATE_ENABLE] = { .type = NLA_U32 }, > [TCA_FQ_FLOW_DEFAULT_RATE] = { .type = NLA_U32 }, > [TCA_FQ_FLOW_MAX_RATE] = { .type = NLA_U32 }, > 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", You probably don't want to backport the test as well? If so I would break this patch into two.
On Thu, 20 Apr 2023 13:25:33 -0300 Pedro Tammela wrote: > > "id": "9398", > > "name": "Create FQ with maxrate setting", > > You probably don't want to backport the test as well? If so I would > break this patch into two. IIRC the preference of stable folks is to backport more not fewer selftests. Practicality of that aside, I think the patch is good as is.
On 20/04/2023 20:24, Jakub Kicinski wrote: > On Thu, 20 Apr 2023 13:25:33 -0300 Pedro Tammela wrote: >>> "id": "9398", >>> "name": "Create FQ with maxrate setting", >> >> You probably don't want to backport the test as well? If so I would >> break this patch into two. > > IIRC the preference of stable folks is to backport more not fewer > selftests. Practicality of that aside, I think the patch is good as is. My concern here was mostly due to conflicts with the tdc code base. Davide explained to me privately that he will take care of this so it's all good.
hi, On Fri, Apr 21, 2023 at 4:24 PM Pedro Tammela <pctammela@mojatatu.com> wrote: > > On 20/04/2023 20:24, Jakub Kicinski wrote: [...] > > IIRC the preference of stable folks is to backport more not fewer > > selftests. Practicality of that aside, I think the patch is good as is. > > My concern here was mostly due to conflicts with the tdc code base. > Davide explained to me privately that he will take care of this so it's > all good. right, and I should color this reply-to-all button in a way that catches better my attention :) the tdc code will be a merge conflict on kernels that don't have [1] (v6.0 and v5.x), so I will skip the tdc testcase for the stable backport (unless somebody has objections). thanks,
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 20 Apr 2023 16:59:46 +0200 you 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. > > [...] Here is the summary with links: - [net,v2] net/sched: sch_fq: fix integer overflow of "credit" https://git.kernel.org/netdev/net/c/7041101ff6c3 You are awesome, thank you!
diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c index 48d14fb90ba0..f59a2cb2c803 100644 --- a/net/sched/sch_fq.c +++ b/net/sched/sch_fq.c @@ -779,13 +779,17 @@ static int fq_resize(struct Qdisc *sch, u32 log) return 0; } +static struct netlink_range_validation iq_range = { + .max = INT_MAX, +}; + static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = { [TCA_FQ_UNSPEC] = { .strict_start_type = TCA_FQ_TIMER_SLACK }, [TCA_FQ_PLIMIT] = { .type = NLA_U32 }, [TCA_FQ_FLOW_PLIMIT] = { .type = NLA_U32 }, [TCA_FQ_QUANTUM] = { .type = NLA_U32 }, - [TCA_FQ_INITIAL_QUANTUM] = { .type = NLA_U32 }, + [TCA_FQ_INITIAL_QUANTUM] = NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range), [TCA_FQ_RATE_ENABLE] = { .type = NLA_U32 }, [TCA_FQ_FLOW_DEFAULT_RATE] = { .type = NLA_U32 }, [TCA_FQ_FLOW_MAX_RATE] = { .type = NLA_U32 }, 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",
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. v2: validation of "initial quantum" is done in fq_policy, instead of open coding in fq_change() _ suggested by Jakub Kicinski 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") Reviewed-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/sched/sch_fq.c | 6 ++++- .../tc-testing/tc-tests/qdiscs/fq.json | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-)