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

[Sagi Grimberg <sagi@grimberg.me>]

>>>>> 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.

> 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.

> Maybe I'm missing something?

I don't know yet.