[RFC 0/2] net/tls: don't block after strparser error

[RFC 0/2] net/tls: don't block after strparser error

[Jakub Kicinski]

Hi!

I was investigating the correct behaviour around small receive
window size and were slightly surprised to find that currently,
if the recv buffer is too small to fit the entire record, TLS will
report -EMSGSIZE once, and then block indefinitely on a subsequent
call.

Perhaps naively I'd think that the error should keep being
reported since the error condition is never cleared.

One way or the other I'd like to engrave the correct behaviour into
a selftest so I'd appreciate guidance on which one that is :)

This set makes the TLS socket keep reporting the strparser error
until it gets closed.

Jakub Kicinski (2):
  net/tls: don't clear socket error if strparser aborted
  selftests/tls: test the small receive buffer case

 net/tls/tls_sw.c                  |  7 +++++--
 tools/testing/selftests/net/tls.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

-- 
2.21.0

[RFC 1/2] net/tls: don't clear socket error if strparser aborted

[Jakub Kicinski]

If strparser encounters an error it reports it on the socket and
stops any further processing. TLS RX will currently pick up that
error code with sock_error(), and report it to user space once.
Subsequent read calls will block indefinitely.

Since the error condition is not cleared and processing is not
restarted it seems more correct to keep returning the error
rather than sleeping.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/tls/tls_sw.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index de7561d4cfa5..a9ca2dbe0531 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1245,8 +1245,11 @@ static struct sk_buff *tls_wait_data(struct sock *sk, struct sk_psock *psock,
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
 	while (!(skb = ctx->recv_pkt) && sk_psock_queue_empty(psock)) {
-		if (sk->sk_err) {
-			*err = sock_error(sk);
+		if (unlikely(sk->sk_err)) {
+			if (ctx->strp.stopped)
+				*err = -sk->sk_err;
+			else
+				*err = sock_error(sk);
 			return NULL;
 		}
 
-- 
2.21.0

[RFC 2/2] selftests/tls: test the small receive buffer case

[Jakub Kicinski]

TLS requires the entire record to fit into the receive buffer.
If user sets the receive buffer too low when larger record
arrives strparser returns EMSGSIZE.

Add a test case for this situation.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 tools/testing/selftests/net/tls.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 4c285b6e1db8..39dfbc4a3652 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -694,6 +694,34 @@ TEST_F(tls, recv_lowat)
 	EXPECT_EQ(memcmp(send_mem, recv_mem + 10, 5), 0);
 }
 
+TEST_F(tls, recv_rcvbuf)
+{
+	char send_mem[4096];
+	char recv_mem[4096];
+	int rcv_buf = 1024;
+
+	memset(send_mem, 0x1c, sizeof(send_mem));
+
+	EXPECT_EQ(setsockopt(self->cfd, SOL_SOCKET, SO_RCVBUF,
+			     &rcv_buf, sizeof(rcv_buf)), 0);
+
+	EXPECT_EQ(send(self->fd, send_mem, 512, 0), 512);
+	memset(recv_mem, 0, sizeof(recv_mem));
+	EXPECT_EQ(recv(self->cfd, recv_mem, sizeof(recv_mem), 0), 512);
+	EXPECT_EQ(memcmp(send_mem, recv_mem, 512), 0);
+
+	if (self->notls)
+		return;
+
+	EXPECT_EQ(send(self->fd, send_mem, 4096, 0), 4096);
+	memset(recv_mem, 0, sizeof(recv_mem));
+	EXPECT_EQ(recv(self->cfd, recv_mem, sizeof(recv_mem), 0), -1);
+	EXPECT_EQ(errno, EMSGSIZE);
+
+	EXPECT_EQ(recv(self->cfd, recv_mem, sizeof(recv_mem), 0), -1);
+	EXPECT_EQ(errno, EMSGSIZE);
+}
+
 TEST_F(tls, bidir)
 {
 	char const *test_str = "test_read";
-- 
2.21.0

Re: [RFC 1/2] net/tls: don't clear socket error if strparser aborted

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Sun,  6 Oct 2019 20:53:22 -0700

> If strparser encounters an error it reports it on the socket and
> stops any further processing. TLS RX will currently pick up that
> error code with sock_error(), and report it to user space once.
> Subsequent read calls will block indefinitely.
> 
> Since the error condition is not cleared and processing is not
> restarted it seems more correct to keep returning the error
> rather than sleeping.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

So I guess this is all about what the usual socket error code
semantics work, which is report-and-clear.

States which should signal errors are always checked in the
various code socket call paths and if necessary the socket
error is resignalled.

From what I'm seeing here, the issue is that the strparser stopped
state isn't resampled at the appropriate spot.  So I would rather
see that fixed rather than having socket errors become level
triggered in this special case.