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

[Sagi Grimberg]

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

Yes, I see.
When starting a listener, it saves inet->sk_data_ready in
svsk->sk_odata, and sets sk->sk_data_ready to svc_tcp_listen_data_ready.
then when accepting, it returns it to newsock->sk->sk_data_ready,

then only when adding the new socket, it sets sk_data_ready to
svc_data_ready...

This is what I agreed with Hannes, that nvmet reset the newsock
inherited sk callbacks, and only restore them after the handshake.

Thanks.