Message-ID: <3c3f9b9d-0fef-fb62-25f8-c9f17ec43a69@novek.ru>
Date:   Wed, 18 Nov 2020 20:51:30 +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 16:23, Jakub Kicinski wrote:
> On Wed, 18 Nov 2020 02:47:24 +0000 Vadim Fedorenko wrote:
>>>>>> This behavior differs from plain TCP socket and
>>>>>> leads to special treating in user-space. Patch unpauses parser
>>>>>> directly if we have unparsed data in tcp receive queue.
>>>>> Sure, but why is the parser paused? Does it pause itself on FIN?
>>>> No, it doesn't start even once. The trace looks like:
>>>>
>>>> tcp_recvmsg is called
>>>> tcp_recvmsg returns 1 (Change Cipher Spec record data)
>>>> tls_setsockopt is called
>>>> tls_setsockopt returns
>>>> tls_recvmsg is called
>>>> tls_recvmsg returns 0
>>>> __strp_recv is called
>>>> stack
>>>>            __strp_recv+1
>>>>            tcp_read_sock+169
>>>>            strp_read_sock+104
>>>>            strp_work+68
>>>>            process_one_work+436
>>>>            worker_thread+80
>>>>            kthread+276
>>>>            ret_from_fork+34tls_read_size called
>>>>
>>>> So it looks like strp_work was scheduled after tls_recvmsg and
>>>> nothing triggered parser because all the data was received before
>>>> tls_setsockopt ended the configuration process.
>>> Um. That makes me think we need to flush_work() on the strparser after
>>> we configure rx tls, no? Or __unpause at the right time instead of
>>> dealing with the async nature of strp_check_rcv()?
>> I'm not sure that flush_work() will do right way in this case. Because:
>> 1. The work is idle after tls_sw_strparser_arm()
> Not sure what you mean by idle, it queues the work via strp_check_rcv().
>
>> 2. The strparser needs socket lock to do it's work - that could be a
>> deadlock because setsockopt_conf already holds socket lock. I'm not
>> sure that it's a good idea to unlock socket just to wait the strparser.
> Ack, I meant in do_tls_setsockopt() after the lock is released.
>
>> 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?

Powered by blists - more mailing lists