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

[Jakub Kicinski <kuba@kernel.org>]

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;
+				}
 				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..