[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA93jw6Oh66k+tA3Ad8QFA7-gGnoY_hsk8d2bRDgs_0AjE69Aw@mail.gmail.com>
Date: Tue, 17 Oct 2023 08:48:12 -0700
From: Dave Taht <dave.taht@...il.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, Christian Theune <ct@...ingcircus.io>,
Budimir Markovic <markovicbudimir@...il.com>
Subject: Re: [PATCH net v2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it
becomes a inner curve
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@...atatu.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@...ingcircus.io>
> Cc: Budimir Markovic <markovicbudimir@...il.com>
> Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve")
> Signed-off-by: Pedro Tammela <pctammela@...atatu.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
Powered by blists - more mailing lists