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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 3 May 2020 08:02:45 -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-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". IOW, I could install that filter alternatively as: ---- # $TC filter add dev $DEV parent 8008: protocol arp prio 11 basic action pass --- Basically these two filters are equivalent and should end in the same qdisc. To test, I added those two filters (the prio is useful to visualize in the dump). Lets see the dump: ------- # $TC filter show dev $DEV root filter protocol arp pref 10 basic chain 0 filter protocol arp pref 10 basic chain 0 handle 0x1 action order 1: gact action pass random type none pass val 0 index 1 ref 1 bind 1 --------- I was hoping i'd see both filters, but alas there's only one. Lets try a different dump explicitly specifying root qdisc id: --------- # $TC filter show dev $DEV parent 8008: filter protocol arp pref 11 basic chain 0 filter protocol arp pref 11 basic chain 0 handle 0x1 action order 1: gact action pass random type none pass val 0 index 2 ref 1 bind 1 --------- Again i was hoping to see both filters. This sounds like the two filters are anchored at two different qdiscs instead of the same one (i.e root). Hence my suspicion... Lets add a filter at ingress: ----- $TC filter add dev $DEV parent ffff:fff1 protocol arp basic action pass ------- Ok lets dump this from ingress: ----- # $TC filter show dev $DEV parent ffff:fff1 filter protocol arp pref 10 basic chain 0 filter protocol arp pref 10 basic chain 0 handle 0x1 action order 1: gact action pass random type none pass val 0 index 1 ref 1 bind 1 filter protocol arp pref 49152 basic chain 0 filter protocol arp pref 49152 basic chain 0 handle 0x1 action order 1: gact action pass random type none pass val 0 index 3 ref 1 bind 1 ------------- Same result if i said "root". I was only expecting to see the one with pref 49152 in the above output. It _feels_ like those two filters(ingress and egress) are installed in the same struct. Ok, last dump without specifying a parent, which should pick root qdisc per code: ------ # $TC filter show dev $DEV filter parent 8008: protocol arp pref 11 basic chain 0 filter parent 8008: protocol arp pref 11 basic chain 0 handle 0x1 action order 1: gact action pass random type none pass val 0 index 2 ref 1 bind 1 ---- Semantically this should have dumped all 3 filters and not just pick root. But that issue has been there for a decade like you said. So it is reasonable to specify parent ffff:fff1 for dumping just ingress side. Also reasonable not to see ingress when dumping root. > If parent is not specified, only egress will be shown, as > we just assign q = dev->qdisc. > Which is the root egress qdisc. That is the "$TC filter show dev $DEV" scenario. See my comment above. > I agree, 'root' should mean the root qdisc on egress, matching > ingress with 'root' doesn't make much sense to me either. > > But I am afraid it is too late to change ,if this behavior has been > there for 12+ years. > Although i cant pinpoint exactly when - this used to work (I dont think its 12+ years but I could be wrong). These semantics are really broken. Do you have time to look at the theory that things break at install? If you dont have time i could try to debug it by Tuesday. cheers, jamal
Powered by blists - more mailing lists