netdev - Re: [Patch net v2] net_sched: fix tcm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 3 May 2020 08:48:18 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net v2] net_sched: fix tcm_parent in tc filter dump

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@...atatu.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