[PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves

[PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves

[Pedro Tammela]

As reported [1] disallowing 'rt' inner curves breaks already existing
scripts. Even though it doesn't make sense 'qdisc wise' users have been
relying on this behaviour since the qdisc inception.

We need users to update the scripts/applications to use 'sc' or 'ls'
as a inner curve instead of 'rt', but also avoid the UAF found by
Budimir, which was present since the qdisc inception.

Instead of raising an error when classes are added with a 'rt' as a
parent, upgrade the 'rt' to an 'sc' on the fly, avoiding the UAF, and set
a warning for the user. Hopefully the warning laso triggers users to update
their scripts/applications.

[1] https://lore.kernel.org/all/297D84E3-736E-4AB4-B825-264279E2043C@flyingcircus.io/

Pedro Tammela (2):
  Revert "net/sched: sch_hfsc: Ensure inner classes have fsc curve"
  net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner
    curve

 net/sched/sch_hfsc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH net 1/2] Revert "net/sched: sch_hfsc: Ensure inner classes have fsc curve"

[Pedro Tammela]

This reverts commit b3d26c5702c7d6c45456326e56d2ccf3f103e60f.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_hfsc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 3554085bc2be..98805303218d 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1011,10 +1011,6 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		if (parent == NULL)
 			return -ENOENT;
 	}
-	if (!(parent->cl_flags & HFSC_FSC) && parent != &q->root) {
-		NL_SET_ERR_MSG(extack, "Invalid parent - parent class must have FSC");
-		return -EINVAL;
-	}
 
 	if (classid == 0 || TC_H_MAJ(classid ^ sch->handle) != 0)
 		return -EINVAL;
-- 
2.39.2

[PATCH net 2/2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve

[Pedro Tammela]

Christian Theune says:
   I upgraded from 6.1.38 to 6.1.55 this morning and it broke my traffic shaping script,
   leaving me with a non-functional uplink on a remote router.

A 'rt' curve cannot be used as a inner curve (parent class), but we were
allowing such configurations since the qdisc was introduced. Such
configurations would trigger a UAF as Budimir explains:
   The parent will have vttree_insert() called on it in init_vf(),
   but will not have vttree_remove() called on it in update_vf()
   because it does not have the HFSC_FSC flag set.

The qdisc always assumes that inner classes have the HFSC_FSC flag set.
This is by design as it doesn't make sense 'qdisc wise' for an 'rt'
curve to be an inner curve.

Budimir's original patch disallows users to add classes with a 'rt'
parent, but this is too strict as it breaks users that have been using
'rt' as a inner class. Another approach, taken by this patch, is to
upgrade the inner 'rt' into a 'sc', warning the user in the process.
It avoids the UAF reported by Budimir while also being more permissive
to bad scripts/users/code using 'rt' as a inner class.

Users checking the `tc class ls [...]` or `tc class get [...]` dumps would
observe the curve change and are potentially breaking with this change.

Cc: Christian Theune <ct@flyingcircus.io>
Cc: Budimir Markovic <markovicbudimir@gmail.com>
Fixes: 0c9570eeed69 ("net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve")
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_hfsc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 98805303218d..880c5f16b29c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -902,6 +902,14 @@ hfsc_change_usc(struct hfsc_class *cl, struct tc_service_curve *usc,
 	cl->cl_flags |= HFSC_USC;
 }
 
+static void
+hfsc_upgrade_rt(struct hfsc_class *cl)
+{
+	cl->cl_fsc = cl->cl_rsc;
+	rtsc_init(&cl->cl_virtual, &cl->cl_fsc, cl->cl_vt, cl->cl_total);
+	cl->cl_flags |= HFSC_FSC;
+}
+
 static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
 	[TCA_HFSC_RSC]	= { .len = sizeof(struct tc_service_curve) },
 	[TCA_HFSC_FSC]	= { .len = sizeof(struct tc_service_curve) },
@@ -1061,6 +1069,12 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	cl->cf_tree = RB_ROOT;
 
 	sch_tree_lock(sch);
+	/* Check if the inner class is a misconfigured 'rt' */
+	if (!(parent->cl_flags & HFSC_FSC) && parent != &q->root) {
+		NL_SET_ERR_MSG(extack,
+			       "Forced curve change on parent 'rt' to 'sc'");
+		hfsc_upgrade_rt(parent);
+	}
 	qdisc_class_hash_insert(&q->clhash, &cl->cl_common);
 	list_add_tail(&cl->siblings, &parent->children);
 	if (parent->level == 0)
-- 
2.39.2

Re: [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves

[Jamal Hadi Salim]

On Fri, Oct 13, 2023 at 11:11 AM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> As reported [1] disallowing 'rt' inner curves breaks already existing
> scripts. Even though it doesn't make sense 'qdisc wise' users have been
> relying on this behaviour since the qdisc inception.
>
> We need users to update the scripts/applications to use 'sc' or 'ls'
> as a inner curve instead of 'rt', but also avoid the UAF found by
> Budimir, which was present since the qdisc inception.
>
> Instead of raising an error when classes are added with a 'rt' as a
> parent, upgrade the 'rt' to an 'sc' on the fly, avoiding the UAF, and set
> a warning for the user. Hopefully the warning laso triggers users to update
> their scripts/applications.
>
> [1] https://lore.kernel.org/all/297D84E3-736E-4AB4-B825-264279E2043C@flyingcircus.io/
>

For the series:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Christian, this should fix it for you but with a caveat: if you
configure a "faulty" inner qdis to be rt it will "fixed" - meaning you
can keep your scripts but when you dump you will see the "fixed"
version instead of the "faulty" one.

cheers,
jamal

Re: [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves

[Christian Theune]

Hi,

> On 16. Oct 2023, at 11:48, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 
> For the series:
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Christian, this should fix it for you but with a caveat: if you
> configure a "faulty" inner qdis to be rt it will "fixed" - meaning you
> can keep your scripts but when you dump you will see the "fixed"
> version instead of the "faulty" one.

Thanks a lot, this is absolutely fine for my situation!

Christian

-- 
Christian Theune · ct@flyingcircus.io · +49 345 219401 0
Flying Circus Internet Operations GmbH · https://flyingcircus.io
Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland
HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick

Re: [PATCH net 2/2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve

[Jakub Kicinski]

On Fri, 13 Oct 2023 12:10:57 -0300 Pedro Tammela wrote:
> Budimir's original patch disallows users to add classes with a 'rt'
> parent, but this is too strict as it breaks users that have been using
> 'rt' as a inner class. Another approach, taken by this patch, is to
> upgrade the inner 'rt' into a 'sc', warning the user in the process.
> It avoids the UAF reported by Budimir while also being more permissive
> to bad scripts/users/code using 'rt' as a inner class.

Perfect, thank you.

> Users checking the `tc class ls [...]` or `tc class get [...]` dumps would
> observe the curve change and are potentially breaking with this change.
> 
> Cc: Christian Theune <ct@flyingcircus.io>
> Cc: Budimir Markovic <markovicbudimir@gmail.com>
> Fixes: 0c9570eeed69 ("net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve")

git says this SHA does not exist. From the title I'm guessing this is
the patch itself, some mis-automation, perhaps?

All in all I think you can squash the revert into this and use
b3d26c5702c7d6c45 for Fixes. I don't think there's a reason to keep
the revert separate, given how small it is. And if the revert is
first what if someone backports just the revert..
-- 
pw-bot: cr