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]

Message-ID: <CAM_iQpW4QTo9goSh4GCzH4SAWCGc_nkY0u_+iCO-bzw5AVPg3g@mail.gmail.com>
Date:   Mon, 4 May 2020 10:42:26 -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 Sun, May 3, 2020 at 5:48 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> 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.

Yeah, I was confused too when my colleagues reported this
to me.

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

Thanks for review!