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

[Sagi Grimberg <sagi@grimberg.me>]

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

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