Date:   Fri, 15 Nov 2019 14:21:17 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        bpf <bpf@...r.kernel.org>
Cc:     Network Development <netdev@...r.kernel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: RE: combining sockmap + ktls

Willem de Bruijn wrote:
> I've been playing with sockmap and ktls. They're fantastic tools.

Great, glad to get more eyes on it. Thanks.

> Combining them I did run into a few issues. Would like to understand
> whether (a) it's just me, else (b) whether these are known issues and
> (c) some feedback on an initial hacky patch.

There are still a few outstanding things I need to flush out of my
queue listed below.

> 
> My test [1] sets up an echo request/response between a client and
> server, optionally interposed by an "icept" guard process on each side
> and optionally enabling ktls between the icept processes.
> 
> Without ktls, most variants of interpositioning {iptables, iptables +
> splice(), iptables + sockmap splice, sk_msg to icept tx } work.
> 
> Only sk_msg redirection to icept ingress with BPF_F_INGRESS does not
> if the destination socket has a verdict program. I *think* this is
> intentional, judging from commit 552de9106882 ("bpf: sk_msg, fix
> socket data_ready events") explicitly ensuring that the process gets
> awoken on new data if a socket has a verdict program and another
> socket redirects to it, as opposed to passing it to the program.

Right.

> 
> For this workload, more interesting is sk_msg directly to icept
> egress, anyway. This works without ktls. Support for ktls is added in
> commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling"). The
> relevant callback function tls_sw_sendpage_locked was not immediately
> used and subsequently removed in commit cc1dbdfed023 ("Revert
> "net/tls: remove unused function tls_sw_sendpage_locked""). It appears
> to work once reverting that change, plus registering the function

I don't fully understand this. Are you saying a BPF_SK_MSG_VERDICT
program attach to a ktls socket is not being called? Or packets are
being dropped or ...? Or that the program doesn't work even with
just KTLS and no bpf involved. 

> 
>         @@ -859,6 +861,7 @@ static int __init tls_register(void)
> 
>                 tls_sw_proto_ops = inet_stream_ops;
>                 tls_sw_proto_ops.splice_read = tls_sw_splice_read;
>         +       tls_sw_proto_ops.sendpage_locked   = tls_sw_sendpage_locked,
> 
> and additionally allowing MSG_NO_SHARED_FRAGS:
> 
>          int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
>                                     int offset, size_t size, int flags)
>          {
>                if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>         -                     MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY))
>         +                     MSG_SENDPAGE_NOTLAST |
> MSG_SENDPAGE_NOPOLICY | MSG_NO_SHARED_FRAGS))
>                          return -ENOTSUPP;
> 

If you had added MSG_NO_SHARED_FRAGS to the existing tls_sw_sendpage
would that have been sufficient?

> and not registering parser+verdict programs on the destination socket.
> Note that without ktls this mode also works with such programs
> attached.

Right ingress + ktls is known to be broken at the moment. Also I have
plans to cleanup ingress side at some point. The current model is a
bit clumsy IMO. The workqueue adds latency spikes on the 99+
percentiles. At this point it makes the ingress side similar to the
egress side without a workqueue and with verdict+parser done in a
single program.

> 
> Lastly, sockmap splicing from icept ingress to egress (no sk_msg) also
> stops working when I enable ktls on the egress socket. I'm taking a
> look at that next. But this email is long enough already ;)

Yes this is a known bug I've got a set of patches to address this. I've
been trying to get to it for awhile now and just resolved a few other
things on my side so plan to do this Monday/Tuesday next week.

FWIW there is also a bugfix on the way to resolve a case where we
receive a FIN/RST flag after redirecting data causing dropped data.

> 
> Thanks for having a look!
> 
>   Willem
> 
> [1] https://github.com/wdebruij/kerneltools/tree/icept.2
> 
> probably more readable is the stack of commits, one per feature:
> 
>   c86c112 icept: initial client/server test
>   727a8ae icept: add iptables interception
>   60c34b2 icept: add splice interception
>   03a516a icept: add sockmap interception
>   c9c6103 icept: run client and server in cgroup
>   579bcae icept: add skmsg interception
>   e1b0d17 icept: add kTLS

Powered by blists - more mailing lists