* [PATCH net 1/2] Revert "net/sched: sch_hfsc: Ensure inner classes have fsc curve"
2023-10-13 15:10 [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves Pedro Tammela
@ 2023-10-13 15:10 ` Pedro Tammela
2023-10-13 15:10 ` [PATCH net 2/2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve Pedro Tammela
2023-10-16 9:48 ` [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves Jamal Hadi Salim
2 siblings, 0 replies; 6+ messages in thread
From: Pedro Tammela @ 2023-10-13 15:10 UTC (permalink / raw)
To: netdev
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 2/2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve
2023-10-13 15:10 [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves Pedro Tammela
2023-10-13 15:10 ` [PATCH net 1/2] Revert "net/sched: sch_hfsc: Ensure inner classes have fsc curve" Pedro Tammela
@ 2023-10-13 15:10 ` Pedro Tammela
2023-10-17 1:17 ` Jakub Kicinski
2023-10-16 9:48 ` [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves Jamal Hadi Salim
2 siblings, 1 reply; 6+ messages in thread
From: Pedro Tammela @ 2023-10-13 15:10 UTC (permalink / raw)
To: netdev
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni,
Pedro Tammela, Christian Theune, Budimir Markovic
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves
2023-10-13 15:10 [PATCH net 0/2] net/sched: sch_hfsc: safely allow 'rt' inner curves Pedro Tammela
2023-10-13 15:10 ` [PATCH net 1/2] Revert "net/sched: sch_hfsc: Ensure inner classes have fsc curve" Pedro Tammela
2023-10-13 15:10 ` [PATCH net 2/2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve Pedro Tammela
@ 2023-10-16 9:48 ` Jamal Hadi Salim
2023-10-16 9:50 ` Christian Theune
2 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2023-10-16 9:48 UTC (permalink / raw)
To: netdev
Cc: xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni,
Christian Theune, Pedro Tammela, Budimir Markovic
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
^ permalink raw reply [flat|nested] 6+ messages in thread