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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 17 Oct 2023 11:59:30 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Dave Taht <dave.taht@...il.com>
Cc: Pedro Tammela <pctammela@...atatu.com>, netdev@...r.kernel.org, 
	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 Tue, Oct 17, 2023 at 11:48 AM Dave Taht <dave.taht@...il.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@...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