netdev - Re: [PATCH net-next 3/4] net/tc: introduce TC_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:   Mon, 23 Jul 2018 14:12:56 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Jiri Pirko <jiri@...nulli.us>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Eyal Birger <eyal.birger@...il.com>
Subject: Re: [PATCH net-next 3/4] net/tc: introduce TC_ACT_MIRRED.

On Fri, Jul 20, 2018 at 2:54 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Hi,
>
> Jiri, Cong, thank you for the feedback. Please allow me to give a
> single reply to both of you, as you rised similar concers.
>
> On Thu, 2018-07-19 at 11:07 -0700, Cong Wang wrote:
> > On Thu, Jul 19, 2018 at 6:03 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > >
> > > This is similar TC_ACT_REDIRECT, but with a slightly different
> > > semantic:
> > > - on ingress the mirred skbs are passed to the target device
> > > network stack without any additional check not scrubbing.
> > > - the rcu-protected stats provided via the tcf_result struct
> > >   are updated on error conditions.
> >
> > At least its name sucks, it means to skip the skb_clone(),
> > that is avoid a copy, but you still call it MIRRED...
> >
> > MIRRED means MIRror and REDirect.
>
> I was not satified with the name, too, but I also wanted to collect
> some feedback, as the different time zones are not helping here.
>
> Would TC_ACT_REINJECT be a better choice? (renaming skb_tc_redirect as
> skb_tc_reinject, too). Do you have some better name?


Any name not implying a copy is better. I don't worry about it, please
see below.


> > Also, I don't understand why this new TC_ACT code needs
> > to be visible to user-space, whether to clone or not is purely
> > internal.
>
> Note this is what already happens with TC_ACT_REDIRECT: currently the
> user space uses it freely, even if only {cls,act}_bpf can return such
> value in a meaningful way, and only from the ingress and the egress
> hooks.
>

Yes, my question is why do we give user such a freedom?

In other words, what do you want users to choose here? To scrub or not
to scrub? To clone or not to clone?

>From my understanding of your whole patchset, your goal is to get rid
of clone, and users definitely don't care about clone or not clone for
redirections, this is why I insist it doesn't need to be visible to user.

If your goal is not just skipping clone, but also, let's say, scrub or not
scrub, then it should be visible to users. However, I don't see why
users care about scrub or not, they have to understand what scrub
is at least, it is a purely kernel-internal behavior.


> I think we can add a clear separation between the values accessible
> from user-space, and the ones used interanally by the kernel, with
> something like the code below (basically unknown actions are explicitly
> mapped to TC_ACT_UNSPEC), WDYT?
>
> Note: as TC_ACT_REDIRECT is already part of the uAPI, it will remain
> accessible from user-space, so patch 1/4 would be still needed.

I think that is doable too, but we should understand whether we
need to do it or not at first.

Thanks.