[PATCH net v2 0/3] net_sched: sch_sfq: reject a derived limit of 1

[PATCH net v2 0/3] net_sched: sch_sfq: reject a derived limit of 1

[Octavian Purdila]

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

$ tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 10 depth 1 divisor 1
$ tc qdisc show dev dummy0
qdisc sfq 2: root refcnt 2 limit 1p quantum 1514b depth 1 divisor 1

As a limit of 1 is invalid, this patch series moves the limit
validation to after all configuration changes have been done. To do
so, the configuration is done in a temporary work area then applied to
the internal state.

The patch series also adds new test cases.

Changes in v2:
 - remove tmp struct and directly use local variables

v1: https://lore.kernel.org/all/20250328201634.3876474-1-tavip@google.com/


Octavian Purdila (3):
  net_sched: sch_sfq: use a temporary work area for validating
    configuration
  net_sched: sch_sfq: move the limit validation
  selftests/tc-testing: sfq: check that a derived limit of 1 is rejected

 net/sched/sch_sfq.c                           | 68 ++++++++++++++-----
 .../tc-testing/tc-tests/qdiscs/sfq.json       | 36 ++++++++++
 2 files changed, 88 insertions(+), 16 deletions(-)

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH net v2 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration

[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>
---
 net/sched/sch_sfq.c | 58 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 65d5b59da583..1af06cd5034a 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -631,6 +631,16 @@ 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;
+	/* work area for validating changes before committing them */
+	int limit;
+	unsigned int divisor;
+	unsigned int maxflows;
+	int perturb_period;
+	unsigned int quantum;
+	u8 headdrop;
+	u8 maxdepth;
+	u8 flags;
+
 
 	if (opt->nla_len < nla_attr_size(sizeof(*ctl)))
 		return -EINVAL;
@@ -656,36 +666,60 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
 		NL_SET_ERR_MSG_MOD(extack, "invalid limit");
 		return -EINVAL;
 	}
+
 	sch_tree_lock(sch);
+
+	/* copy configuration to work area */
+	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, no return from this point further */
+	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.472.ge94155a9ec-goog

Re: [PATCH net v2 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration

[Stephen Hemminger]

On Wed,  2 Apr 2025 09:27:48 -0700
Octavian Purdila <tavip@google.com> wrote:

> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> index 65d5b59da583..1af06cd5034a 100644
> --- a/net/sched/sch_sfq.c
> +++ b/net/sched/sch_sfq.c
> @@ -631,6 +631,16 @@ 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;
> +	/* work area for validating changes before committing them */
Unnecessary comment.
> +	int limit;
> +	unsigned int divisor;
> +	unsigned int maxflows;
> +	int perturb_period;
> +	unsigned int quantum;
> +	u8 headdrop;
> +	u8 maxdepth;
> +	u8 flags;
> +

Network code prefers reverse christmas tree style declaration order.

+	/* copy configuration to work area */
+	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;

Comment is unneeded. Rather than doing individual fields, why not just use
a whole temporary structure?

Re: [PATCH net v2 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration

[Octavian Purdila]

On Wed, Apr 2, 2025 at 1:46 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>

Hi Stephen,

Thanks for the review.

> On Wed,  2 Apr 2025 09:27:48 -0700
> Octavian Purdila <tavip@google.com> wrote:
>
> > diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> > index 65d5b59da583..1af06cd5034a 100644
> > --- a/net/sched/sch_sfq.c
> > +++ b/net/sched/sch_sfq.c
> > @@ -631,6 +631,16 @@ 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;
> > +     /* work area for validating changes before committing them */
> Unnecessary comment.

Fair, I'll remove the comments (here and below).

> > +     int limit;
> > +     unsigned int divisor;
> > +     unsigned int maxflows;
> > +     int perturb_period;
> > +     unsigned int quantum;
> > +     u8 headdrop;
> > +     u8 maxdepth;
> > +     u8 flags;
> > +
>
> Network code prefers reverse christmas tree style declaration order.
>

Thanks, I'll fix the declaration order.

> +       /* copy configuration to work area */
> +       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;
>
> Comment is unneeded. Rather than doing individual fields, why not just use
> a whole temporary structure?

Do you mean save/restore a full struct sfq_sched_data? I am not sure
it is safe to do so since it includes a struct timer_list which, if I
am not mistaken, could change under us during save/restore as other
timers are added / removed.

Beside that, it is quite big (688 bytes on 64bit) so not ideal to put
it on stack, would probably need to allocate / free it. And we need to
copy a lot more fields than we need. It seems a bit inefficient.

[PATCH net v2 2/3] net_sched: sch_sfq: move the limit validation

[Octavian Purdila]

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>
---
 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 1af06cd5034a..d731013ec851 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -662,10 +662,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);
 
@@ -707,6 +703,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, no return from this point further */
 	q->limit = limit;
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH net v2 3/3] selftests/tc-testing: sfq: check that a derived limit of 1 is rejected

[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>
---
 .../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.472.ge94155a9ec-goog

Re: [PATCH net v2 0/3] net_sched: sch_sfq: reject a derived limit of 1

[Cong Wang]

On Wed, Apr 02, 2025 at 09:27:47AM -0700, Octavian Purdila 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
> 
> $ tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 10 depth 1 divisor 1
> $ tc qdisc show dev dummy0
> qdisc sfq 2: root refcnt 2 limit 1p quantum 1514b depth 1 divisor 1
> 
> As a limit of 1 is invalid, this patch series moves the limit
> validation to after all configuration changes have been done. To do
> so, the configuration is done in a temporary work area then applied to
> the internal state.
> 
> The patch series also adds new test cases.

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Please carry this Ack when you send v3.

Thanks.