netdev - Re: [PATCH net-next 4/4] act_mirred: use ACT

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:   Tue, 17 Jul 2018 02:38:44 -0700
From:   Dave Taht <dave.taht@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiří Pírko <jiri@...nulli.us>,
        ast@...nel.org, Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        eyal.birger@...il.com
Subject: Re: [PATCH net-next 4/4] act_mirred: use ACT_REDIRECT when possible

On Tue, Jul 17, 2018 at 2:16 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Hi,
>
> On Mon, 2018-07-16 at 16:39 -0700, Cong Wang wrote:
> > On Fri, Jul 13, 2018 at 2:55 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > >
> > > When mirred is invoked from the ingress path, and it wants to redirect
> > > the processed packet, it can now use the ACT_REDIRECT action,
> > > filling the tcf_result accordingly.
> > >
> > > This avoids a skb_clone() in the TC S/W data path giving a ~10%
> > > improvement in forwarding performances. Overall TC S/W performances
> > > are now comparable to the kernel openswitch datapath.
>
> Thank you for the feedback.
>
> > Avoiding skb_clone() for redirection is cool, but why need to use
> > skb_do_redirect() here?
>
> Well, it's not needed. I tried to reduce code duplication, and I tried
> to avoid adding another TC_ACT_* value.
>
> > There is a subtle difference here:
> >
> > skb_do_redirect() calls __bpf_rx_skb() which calls
> > dev_forward_skb().
> >
> > while the current mirred action doesn't scrub packets when
> > redirecting to ingress (from egress). Although I forget if it is
> > intentionally.
>
> Understood.
>
> A possible option out of this issues would be adding another action
> value - TC_ACT_MIRRED ? - and handle it in sch_handle_egress()[1] with
> the appropriate semantic. That should address also Daniel and Eyal
> concerns.
>
> Would you consider the above acceptable?

Our dream has been to be able to specify a 1 liner:

tc qdisc add dev eth0 ingress cake bandwidth 200mbit besteffort wash

and be done with it, eliminating ifb, mirred, etc, entirely.

(I am otherwise delighted to see anything appear that makes inbound
shaping cheaper along the lines of this patchset)
>
> Thanks,
>
> Paolo
>
>


-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619