| Message ID | a5288a1f4b69eb2da3e704d0e1ff082489432d25.1681728988.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: sch_fq: fix integer overflow of "credit" | expand |
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.
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.
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
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.
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
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",
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(-)