| Message ID | 20231017143602.3191556-1-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a13b67c9a015c4e21601ef9aa4ec9c5d972df1b4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve | expand |
On Tue, Oct 17, 2023 at 10:36 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > 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. > > v1->v2: https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/ > - Correct 'Fixes' tag and merge with revert (Jakub) > > Cc: Christian Theune <ct@flyingcircus.io> > Cc: Budimir Markovic <markovicbudimir@gmail.com> > Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve") > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > --- > net/sched/sch_hfsc.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c > index 3554085bc2be..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) }, > @@ -1011,10 +1019,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; > @@ -1065,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 >
On occasion I try to get those still using hfsc to give CAKE a shot, or give me a benchmark as to why the view hfsc as better. It is a very interesting and hard to understand shaping mechanism. On Tue, Oct 17, 2023 at 7:36 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > 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. > > v1->v2: https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/ > - Correct 'Fixes' tag and merge with revert (Jakub) > > Cc: Christian Theune <ct@flyingcircus.io> > Cc: Budimir Markovic <markovicbudimir@gmail.com> > Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve") > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- > net/sched/sch_hfsc.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c > index 3554085bc2be..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) }, > @@ -1011,10 +1019,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; > @@ -1065,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 > >
On Tue, Oct 17, 2023 at 11:48 AM Dave Taht <dave.taht@gmail.com> wrote: > > On occasion I try to get those still using hfsc to give CAKE a shot, > or give me a benchmark as to why the view hfsc as better. It is a very > interesting and hard to understand shaping mechanism. Yes, hfsc is a bit too clever. If you have some results you are more than welcome to present them at the TC workshop. cheers, jamal > On Tue, Oct 17, 2023 at 7:36 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > > > 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. > > > > v1->v2: https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/ > > - Correct 'Fixes' tag and merge with revert (Jakub) > > > > Cc: Christian Theune <ct@flyingcircus.io> > > Cc: Budimir Markovic <markovicbudimir@gmail.com> > > Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve") > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > > --- > > net/sched/sch_hfsc.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c > > index 3554085bc2be..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) }, > > @@ -1011,10 +1019,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; > > @@ -1065,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 > > > > > > > -- > Oct 30: https://netdevconf.info/0x17/news/the-maestro-and-the-music-bof.html > Dave Täht CSO, LibreQos
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 17 Oct 2023 11:36:02 -0300 you wrote: > 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. > > [...] Here is the summary with links: - [net,v2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve https://git.kernel.org/netdev/net/c/a13b67c9a015 You are awesome, thank you!
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c index 3554085bc2be..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) }, @@ -1011,10 +1019,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; @@ -1065,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)
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. v1->v2: https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/ - Correct 'Fixes' tag and merge with revert (Jakub) Cc: Christian Theune <ct@flyingcircus.io> Cc: Budimir Markovic <markovicbudimir@gmail.com> Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve") Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/sch_hfsc.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)