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

[Hannes Reinecke <hare@suse.de>]

On 3/28/23 11:43, Sagi Grimberg wrote:
> 
>>>> 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.
>>>
>> This is not a 'race;
>>>> 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 ?
>>>
>> No. The callback is changed when the userspace daemon starts ktls, as
>> that will set sk->sk_data_ready to tls_data_ready().
>>
>> So before the userspace upcall we have the chain:
>>
>> sk_data_ready -> nvmet_tcp_listen_data_ready()
>>
>> and after a (successful) upcall we have the chain
>>
>> sk_data_ready -> tls_data_ready
>>    -> nvmet_tcp_listen_data_ready()
>>
>> once we set our callback we have
>>
>> sk_data_ready -> nvmet_tcp_data_ready
>>    -> tls_data_ready
>>      -> nvmet_tcp_listen_data_ready
>>
>> Calling into nvmet_tcp_listen_data_ready() is pointless here,
>> but we cannot remove the callback either. So we should to deactivate
>> it to avoid any side effects.
> 
> Thanks for the explanation.
> 
> First, I would like to understand how is this different from
> svc.
> 
> Second, we don't want nvmet_tcp_listen_data_ready keep being
> called from the datapath, especially as it is taking a read
> lock, for absolutly no good reason.
> 
> Why don't we restore the original socket callback before
> we trigger the handshake? then we wait for the handshake
> to complete, and then store nvmet_tcp_data_ready once it
> is done?

Yep, that works.

Will be updating the patch.

Cheers,

Hannes