sk_data_ready callback restore

sk_data_ready callback restore

[Hannes Reinecke]

Hi Chuck,

some good news; my prototype is up and running and seems to be happy 
with the infrastructure:

nvme nvme0: connecting queue 0
nvme nvme0: queue 0: start TLS with key 389e6cd5 keyring 23836d8b
nvmet_tcp: queue 0 hdr type 22 hlen 1 plen 33554719 size 128
nvmet_tcp: queue 0: TLS ServerHello
nvmet_tcp: queue 0 wakeup userspace
nvme nvme0: queue 0: TLS handshake done, key 389e6cd5, status 0
nvmet_tcp: queue 0: TLS handshake done, key 389e6cd5, status 0

but there's one question: in order to get this to work I had to
set the sk_data_ready() callback before starting the TLS handshake.

queue->data_ready = sk->sk_data_ready;
sk->sk_data_ready = nvmet_tcp_tls_data_ready;

That works, but it's getting tricky once the TLS handshake is done.
Naively I would have thought that I need to reset the sk_data_ready()
callback

sk->sk_data_ready = queue->data_ready;
queue->data_ready = NULL;

But as I'm paranoid I've check whether I'll be resetting the correct
sk_data_ready() callback:

WARN_ON(sk->sk_data_ready != nvmet_tcp_tls_data_ready);

before resetting.
And, surprise, surprise, that WARN_ON triggers.
Probably understandable, as kTLS is changing the callbacks, too.

Leaving the question: how do I _disable_ my sk_data_ready() callback?
Just blindly reassigning the original callback clearly is not a good 
idea, but doing nothing would mean that my callback will be left in 
place during the entire connection, which also is not a good idea.
So what is the correct way of disabling the callback?

Cheers,

Hannes

Re: sk_data_ready callback restore

[Chuck Lever III]

> On Mar 16, 2023, at 7:26 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> Hi Chuck,
> 
> some good news; my prototype is up and running and seems to be happy with the infrastructure:
> 
> nvme nvme0: connecting queue 0
> nvme nvme0: queue 0: start TLS with key 389e6cd5 keyring 23836d8b
> nvmet_tcp: queue 0 hdr type 22 hlen 1 plen 33554719 size 128
> nvmet_tcp: queue 0: TLS ServerHello
> nvmet_tcp: queue 0 wakeup userspace
> nvme nvme0: queue 0: TLS handshake done, key 389e6cd5, status 0
> nvmet_tcp: queue 0: TLS handshake done, key 389e6cd5, status 0
> 
> but there's one question: in order to get this to work I had to
> set the sk_data_ready() callback before starting the TLS handshake.
> 
> queue->data_ready = sk->sk_data_ready;
> sk->sk_data_ready = nvmet_tcp_tls_data_ready;
> 
> That works, but it's getting tricky once the TLS handshake is done.
> Naively I would have thought that I need to reset the sk_data_ready()
> callback
> 
> sk->sk_data_ready = queue->data_ready;
> queue->data_ready = NULL;
> 
> But as I'm paranoid I've check whether I'll be resetting the correct
> sk_data_ready() callback:
> 
> WARN_ON(sk->sk_data_ready != nvmet_tcp_tls_data_ready);
> 
> before resetting.
> And, surprise, surprise, that WARN_ON triggers.
> Probably understandable, as kTLS is changing the callbacks, too.

Socket callbacks form a chain. Each module wanting a callback
saves the callback function value, then updates the socket's
callback function pointer to point to its callback.

Then, when it's data_ready callback function is invoked, it
calls the saved callback function first, then does whatever
it needs to do, and returns.


> Leaving the question: how do I _disable_ my sk_data_ready() callback?
> Just blindly reassigning the original callback clearly is not a good idea, but doing nothing would mean that my callback will be left in place during the entire connection, which also is not a good idea.
> So what is the correct way of disabling the callback?

The way I've done it is to add an atomic flag that is set
only during handshakes. When RPC's sk_data_ready callback
function is called, if that flag is set, just call the
saved callback and then return immediately without
doing any RPC receive processing.

Here's the RPC client side that demonstrates both how
callback chaining works and how I disable the function
during a handshake:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/tree/net/sunrpc/xprtsock.c?h=topic-rpc-with-tls-upcall#n1436


--
Chuck Lever

Re: sk_data_ready callback restore

[Hannes Reinecke]

