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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAM_iQpWi9MA5DEk7933aah3yeOQ+=bHO8H2-xpqTtcXn0k=+0Q@mail.gmail.com>
Date:   Sat, 2 May 2020 19:28:10 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.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 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:
> > On 2020-04-30 11:53 p.m., Cong Wang wrote:
>
> [..]
> >> Steps to reproduce this:
> >>   ip li set dev dummy0 up
> >>   tc qd add dev dummy0 ingress
> >>   tc filter add dev dummy0 parent ffff: protocol arp basic action pass
> >>   tc filter show dev dummy0 root
> >>
> >> Before this patch:
> >>   filter protocol arp pref 49152 basic
> >>   filter protocol arp pref 49152 basic handle 0x1
> >>     action order 1: gact action pass
> >>      random type none pass val 0
> >>      index 1 ref 1 bind 1
> >>
> >> After this patch:
> >>   filter parent ffff: protocol arp pref 49152 basic
> >>   filter parent ffff: protocol arp pref 49152 basic handle 0x1
> >>       action order 1: gact action pass
> >>        random type none pass val 0
> >>      index 1 ref 1 bind 1
> >
> > 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).

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.


> >
> > root and ffff: are distinct/unique installation hooks.
> >
>
> Suprised no one raised this earlier - since it is so
> fundamental (we should add a tdc test for it). I went back
> to the oldest kernel i have from early 2018 and it was broken..
>
> Cong, your patch is good for the case where we
> want to show _all_ filters regardless of where they
> were installed but only if no parent is specified. i.e if i did this:
> tc filter show dev dummy0
> then i am asking to see all the filters for that device.
> I am actually not sure if "tc filter show dev dummy0"
> ever worked that way - but it makes sense since
> no dump-filtering is specified.

If parent is not specified, only egress will be shown, as
we just assign q = dev->qdisc.

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.

Thanks.