[PATCH net 0/4] net/tls: two fixes for rx_list pre-handling

[PATCH net 0/4] net/tls: two fixes for rx_list pre-handling

[Jakub Kicinski]

Hi!

tls_sw_recvmsg() had been modified to cater better to async decrypt.
Partially read records now live on the rx_list. Data is copied from
this list before the old do {} while loop, and the not included
correctly in deciding whether to sleep or not and lowat threshold
handling. These modifications, unfortunately, added some bugs.

First patch fixes lowat - we need to calculate the threshold early
and make sure all copied data is compared to the threshold, not just
the freshly decrypted data.

Third patch fixes sleep - if data is picked up from rx_list and
no flags are set, we should not put the process to sleep, but
rather return the partial read.

Patches 2 and 4 add test cases for these bugs, both will cause
a sleep and test timeout before the fix.

Jakub Kicinski (4):
  net/tls: fix lowat calculation if some data came from previous record
  selftests/tls: test for lowat overshoot with multiple records
  net/tls: fix no wakeup on partial reads
  selftests/tls: add test for sleeping even though there is data

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

-- 
2.21.0

[PATCH net 1/4] net/tls: fix lowat calculation if some data came from previous record

[Jakub Kicinski]

If some of the data came from the previous record, i.e. from
the rx_list it had already been decrypted, so it's not counted
towards the "decrypted" variable, but the "copied" variable.
Take that into account when checking lowat.

When calculating lowat target we need to pass the original len.
E.g. if lowat is at 80, len is 100 and we had 30 bytes on rx_list
target would currently be incorrectly calculated as 70, even though
we only need 50 more bytes to make up the 80.

Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Tested-by: David Beckett <david.beckett@netronome.com>
---
 net/tls/tls_sw.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d93f83f77864..fc13234db74a 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1712,13 +1712,12 @@ int tls_sw_recvmsg(struct sock *sk,
 		copied = err;
 	}
 
-	len = len - copied;
-	if (len) {
-		target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
-		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-	} else {
+	if (len <= copied)
 		goto recv_end;
-	}
+
+	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
+	len = len - copied;
+	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 
 	do {
 		bool retain_skb = false;
@@ -1853,7 +1852,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		}
 
 		/* If we have a new message from strparser, continue now. */
-		if (decrypted >= target && !ctx->recv_pkt)
+		if (decrypted + copied >= target && !ctx->recv_pkt)
 			break;
 	} while (len);
 
-- 
2.21.0

[PATCH net 2/4] selftests/tls: test for lowat overshoot with multiple records

[Jakub Kicinski]

Set SO_RCVLOWAT and test it gets respected when gathering
data from multiple records.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
---
 tools/testing/selftests/net/tls.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 47ddfc154036..01efbcd2258c 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -575,6 +575,25 @@ TEST_F(tls, recv_peek_large_buf_mult_recs)
 	EXPECT_EQ(memcmp(test_str, buf, len), 0);
 }
 
