Hi Peilin:
	Thank you for your reply.
On 2023/5/30 8:17, Peilin Ye wrote:

On Mon, May 29, 2023 at 09:53:28AM -0400, Jamal Hadi Salim wrote:

quoted

On Mon, May 29, 2023 at 4:59 AM Peilin Ye [off-list ref] wrote:

quoted

On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:

quoted

On 2023/5/29 3:05, Jamal Hadi Salim wrote:

quoted

On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao [off-list ref] wrote:

quoted

When use the following command to test:
1)ip link add bond0 type bond
2)ip link set bond0 up
3)tc qdisc add dev bond0 root handle ffff: mq
4)tc qdisc replace dev bond0 parent ffff:fff1 handle ffff: mq

This is fixed by Peilin in this ongoing discussion:
https://lore.kernel.org/netdev/cover.1684887977.git.peilin.ye@bytedance.com/ (local)

       Thank you for your reply. I have notice Peilin's patches before,
and test after the patch is incorporated in local host. But it still
triggers the problem.
       Peilin's patches can be filtered out when the query result of
qdisc_lookup is of the ingress type. Here is 4/6 patch in his patches.
+if (q->flags & TCQ_F_INGRESS) {
+     NL_SET_ERR_MSG(extack,
+                    "Cannot regraft ingress or clsact Qdiscs");
+     return -EINVAL;
+}
       However, the query result of my test case in qdisc_lookup is mq.
Therefore, the patch cannot solve my problem.

Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
from being regrafted (to elsewhere), and Zhengchao's patch prevents other
Qdiscs from being regrafted to ffff:fff1.

Ok, at first glance it was not obvious.
Do we catch all combinations? for egress (0xffffffff) allowed minor is
0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
is 0xfff1 and 0xfff2(clsact).

ffff:fff1 is special in tc_modify_qdisc(); if minor isn't fff1,
tc_modify_qdisc() thinks user wants to graft a Qdisc under existing ingress
or clsact Qdisc:

	if (clid != TC_H_INGRESS) {	/* ffff:fff1 */
		p = qdisc_lookup(dev, TC_H_MAJ(clid));
		if (!p) {
			NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
			return -ENOENT;
		}
		q = qdisc_leaf(p, clid);
	} else if (dev_ingress_queue_create(dev)) {
		q = dev_ingress_queue(dev)->qdisc_sleeping;
	}

This will go to the "parent != NULL" path in qdisc_graft(), and
sch_{ingress,clsact} doesn't implement cl_ops->graft(), so -EOPNOTSUPP will
be returned.

Yes, I agree.

In short, yes, I think ffff:fff1 is the only case should be fixed.

By the way I just noticed that currently it is possible to create a e.g.
HTB class with a class ID of ffff:fff1...

   $ tc qdisc add dev eth0 root handle ffff: htb default fff1
   $ tc class add dev eth0 \
             parent ffff: classid ffff:fff1 htb rate 100%

Regrafting a Qdisc to such classes won't work as intended at all.  It's a
separate issue though.

This seems to be another problem.

Zhengchao Shao

Thanks,
Peilin Ye