netdev - Re: [Patch bpf-next v3 4/5] skmsg: use skb ext instead of TCP_SKB

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: <602b17b0492a8_3ed41208f2@john-XPS-13-9370.notmuch>
Date:   Mon, 15 Feb 2021 16:54:08 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, duanxiongchun@...edance.com,
        Dongdong Wang <wangdongdong.6@...edance.com>,
        jiang.wang@...edance.com, Cong Wang <cong.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: Re: [Patch bpf-next v3 4/5] skmsg: use skb ext instead of TCP_SKB_CB

Cong Wang wrote:
> On Mon, Feb 15, 2021 at 3:57 PM John Fastabend <john.fastabend@...il.com> wrote:
> >
> > For TCP case we can continue to use CB and not pay the price. For UDP
> > and AF_UNIX we can do the extra alloc.
> 
> I see your point, but specializing TCP case does not give much benefit
> here, the skmsg code would have to check skb->protocol etc. to decide
> whether to use TCP_SKB_CB() or skb_ext:
> 
> if (skb->protocol == ...)
>   TCP_SKB_CB(skb) = ...;
> else
>   ext = skb_ext_find(skb);
> 
> which looks ugly to me. And I doubt skb->protocol alone is sufficient to
> distinguish TCP, so we may end up having more checks above.
> 
> So do you really want to trade code readability with an extra alloc?

Above is ugly. So I look at where the patch replaces things,

sk_psock_tls_strp_read(), this is TLS specific read hook so can't really
work in generic case anyways.

sk_psock_strp_read(), will you have UDP, AF_UNIX stream parsers? Do these
even work outside TCP cases.

For these ones: sk_psock_verdict_apply(), sk_psock_verdict_recv(),
sk_psock_backlog(), can't we just do some refactoring around their
hook points so we know the context. For example sk_psock_tls_verdict_apply
is calling sk_psock_skb_redirect(). Why not have a sk_psock_unix_redirect()
and a sk_psock_udp_redirect(). There are likely some optimizations we can
deploy this way. We've already don this for tls and sk_msg types for example.

Then the helpers will know their types by program type, just use the right
variants.

So not suggestiong if/else the checks so much as having per type hooks.

> 
> >
> > The use in tcf_classify_ingress is a miss case so not the common path. If
> > it is/was in the common path I would suggest we rip it out.
> >
> 
> Excellent point, what about nf_bridge_unshare()? It is a common path
> for bridge netfilter, which is also probably why skb ext was introduced
> (IIRC). secpath_set() seems on a common path for XFRM too.

Yeah not nice, but we don't use nf_bridge so doesn't bother me.

> 
> Are you suggesting to remove them all? ;)

>From the hotpath where I care about perfromance yes. 

> 
> Thanks.