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

Hannes Reinecke
2023-03-27T06:20:22Z

On 3/26/23 09:18, 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.
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().
Then we cannot guarantee that sk_user_data really is set
to the 'queue' pointer, so we should skip the function.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman