Re: [net] net/tls: missing received data after fast remote close

[Vadim Fedorenko]

On 18.11.2020 23:39, Jakub Kicinski wrote:
On Wed, 18 Nov 2020 20:51:30 +0000 Vadim Fedorenko wrote:
The async nature of parser is OK for classic HTTPS server/client case
because it's very good to have parsed record before actual call to recvmsg
or splice_read is done. The code inside the loop in tls_wait_data is slow
path - maybe just move the check and the __unpause in this slow path?
Yeah, looking closer this problem can arise after we start as well :/
How about we move the __unparse code into the loop tho? Seems like this
could help with latency. Right now AFAICT we get a TCP socket ready
notification, which wakes the waiter for no good reason and makes
strparser queue its work, the work then will call tls_queue() ->
data_ready waking the waiting thread second time this time with the
record actually parsed.

Did I get that right? Should the waiter not cancel the work on the
first wake up and just kick of the parsing itself?
I was thinking of the same solution too, but simple check of emptyness of
socket's receive queue is not working in case when we have partial record
in queue - __unpause will return without changing ctx->skb and still having
positive value in socket queue and we will have blocked loop until new data
is received or strp_abort_strp is fired because of timeout. If you could
suggest proper condition to break the loop it would be great.

Or probably I misunderstood what loop did you mean exactly?
Damn, you may be seeing some problem I'm missing again ;) Running
__unparse can be opportunistic, if it doesn't parse anything that's
fine. I was thinking:

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 95ab5545a931..6478bd968506 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1295,6 +1295,10 @@ static struct sk_buff *tls_wait_data(struct sock *sk, 
struct sk_psock *psock,
                         return NULL;
                 }
  
+               __strp_unpause(&ctx->strp);
+               if (ctx->recv_pkt)
+                       return ctx->recv_pkt;
+
                 if (sk->sk_shutdown & RCV_SHUTDOWN)
                         return NULL;
  
Optionally it would be nice if unparse cancelled the work if it managed
to parse the record out.
Oh, simple and fine solution. But is it better to unpause parser conditionally 
when
there is something in the socket queue? Otherwise this call will be just wasting
cycles. Maybe like this:

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 2fe9e2c..97c5f6e 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1295,6 +1295,12 @@ static struct sk_buff *tls_wait_data(struct sock *sk, 
struct sk_psock *psock,
                        return NULL;
                }

+               if (!skb_queue_empty(&sk->sk_receive_queue)) {
+                       __strp_unpause(&ctx->strp);
+                       if (ctx->recv_pkt)
+                               return ctx->recv_pkt;
+               }
+
                if (sk->sk_shutdown & RCV_SHUTDOWN)
                        return NULL;