netdev - Re: combining sockmap + ktls

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: <5dd222f214374_63b82b118b2685b42d@john-XPS-13-9370.notmuch>
Date:   Sun, 17 Nov 2019 20:49:54 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: combining sockmap + ktls

Willem de Bruijn wrote:
> > > 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.
> 
> Not the verdict program. The setup is as follows:
> 
>   client --> icept.1 -- (optionally ktls) --> icept.2 --> server
> 
> I'm trying to redirect on send from the client directly into the send
> side of the ktls socket, to avoid waking up the icept.1 process
> completely. The ktls enabled socket has no BPF programs associated.
> 

Ah ok. This is probably the least optimized case, we've focused mostly
on the ingress sk_msg case so far.

> >
> > >
> > >         @@ -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?
> 
> No, the stack trace I observed is
> 
>   tcp_bpf_sendmsg_redir
>     tcp_bpf_push_locked
>       tcp_bpf_push
>         kernel_sendpage_locked
>           sock->ops->sendpage_locked
> 
> which never tries tls_sw_sendpage. Perhaps the relevant part is the
> following in tcp_bpf_push?
> 
>                 if (has_tx_ulp) {
>                         flags |= MSG_SENDPAGE_NOPOLICY;
>                         ret = kernel_sendpage_locked(sk,
>                                                      page, off, size, flags);
>                 } else {
>                         ret = do_tcp_sendpages(sk, page, off, size, flags);
>                 }
> 

Got it, want to submit a fix? Or I can this is a bug.

> > > 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.
> 
> Good to know thanks. Then I won't spend too much time on this.
> 
> > >
> > > 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.
> 
> Same thing. Good to know I'm not crazy :)
> 
> > 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.
> 
> To be clear, I don't mean to pressure at all. Was just running through
> a variety of points in the option space. The sk_msg  plus redirect to
> egress of a ktls-enabled socket is the variant I'm most interested in.

No pressure I need to get my queue here out I've been sitting on it
for awhile.

> 
> Do let me know if there's anything I can help out with. Thanks for
> your quick answer!

Can you send out a fix for above sendpage_locked case?

Thanks,
John