* [PATCH net v3 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration
2025-04-07 20:24 [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 Octavian Purdila
@ 2025-04-07 20:24 ` Octavian Purdila
2025-04-07 20:24 ` [PATCH net v3 2/3] net_sched: sch_sfq: move the limit validation Octavian Purdila
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Octavian Purdila @ 2025-04-07 20:24 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri
Cc: davem, edumazet, kuba, pabeni, horms, shuah, netdev,
Octavian Purdila
Many configuration parameters have influence on others (e.g. divisor
-> flows -> limit, depth -> limit) and so it is difficult to correctly
do all of the validation before applying the configuration. And if a
validation error is detected late it is difficult to roll back a
partially applied configuration.
To avoid these issues use a temporary work area to update and validate
the configuration and only then apply the configuration to the
internal state.
Signed-off-by: Octavian Purdila <tavip@google•com>
Acked-by: Cong Wang <xiyou.wangcong@gmail•com>
---
net/sched/sch_sfq.c | 56 +++++++++++++++++++++++++++++++++++----------
1 file changed, 44 insertions(+), 12 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 65d5b59da583..7714ae94e052 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -631,6 +631,15 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
struct red_parms *p = NULL;
struct sk_buff *to_free = NULL;
struct sk_buff *tail = NULL;
+ unsigned int maxflows;
+ unsigned int quantum;
+ unsigned int divisor;
+ int perturb_period;
+ u8 headdrop;
+ u8 maxdepth;
+ int limit;
+ u8 flags;
+
if (opt->nla_len < nla_attr_size(sizeof(*ctl)))
return -EINVAL;
@@ -656,36 +665,59 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
NL_SET_ERR_MSG_MOD(extack, "invalid limit");
return -EINVAL;
}
+
sch_tree_lock(sch);
+
+ limit = q->limit;
+ divisor = q->divisor;
+ headdrop = q->headdrop;
+ maxdepth = q->maxdepth;
+ maxflows = q->maxflows;
+ perturb_period = q->perturb_period;
+ quantum = q->quantum;
+ flags = q->flags;
+
+ /* update and validate configuration */
if (ctl->quantum)
- q->quantum = ctl->quantum;
- WRITE_ONCE(q->perturb_period, ctl->perturb_period * HZ);
+ quantum = ctl->quantum;
+ perturb_period = ctl->perturb_period * HZ;
if (ctl->flows)
- q->maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);
+ maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);
if (ctl->divisor) {
- q->divisor = ctl->divisor;
- q->maxflows = min_t(u32, q->maxflows, q->divisor);
+ divisor = ctl->divisor;
+ maxflows = min_t(u32, maxflows, divisor);
}
if (ctl_v1) {
if (ctl_v1->depth)
- q->maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);
+ maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);
if (p) {
- swap(q->red_parms, p);
- red_set_parms(q->red_parms,
+ red_set_parms(p,
ctl_v1->qth_min, ctl_v1->qth_max,
ctl_v1->Wlog,
ctl_v1->Plog, ctl_v1->Scell_log,
NULL,
ctl_v1->max_P);
}
- q->flags = ctl_v1->flags;
- q->headdrop = ctl_v1->headdrop;
+ flags = ctl_v1->flags;
+ headdrop = ctl_v1->headdrop;
}
if (ctl->limit) {
- q->limit = min_t(u32, ctl->limit, q->maxdepth * q->maxflows);
- q->maxflows = min_t(u32, q->maxflows, q->limit);
+ limit = min_t(u32, ctl->limit, maxdepth * maxflows);
+ maxflows = min_t(u32, maxflows, limit);
}
+ /* commit configuration */
+ q->limit = limit;
+ q->divisor = divisor;
+ q->headdrop = headdrop;
+ q->maxdepth = maxdepth;
+ q->maxflows = maxflows;
+ WRITE_ONCE(q->perturb_period, perturb_period);
+ q->quantum = quantum;
+ q->flags = flags;
+ if (p)
+ swap(q->red_parms, p);
+
qlen = sch->q.qlen;
while (sch->q.qlen > q->limit) {
dropped += sfq_drop(sch, &to_free);
--
2.49.0.504.g3bcea36a83-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v3 2/3] net_sched: sch_sfq: move the limit validation
2025-04-07 20:24 [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 Octavian Purdila
2025-04-07 20:24 ` [PATCH net v3 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration Octavian Purdila
@ 2025-04-07 20:24 ` Octavian Purdila
2025-04-07 20:24 ` [PATCH net v3 3/3] selftests/tc-testing: sfq: check that a derived limit of 1 is rejected Octavian Purdila
2025-04-09 12:00 ` [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Octavian Purdila @ 2025-04-07 20:24 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri
Cc: davem, edumazet, kuba, pabeni, horms, shuah, netdev,
Octavian Purdila, syzbot
It is not sufficient to directly validate the limit on the data that
the user passes as it can be updated based on how the other parameters
are changed.
Move the check at the end of the configuration update process to also
catch scenarios where the limit is indirectly updated, for example
with the following configurations:
tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 1 depth 1
tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 1 divisor 1
This fixes the following syzkaller reported crash:
------------[ cut here ]------------
UBSAN: array-index-out-of-bounds in net/sched/sch_sfq.c:203:6
index 65535 is out of range for type 'struct sfq_head[128]'
CPU: 1 UID: 0 PID: 3037 Comm: syz.2.16 Not tainted 6.14.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 12/27/2024
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:94 [inline]
dump_stack_lvl+0x201/0x300 lib/dump_stack.c:120
ubsan_epilogue lib/ubsan.c:231 [inline]
__ubsan_handle_out_of_bounds+0xf5/0x120 lib/ubsan.c:429
sfq_link net/sched/sch_sfq.c:203 [inline]
sfq_dec+0x53c/0x610 net/sched/sch_sfq.c:231
sfq_dequeue+0x34e/0x8c0 net/sched/sch_sfq.c:493
sfq_reset+0x17/0x60 net/sched/sch_sfq.c:518
qdisc_reset+0x12e/0x600 net/sched/sch_generic.c:1035
tbf_reset+0x41/0x110 net/sched/sch_tbf.c:339
qdisc_reset+0x12e/0x600 net/sched/sch_generic.c:1035
dev_reset_queue+0x100/0x1b0 net/sched/sch_generic.c:1311
netdev_for_each_tx_queue include/linux/netdevice.h:2590 [inline]
dev_deactivate_many+0x7e5/0xe70 net/sched/sch_generic.c:1375
Reported-by: syzbot <syzkaller@googlegroups•com>
Fixes: 10685681bafc ("net_sched: sch_sfq: don't allow 1 packet limit")
Signed-off-by: Octavian Purdila <tavip@google•com>
Acked-by: Cong Wang <xiyou.wangcong@gmail•com>
---
net/sched/sch_sfq.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 7714ae94e052..58b42dcf8f20 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -661,10 +661,6 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
if (!p)
return -ENOMEM;
}
- if (ctl->limit == 1) {
- NL_SET_ERR_MSG_MOD(extack, "invalid limit");
- return -EINVAL;
- }
sch_tree_lock(sch);
@@ -705,6 +701,12 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
limit = min_t(u32, ctl->limit, maxdepth * maxflows);
maxflows = min_t(u32, maxflows, limit);
}
+ if (limit == 1) {
+ sch_tree_unlock(sch);
+ kfree(p);
+ NL_SET_ERR_MSG_MOD(extack, "invalid limit");
+ return -EINVAL;
+ }
/* commit configuration */
q->limit = limit;
--
2.49.0.504.g3bcea36a83-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v3 3/3] selftests/tc-testing: sfq: check that a derived limit of 1 is rejected
2025-04-07 20:24 [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 Octavian Purdila
2025-04-07 20:24 ` [PATCH net v3 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration Octavian Purdila
2025-04-07 20:24 ` [PATCH net v3 2/3] net_sched: sch_sfq: move the limit validation Octavian Purdila
@ 2025-04-07 20:24 ` Octavian Purdila
2025-04-09 12:00 ` [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Octavian Purdila @ 2025-04-07 20:24 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri
Cc: davem, edumazet, kuba, pabeni, horms, shuah, netdev,
Octavian Purdila
Because the limit is updated indirectly when other parameters are
updated, there are cases where even though the user requests a limit
of 2 it can actually be set to 1.
Add the following test cases to check that the kernel rejects them:
- limit 2 depth 1 flows 1
- limit 2 depth 1 divisor 1
Signed-off-by: Octavian Purdila <tavip@google•com>
Acked-by: Cong Wang <xiyou.wangcong@gmail•com>
---
.../tc-testing/tc-tests/qdiscs/sfq.json | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/sfq.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/sfq.json
index 50e8d72781cb..28c6ce6da7db 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/sfq.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/sfq.json
@@ -228,5 +228,41 @@
"matchCount": "0",
"teardown": [
]
+ },
+ {
+ "id": "7f8f",
+ "name": "Check that a derived limit of 1 is rejected (limit 2 depth 1 flows 1)",
+ "category": [
+ "qdisc",
+ "sfq"
+ ],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "setup": [],
+ "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root sfq limit 2 depth 1 flows 1",
+ "expExitCode": "2",
+ "verifyCmd": "$TC qdisc show dev $DUMMY",
+ "matchPattern": "sfq",
+ "matchCount": "0",
+ "teardown": []
+ },
+ {
+ "id": "5168",
+ "name": "Check that a derived limit of 1 is rejected (limit 2 depth 1 divisor 1)",
+ "category": [
+ "qdisc",
+ "sfq"
+ ],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "setup": [],
+ "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root sfq limit 2 depth 1 divisor 1",
+ "expExitCode": "2",
+ "verifyCmd": "$TC qdisc show dev $DUMMY",
+ "matchPattern": "sfq",
+ "matchCount": "0",
+ "teardown": []
}
]
--
2.49.0.504.g3bcea36a83-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1
2025-04-07 20:24 [PATCH net v3 0/3] net_sched: sch_sfq: reject a derived limit of 1 Octavian Purdila
` (2 preceding siblings ...)
2025-04-07 20:24 ` [PATCH net v3 3/3] selftests/tc-testing: sfq: check that a derived limit of 1 is rejected Octavian Purdila
@ 2025-04-09 12:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-04-09 12:00 UTC (permalink / raw)
To: Octavian Purdila
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, horms,
shuah, netdev
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft•net>:
On Mon, 7 Apr 2025 13:24:06 -0700 you wrote:
> Because sfq parameters can influence each other there can be
> situations where although the user sets a limit of 2 it can be lowered
> to 1:
>
> $ tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 1 depth 1
> $ tc qdisc show dev dummy0
> qdisc sfq 1: dev dummy0 root refcnt 2 limit 1p quantum 1514b depth 1 divisor 1024
>
> [...]
Here is the summary with links:
- [net,v3,1/3] net_sched: sch_sfq: use a temporary work area for validating configuration
https://git.kernel.org/netdev/net/c/8c0cea59d40c
- [net,v3,2/3] net_sched: sch_sfq: move the limit validation
https://git.kernel.org/netdev/net/c/b3bf8f63e617
- [net,v3,3/3] selftests/tc-testing: sfq: check that a derived limit of 1 is rejected
https://git.kernel.org/netdev/net/c/26e705184e7a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread