Re: [PATCH 15/18] nvmet-tcp: enable TLS handshake upcall

Chuck Lever III
2023-03-28T15:57:21Z

> On Mar 28, 2023, at 11:29 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
>>>>>> As the callbacks will be changed once TLS is activated, and we really should not attempt to run if sk_data_ready() points to another function,
>>>>>> as then the sk_user_data pointer will most likely be changed, too,
>>>>>> causing all sorts of issues.
>>>>>
>>>>> Umm, something is unclear to me. if nvmet_tcp_listen_data_ready is
>>>>> called doesn't it by definition mean that sk->sk_data_ready ==
>>>>> nvmet_tcp_listen_data_ready ?
>>>>>
>>>>> Are you talking about a situation where between
>>>>> nvmet_tcp_listen_data_ready is starting and until the
>>>>> sk->sk_callback_lock the data_ready cb (and the user data
>>>>> pointer) is changed?
>>>> No. Far simpler:
>>>> Starting kTLS will change the callbacks.
>>>> So sk->sk_data_ready will point to our callback before
>>>> the upcall, but to the kTLS version _after_ the upcall.
>>>> It typically doesn't matter, as we're setting it to
>>>> nvmet_tcp_data_ready() anyway.
>>>
>>> For ktls won't we set it to nvmet_tcp_data_ready only when
>>> the handshake is done?
>>>
>>>> But there might be the odd case where the data_ready
>>>> callback is invoked after kTLS has been started but before
>>>> we're setting it to nvmet_tcp_data_ready().
>>>
>>> What does nvmet_tcp_data_ready has to do with it?
>>> You are changing nvmet_tcp_listen_data_ready.
>>>
>>>> Then we cannot guarantee that sk_user_data really is set
>>>> to the 'queue' pointer, so we should skip the function.
>>>
>>> nvme_tcp_listen_data_ready was invoked, then sk->sk_data_ready
>>> is nvme_tcp_listen_data_ready. Are you referring to the case
>>> where the callback has changed before the read lock ?
>>>
>>> I would like to understand why svc_tcp_listen_data_ready doesn't
>>> have this race as well. Chuck?
>> svc doesn't alter the pointer address in the listener's
>> sk_data_ready field, and neither does kTLS.
>
> It is inherited from the parent socket sk. IIRC it is cloned in
> inet_reqsk_clone.

Ah. I meant the changes to support the use of kTLS sockets do
not alter this svc code.

>> For a connected socket, when the handshake has completed,
>> svc examines the socket for more work one last time before
>> deciding it's idle (a kind of simulated data_ready). That
>> should pick up work that arrived undetected.
>
> The question is how does a ktls socket never calls its inherited
> sk_data_ready (svc_tcp_listen_data_ready), which according to Hannes'
> explanation, should be the case if this socket was created as its
> offspring.

Have a look at svc_tcp_accept(). It resets the inherited callbacks
before setting up the socket.

svc_tcp_init() then sets sk_data_ready to our data_ready function.

--
Chuck Lever