[bpf PATCH 0/2] tls, sockmap, fixes for sk_wait_event

[bpf PATCH 0/2] tls, sockmap, fixes for sk_wait_event

[John Fastabend]

I have been testing ktls and sockmap lately and noticed that neither
was handling sk_write_space events correctly. We need to ensure
these events are pushed down to the lower layer in all cases to
handle the case where the lower layer sendpage call has called
sk_wait_event and needs to be woken up. Without this I see
occosional stalls of sndtimeo length while we wait for the
timeout value even though space is available.

Two fixes below. Thanks.

---

John Fastabend (2):
      tls: possible hang when do_tcp_sendpages hits sndbuf is full case
      bpf: sockmap: write_space events need to be passed to TCP handler


 kernel/bpf/sockmap.c |    3 +++
 net/tls/tls_main.c   |    9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

[bpf PATCH 1/2] tls: possible hang when do_tcp_sendpages hits sndbuf is full case

[John Fastabend]

Currently, the lower protocols sk_write_space handler is not called if
TLS is sending a scatterlist via  tls_push_sg. However, normally
tls_push_sg calls do_tcp_sendpage, which may be under memory pressure,
that in turn may trigger a wait via sk_wait_event. Typically, this
happens when the in-flight bytes exceed the sdnbuf size. In the normal
case when enough ACKs are received sk_write_space() will be called and
the sk_wait_event will be woken up allowing it to send more data
and/or return to the user.

But, in the TLS case because the sk_write_space() handler does not
wake up the events the above send will wait until the sndtimeo is
exceeded. By default this is MAX_SCHEDULE_TIMEOUT so it look like a
hang to the user (especially this impatient user). To fix this pass
the sk_write_space event to the lower layers sk_write_space event
which in the TCP case will wake any pending events.

I observed the above while integrating sockmap and ktls. It
initially appeared as test_sockmap (modified to use ktls) occasionally
hanging. To reliably reproduce this reduce the sndbuf size and stress
the tls layer by sending many 1B sends. This results in every byte
needing a header and each byte individually being sent to the crypto
layer.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/tls/tls_main.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 93c0c22..180b664 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -213,9 +213,14 @@ static void tls_write_space(struct sock *sk)
 {
 	struct tls_context *ctx = tls_get_ctx(sk);
 
-	/* We are already sending pages, ignore notification */
-	if (ctx->in_tcp_sendpages)
+	/* If in_tcp_sendpages call lower protocol write space handler
+	 * to ensure we wake up any waiting operations there. For example
+	 * if do_tcp_sendpages where to call sk_wait_event.
+	 */
+	if (ctx->in_tcp_sendpages) {
+		ctx->sk_write_space(sk);
 		return;
+	}
 
 	if (!sk->sk_write_pending && tls_is_pending_closed_record(ctx)) {
 		gfp_t sk_allocation = sk->sk_allocation;

Re: [bpf PATCH 1/2] tls: possible hang when do_tcp_sendpages hits sndbuf is full case

[Dave Watson]

On 08/22/18 08:37 AM, John Fastabend wrote:
> Currently, the lower protocols sk_write_space handler is not called if
> TLS is sending a scatterlist via  tls_push_sg. However, normally
> tls_push_sg calls do_tcp_sendpage, which may be under memory pressure,
> that in turn may trigger a wait via sk_wait_event. Typically, this
> happens when the in-flight bytes exceed the sdnbuf size. In the normal
> case when enough ACKs are received sk_write_space() will be called and
> the sk_wait_event will be woken up allowing it to send more data
> and/or return to the user.
> 
> But, in the TLS case because the sk_write_space() handler does not
> wake up the events the above send will wait until the sndtimeo is
> exceeded. By default this is MAX_SCHEDULE_TIMEOUT so it look like a
> hang to the user (especially this impatient user). To fix this pass
> the sk_write_space event to the lower layers sk_write_space event
> which in the TCP case will wake any pending events.
> 
> I observed the above while integrating sockmap and ktls. It
> initially appeared as test_sockmap (modified to use ktls) occasionally
> hanging. To reliably reproduce this reduce the sndbuf size and stress
> the tls layer by sending many 1B sends. This results in every byte
> needing a header and each byte individually being sent to the crypto
> layer.
> 
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

Super, thanks!

Acked-by: Dave Watson <davejwatson@fb.com>

[bpf PATCH 2/2] bpf: sockmap: write_space events need to be passed to TCP handler

[John Fastabend]

When sockmap code is using the stream parser it also handles the write
space events in order to handle the case where (a) verdict redirects
skb to another socket and (b) the sockmap then sends the skb but due
to memory constraints (or other EAGAIN errors) needs to do a retry.

But the initial code missed a third case where the
skb_send_sock_locked() triggers an sk_wait_event(). A typically case
would be when sndbuf size is exceeded. If this happens because we
do not pass the write_space event to the lower layers we never wake
up the event and it will wait for sndtimeo. Which as noted in ktls
fix may be rather large and look like a hang to the user.

To reproduce the best test is to reduce the sndbuf size and send
1B data chunks to stress the memory handling.

To fix this pass the event from the upper layer to the lower layer.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 kernel/bpf/sockmap.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 98e621a..1d092f3 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1427,12 +1427,15 @@ static void smap_tx_work(struct work_struct *w)
 static void smap_write_space(struct sock *sk)
 {
 	struct smap_psock *psock;
+	void (*write_space)(struct sock *sk);
 
 	rcu_read_lock();
 	psock = smap_psock_sk(sk);
 	if (likely(psock && test_bit(SMAP_TX_RUNNING, &psock->state)))
 		schedule_work(&psock->tx_work);
+	write_space = psock->save_write_space;
 	rcu_read_unlock();
+	write_space(sk);
 }
 
 static void smap_stop_sock(struct smap_psock *psock, struct sock *sk)

Re: [bpf PATCH 0/2] tls, sockmap, fixes for sk_wait_event

[Alexei Starovoitov]

On Wed, Aug 22, 2018 at 08:37:27AM -0700, John Fastabend wrote:
> I have been testing ktls and sockmap lately and noticed that neither
> was handling sk_write_space events correctly. We need to ensure
> these events are pushed down to the lower layer in all cases to
> handle the case where the lower layer sendpage call has called
> sk_wait_event and needs to be woken up. Without this I see
> occosional stalls of sndtimeo length while we wait for the
> timeout value even though space is available.
> 
> Two fixes below. Thanks.

for the set
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [bpf PATCH 0/2] tls, sockmap, fixes for sk_wait_event

[Daniel Borkmann]

On 08/22/2018 05:37 PM, John Fastabend wrote:
> I have been testing ktls and sockmap lately and noticed that neither
> was handling sk_write_space events correctly. We need to ensure
> these events are pushed down to the lower layer in all cases to
> handle the case where the lower layer sendpage call has called
> sk_wait_event and needs to be woken up. Without this I see
> occosional stalls of sndtimeo length while we wait for the
> timeout value even though space is available.
> 
> Two fixes below. Thanks.
> 
> ---
> 
> John Fastabend (2):
>       tls: possible hang when do_tcp_sendpages hits sndbuf is full case
>       bpf: sockmap: write_space events need to be passed to TCP handler
> 
> 
>  kernel/bpf/sockmap.c |    3 +++
>  net/tls/tls_main.c   |    9 +++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)

Applied to bpf, thanks John!