[PATCH 15/18] nvmet-tcp: enable TLS handshake upcall
Hannes Reinecke
hare at suse.de
Tue Mar 28 02:20:33 PDT 2023
On 3/28/23 10:44, Sagi Grimberg wrote:
>
>>>>
>>>> Nearly there.
>>>>
>>>> The actual patch would be:
>>>>
>>>> @@ -2031,10 +1988,16 @@ static void
>>>> nvmet_tcp_listen_data_ready(struct sock *sk)
>>>> trace_sk_data_ready(sk);
>>>>
>>>> read_lock_bh(&sk->sk_callback_lock);
>>>> + /* Ignore if the callback has been changed */
>>>> + if (sk->sk_data_ready != nvmet_tcp_listen_data_ready)
>>>> + goto out;
>>>> port = sk->sk_user_data;
>>>> if (!port)
>>>> goto out;
>>>>
>>>> + if (port->data_ready)
>>>> + port->data_ready(sk);
>>>> +
>>>> if (sk->sk_state == TCP_LISTEN)
>>>> queue_work(nvmet_wq, &port->accept_work);
>>>> out:
>>>>
>>>> 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?
>
Yes.
>> 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.
Cheers,
Hannes
More information about the Linux-nvme
mailing list