netdev - Re: [PATCH net 1/1 v3] net: sched: Disallow replacing of child qdisc from one parent to another

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAM0EoM=zzidXbUNYUfnV-P5vVU7KOYZJPw7bdfKt4+nSdyWCvw@mail.gmail.com>
Date: Wed, 15 Jan 2025 15:39:43 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, jiri@...nulli.us, xiyou.wangcong@...il.com, 
	davem@...emloft.net, edumazet@...gle.com, security@...nel.org, 
	nnamrec@...il.com
Subject: Re: [PATCH net 1/1 v3] net: sched: Disallow replacing of child qdisc
 from one parent to another

On Wed, Jan 15, 2025 at 10:51 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 15 Jan 2025 09:53:27 -0500 Jamal Hadi Salim wrote:
> > > > I may be missing something - what does TC_H_MAJ() provide?
> > > > The 3:1 and 1:3 in that example are both descendants of the same
> > > > parent. It could have been 1:3 vs 1:2 and the same rules would apply.
> > >
> > > Let me flip the question. What qdisc movement / grafts are you intending
> > > to still support?
> > >
> >
> > Grafting essentially boils down to a del/add of a qdisc. The
> > ambiguities: Does it mean deleting it from one hierachy point and
> > adding it to another point? Or does it mean not deleting it from the
> > first location but making it available in the other one?
> >
> > > From the report it sounds like we don't want to support _any_ movement
> > > of existing qdiscs within the hierarchy. Only purpose of graft would
> > > be to install a new / fresh qdisc as a child.
> >
> > That sounded like the safest approach. If there is a practical use for
> > moving queues around (I am not aware of any, doesnt mean there is no
> > practical use) then we can do the much bigger surgery.
>
> So coming back to the code I would have expected the patch to look
> something along the lines of:
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 300430b8c4d2..fac9c946a4c7 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1664,6 +1664,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>                                 q = qdisc_lookup(dev, tcm->tcm_handle);
>                                 if (!q)
>                                         goto create_n_graft;
> +                               if (q->parent != tcm->tcm_parent) {
> +                                       NL_SET_ERR_MSG(extack, "Cannot move an existing qdisc to a different parent");
> +                                       return -EINVAL;
> +                               }

Yes, this should work as well - doesnt have to save the leaf_q, so more optimal.
Please send the patch.

cheers,
jamal
>                                 if (n->nlmsg_flags & NLM_F_EXCL) {
>                                         NL_SET_ERR_MSG(extack, "Exclusivity flag on, cannot override");
>                                         return -EEXIST;
>
>
> Whether a real (non-default) leaf already existed in that spot
> is not important..