On Tue, May 30, 2023 at 6:16 AM Paolo Abeni [off-list ref] wrote:

On Mon, 2023-05-29 at 17:17 -0700, Peilin Ye wrote:

quoted

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

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.

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.

Jamal, are you ok with the above explanation? Perhaps it would be
worthy to add a specific test-case under tc-testing for this issue?

I am fine with this one going in as is and then adding tests. I will ACK it.

cheers,
jamal

Thanks!

Paolo