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

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]

Date:   Thu, 19 Nov 2020 00:26:52 +0000
From:   Vadim Fedorenko <vfedorenko@...ek.ru>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Boris Pismenny <borisp@...dia.com>,
        Aviad Yehezkel <aviadye@...dia.com>, netdev@...r.kernel.org
Subject: Re: [net] net/tls: missing received data after fast remote close

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;