Messages in this thread | | | From | Kurt Kanzenbach <> | Subject | Re: [PATCH v2 net 2/2] net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs | Date | Wed, 22 Feb 2023 15:03:04 +0100 |
| |
Hi Vladimir,
On Thu Sep 15 2022, Vladimir Oltean wrote: > taprio can only operate as root qdisc, and to that end, there exists the > following check in taprio_init(), just as in mqprio: > > if (sch->parent != TC_H_ROOT) > return -EOPNOTSUPP; > > And indeed, when we try to attach taprio to an mqprio child, it fails as > expected: > > $ tc qdisc add dev swp0 root handle 1: mqprio num_tc 8 \ > map 0 1 2 3 4 5 6 7 \ > queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 hw 0 > $ tc qdisc replace dev swp0 parent 1:2 taprio num_tc 8 \ > map 0 1 2 3 4 5 6 7 \ > queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ > base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \ > flags 0x0 clockid CLOCK_TAI > Error: sch_taprio: Can only be attached as root qdisc. > > (extack message added by me) > > But when we try to attach a taprio child to a taprio root qdisc, > surprisingly it doesn't fail: > > $ tc qdisc replace dev swp0 root handle 1: taprio num_tc 8 \ > map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ > base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \ > flags 0x0 clockid CLOCK_TAI > $ tc qdisc replace dev swp0 parent 1:2 taprio num_tc 8 \ > map 0 1 2 3 4 5 6 7 \ > queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ > base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \ > flags 0x0 clockid CLOCK_TAI > > This is because tc_modify_qdisc() behaves differently when mqprio is > root, vs when taprio is root. > > In the mqprio case, it finds the parent qdisc through > p = qdisc_lookup(dev, TC_H_MAJ(clid)), and then the child qdisc through > q = qdisc_leaf(p, clid). This leaf qdisc q has handle 0, so it is > ignored according to the comment right below ("It may be default qdisc, > ignore it"). As a result, tc_modify_qdisc() goes through the > qdisc_create() code path, and this gives taprio_init() a chance to check > for sch_parent != TC_H_ROOT and error out. > > Whereas in the taprio case, the returned q = qdisc_leaf(p, clid) is > different. It is not the default qdisc created for each netdev queue > (both taprio and mqprio call qdisc_create_dflt() and keep them in > a private q->qdiscs[], or priv->qdiscs[], respectively). Instead, taprio > makes qdisc_leaf() return the _root_ qdisc, aka itself. > > When taprio does that, tc_modify_qdisc() goes through the qdisc_change() > code path, because the qdisc layer never finds out about the child qdisc > of the root. And through the ->change() ops, taprio has no reason to > check whether its parent is root or not, just through ->init(), which is > not called. > > The problem is the taprio_leaf() implementation. Even though code wise, > it does the exact same thing as mqprio_leaf() which it is copied from, > it works with different input data. This is because mqprio does not > attach itself (the root) to each device TX queue, but one of the default > qdiscs from its private array. > > In fact, since commit 13511704f8d7 ("net: taprio offload: enforce qdisc > to netdev queue mapping"), taprio does this too, but just for the full > offload case. So if we tried to attach a taprio child to a fully > offloaded taprio root qdisc, it would properly fail too; just not to a > software root taprio. > > To fix the problem, stop looking at the Qdisc that's attached to the TX > queue, and instead, always return the default qdiscs that we've > allocated (and to which we privately enqueue and dequeue, in software > scheduling mode). > > Since Qdisc_class_ops :: leaf is only called from tc_modify_qdisc(), > the risk of unforeseen side effects introduced by this change is > minimal. > > Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio scheduler") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
This commit was backported to v5.15-LTS which results in NULL pointer dereferences e.g., when attaching an ETF child qdisc to taprio.
From what I can see is, that the issue was reported back then and this commit was reverted [1]. However, the revert didn't make it into v5.15-LTS? Is there a reason for it? I'm testing 5.15.94-rt59 here.
Thanks, Kurt
[1] - https://lore.kernel.org/all/20221004220100.1650558-1-vladimir.oltean@nxp.com/ [unhandled content-type:application/pgp-signature] | |