Re: [Patch net v2] net_sched: fix tcm_parent in tc filter dump

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 2020-05-03 8:02 a.m., Jamal Hadi Salim wrote:
> On 2020-05-02 10:28 p.m., Cong Wang wrote:
>> On Sat, May 2, 2020 at 2:19 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> On 2020-05-02 4:48 a.m., Jamal Hadi Salim wrote:
> 
> [..]
>>>> Note:
>>>> tc filter show dev dummy0 root
>>>> should not show that filter. OTOH,
>>>> tc filter show dev dummy0 parent ffff:
>>>> should.
>>
>> Hmm, but we use TC_H_MAJ(tcm->tcm_parent) to do the
>> lookup, 'root' (ffff:ffff) has the same MAJ with ingress
>> (ffff:0000).
>>
> 
> I have some long analysis and theory below.
> 
>> And qdisc_lookup() started to search for ingress since 2008,
>> commit 8123b421e8ed944671d7241323ed3198cccb4041.
>>
>> So it is likely too late to change this behavior even if it is not
>> what we prefer.
>>
> 
> My gut feeling is that whatever broke (likely during block addition
> maybe even earlier during clsact addition) is in the code
> path for adding filter. Dumping may have bugs but i would
> point a finger to filter addition first.
> More below.... (sorry long email).
> 
> 
> Here's what i tried after applying your patch:
> 
> ----
> # $TC qd add dev $DEV ingress
> # $TC qd add dev $DEV root prio
> # $TC qd ls dev $DEV
> qdisc noqueue 0: dev lo root refcnt 2
> qdisc prio 8008: dev enp0s1 root refcnt 2 bands 3 priomap 1 2 2 2 1 2 0 
> 0 1 1 1 1 1 1 1 1
> qdisc ingress ffff: dev enp0s1 parent ffff:fff1 ----------------
> -----
> 
> egress i.e root is at 8008:
> ingress is at ffff:fff1
> 
> If say:
> ---
> # $TC filter add dev $DEV root protocol arp prio 10 basic action pass
> ----
> 
> i am instructing the kernel to "go and find root (which is 8008:)
> and install the filter there".

Ok, I went backwards and looked at many kernel sources.
It is true we install the filters in two different locations
i.e just specifying TC_H_ROOT does not equate to picking
the egress qdisc with that flag.
And has been broken for way too long - so we have to live
with it.
I wish we had more tdc tests and earlier.

Advise to users is not to use semantics like "root" or ingress
but rather explicitly specify the parent.

So ignore what i said above. I will ACK your patch.

cheers,
jamal