On 3/16/23 14:35, Chuck Lever III wrote:
> 
> 
>> On Mar 16, 2023, at 7:26 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> Hi Chuck,
>>
>> some good news; my prototype is up and running and seems to be happy with the infrastructure:
>>
>> nvme nvme0: connecting queue 0
>> nvme nvme0: queue 0: start TLS with key 389e6cd5 keyring 23836d8b
>> nvmet_tcp: queue 0 hdr type 22 hlen 1 plen 33554719 size 128
>> nvmet_tcp: queue 0: TLS ServerHello
>> nvmet_tcp: queue 0 wakeup userspace
>> nvme nvme0: queue 0: TLS handshake done, key 389e6cd5, status 0
>> nvmet_tcp: queue 0: TLS handshake done, key 389e6cd5, status 0
>>
>> but there's one question: in order to get this to work I had to
>> set the sk_data_ready() callback before starting the TLS handshake.
>>
>> queue->data_ready = sk->sk_data_ready;
>> sk->sk_data_ready = nvmet_tcp_tls_data_ready;
>>
>> That works, but it's getting tricky once the TLS handshake is done.
>> Naively I would have thought that I need to reset the sk_data_ready()
>> callback
>>
>> sk->sk_data_ready = queue->data_ready;
>> queue->data_ready = NULL;
>>
>> But as I'm paranoid I've check whether I'll be resetting the correct
>> sk_data_ready() callback:
>>
>> WARN_ON(sk->sk_data_ready != nvmet_tcp_tls_data_ready);
>>
>> before resetting.
>> And, surprise, surprise, that WARN_ON triggers.
>> Probably understandable, as kTLS is changing the callbacks, too.
> 
> Socket callbacks form a chain. Each module wanting a callback
> saves the callback function value, then updates the socket's
> callback function pointer to point to its callback.
> 
> Then, when it's data_ready callback function is invoked, it
> calls the saved callback function first, then does whatever
> it needs to do, and returns.
> 
Yes, understood; that's what my prototype does.

> 
>> Leaving the question: how do I _disable_ my sk_data_ready() callback?
>> Just blindly reassigning the original callback clearly is not a good idea, but doing nothing would mean that my callback will be left in place during the entire connection, which also is not a good idea.
>> So what is the correct way of disabling the callback?
> 
> The way I've done it is to add an atomic flag that is set
> only during handshakes. When RPC's sk_data_ready callback
> function is called, if that flag is set, just call the
> saved callback and then return immediately without
> doing any RPC receive processing.
> 
Which is what I ended up doing; it felt slightly odd as 'normal' 
programming rules would imply that any action you do should be reverted 
once you're done.
Which doesn't work here, as essentially it's impossible to restore
the callback.

But if that's the way it should be I'm fine.
Just wanted to clarify.

Cheers,

Hannes

Re: sk_data_ready callback restore

[Chuck Lever III]

> On Mar 16, 2023, at 9:40 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 3/16/23 14:35, Chuck Lever III wrote:
>>> On Mar 16, 2023, at 7:26 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> Hi Chuck,
>>> 
>>> some good news; my prototype is up and running and seems to be happy with the infrastructure:
>>> 
>>> nvme nvme0: connecting queue 0
>>> nvme nvme0: queue 0: start TLS with key 389e6cd5 keyring 23836d8b
>>> nvmet_tcp: queue 0 hdr type 22 hlen 1 plen 33554719 size 128
>>> nvmet_tcp: queue 0: TLS ServerHello
>>> nvmet_tcp: queue 0 wakeup userspace
>>> nvme nvme0: queue 0: TLS handshake done, key 389e6cd5, status 0
>>> nvmet_tcp: queue 0: TLS handshake done, key 389e6cd5, status 0
>>> 
>>> but there's one question: in order to get this to work I had to
>>> set the sk_data_ready() callback before starting the TLS handshake.
>>> 
>>> queue->data_ready = sk->sk_data_ready;
>>> sk->sk_data_ready = nvmet_tcp_tls_data_ready;
>>> 
>>> That works, but it's getting tricky once the TLS handshake is done.
>>> Naively I would have thought that I need to reset the sk_data_ready()
>>> callback
>>> 
>>> sk->sk_data_ready = queue->data_ready;
>>> queue->data_ready = NULL;
>>> 
>>> But as I'm paranoid I've check whether I'll be resetting the correct
>>> sk_data_ready() callback:
>>> 
>>> WARN_ON(sk->sk_data_ready != nvmet_tcp_tls_data_ready);
>>> 
>>> before resetting.
>>> And, surprise, surprise, that WARN_ON triggers.
>>> Probably understandable, as kTLS is changing the callbacks, too.
>> Socket callbacks form a chain. Each module wanting a callback
>> saves the callback function value, then updates the socket's
>> callback function pointer to point to its callback.
>> Then, when it's data_ready callback function is invoked, it
>> calls the saved callback function first, then does whatever
>> it needs to do, and returns.
> Yes, understood; that's what my prototype does.
> 
>>> Leaving the question: how do I _disable_ my sk_data_ready() callback?
>>> Just blindly reassigning the original callback clearly is not a good idea, but doing nothing would mean that my callback will be left in place during the entire connection, which also is not a good idea.
>>> So what is the correct way of disabling the callback?
>> The way I've done it is to add an atomic flag that is set
>> only during handshakes. When RPC's sk_data_ready callback
>> function is called, if that flag is set, just call the
>> saved callback and then return immediately without
>> doing any RPC receive processing.
> Which is what I ended up doing; it felt slightly odd as 'normal' programming rules would imply that any action you do should be reverted once you're done.
> Which doesn't work here, as essentially it's impossible to restore
> the callback.

Changing the callback function addresses while the socket
is in use is racy, and also you have to preserve the callback
chain.


> But if that's the way it should be I'm fine.
> Just wanted to clarify.
> 
> Cheers,
> 
> Hannes

--
Chuck Lever