+TEST_F(tls, recv_lowat)
+{
+	char send_mem[10] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+	char recv_mem[20];
+	int lowat = 8;
+
+	EXPECT_EQ(send(self->fd, send_mem, 10, 0), 10);
+	EXPECT_EQ(send(self->fd, send_mem, 5, 0), 5);
+
+	memset(recv_mem, 0, 20);
+	EXPECT_EQ(setsockopt(self->cfd, SOL_SOCKET, SO_RCVLOWAT,
+			     &lowat, sizeof(lowat)), 0);
+	EXPECT_EQ(recv(self->cfd, recv_mem, 1, MSG_WAITALL), 1);
+	EXPECT_EQ(recv(self->cfd, recv_mem + 1, 6, MSG_WAITALL), 6);
+	EXPECT_EQ(recv(self->cfd, recv_mem + 7, 10, 0), 8);
+
+	EXPECT_EQ(memcmp(send_mem, recv_mem, 10), 0);
+	EXPECT_EQ(memcmp(send_mem, recv_mem + 10, 5), 0);
+}
 
 TEST_F(tls, pollin)
 {
-- 
2.21.0

[PATCH net 3/4] net/tls: fix no wakeup on partial reads

[Jakub Kicinski]

When tls_sw_recvmsg() partially copies a record it pops that
record from ctx->recv_pkt and places it on rx_list.

Next iteration of tls_sw_recvmsg() reads from rx_list via
process_rx_list() before it enters the decryption loop.
If there is no more records to be read tls_wait_data()
will put the process on the wait queue and got to sleep.
This is incorrect, because some data was already copied
in process_rx_list().

In case of RPC connections process may never get woken up,
because peer also simply blocks in read().

I think this may also fix a similar issue when BPF is at
play, because after __tcp_bpf_recvmsg() returns some data
we subtract it from len and use continue to restart the
loop, but len could have just reached 0, so again we'd
sleep unnecessarily. That's added by:
commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling")

Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
Reported-by: David Beckett <david.beckett@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Tested-by: David Beckett <david.beckett@netronome.com>
---
 net/tls/tls_sw.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index fc13234db74a..960494f437ac 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1719,7 +1719,7 @@ int tls_sw_recvmsg(struct sock *sk,
 	len = len - copied;
 	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 
-	do {
+	while (len && (decrypted + copied < target || ctx->recv_pkt)) {
 		bool retain_skb = false;
 		bool zc = false;
 		int to_decrypt;
@@ -1850,11 +1850,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		} else {
 			break;
 		}
-
-		/* If we have a new message from strparser, continue now. */
-		if (decrypted + copied >= target && !ctx->recv_pkt)
-			break;
-	} while (len);
+	}
 
 recv_end:
 	if (num_async) {
-- 
2.21.0

[PATCH net 4/4] selftests/tls: add test for sleeping even though there is data

[Jakub Kicinski]

Add a test which sends 15 bytes of data, and then tries
to read 10 byes twice.  Previously the second read would
sleep indifinitely, since the record was already decrypted
and there is only 5 bytes left, not full 10.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
---
 tools/testing/selftests/net/tls.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 01efbcd2258c..278c86134556 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -442,6 +442,21 @@ TEST_F(tls, multiple_send_single_recv)
 	EXPECT_EQ(memcmp(send_mem, recv_mem + send_len, send_len), 0);
 }
 
+TEST_F(tls, single_send_multiple_recv_non_align)
+{
+	const unsigned int total_len = 15;
+	const unsigned int recv_len = 10;
+	char recv_mem[recv_len * 2];
+	char send_mem[total_len];
+
+	EXPECT_GE(send(self->fd, send_mem, total_len, 0), 0);
+	memset(recv_mem, 0, total_len);
+
+	EXPECT_EQ(recv(self->cfd, recv_mem, recv_len, 0), recv_len);
+	EXPECT_EQ(recv(self->cfd, recv_mem + recv_len, recv_len, 0), 5);
+	EXPECT_EQ(memcmp(send_mem, recv_mem, total_len), 0);
+}
+
 TEST_F(tls, recv_partial)
 {
 	char const *test_str = "test_read_partial";
-- 
2.21.0

Re: [PATCH net 0/4] net/tls: two fixes for rx_list pre-handling

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 24 May 2019 10:34:29 -0700

> tls_sw_recvmsg() had been modified to cater better to async decrypt.
> Partially read records now live on the rx_list. Data is copied from
> this list before the old do {} while loop, and the not included
> correctly in deciding whether to sleep or not and lowat threshold
> handling. These modifications, unfortunately, added some bugs.
> 
> First patch fixes lowat - we need to calculate the threshold early
> and make sure all copied data is compared to the threshold, not just
> the freshly decrypted data.
> 
> Third patch fixes sleep - if data is picked up from rx_list and
> no flags are set, we should not put the process to sleep, but
> rather return the partial read.
> 
> Patches 2 and 4 add test cases for these bugs, both will cause
> a sleep and test timeout before the fix.

Thanks for the test cases.

Series applied and queued up for -stable.