Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Hannes Reinecke <hare@suse.de>]

On 4/8/24 23:21, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 9:21, Hannes Reinecke wrote:
>> On 4/7/24 23:46, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> When secure concatenation is requested the connection needs to be
>>>> reset to enable TLS encryption on the new cnnection.
>>>> That implies that the original connection used for the DH-CHAP
>>>> negotiation really shouldn't be used, and we should reset as soon
>>>> as the DH-CHAP negotiation has succeeded on the admin queue.
>>>> The current implementation does not allow to easily skip
>>>> connection attempts on the I/O queues, so we connect I/O
>>>> queues, but disable namespace scanning on these queues.
>>>> With that no I/O can be issued on these queues, so we
>>>> can tear them down quickly without having to wait for
>>>> quiescing etc.
>>>
>>> We shouldn't have to connect io queues here. The scan prevention
>>> is just a hack...
>>>
>> Oh, believe me, I tried. What _should_ be possible is to create a
>> controller with just admin queues, and skip the io queue setup part.
>> But once you do that it's impossible to create io queues after reset
>> (which is what you'd need here).
>> I tried to fix this, but that ended up with a massive rewrite/reordering
>> of the init path. Which sure has its merits, but I deemed out of scope
>> for this patchset.
>>
>> So I decided for this 'hack', and shelved the init path reorg for a
>> later patchset.
> 
> Interesting that it requires a full rewrite. Can you share some info
> what particularly breaks when you change the nr_io_queues between
> resets? nr_io_queues can be changed across resets from the controller side
> as well.
> 
Just look at the 'new' parameter to nvme_tcp_configure_io_queues(),
and try to modify the code to _not_ required the parameter.
Key observation here is that the _size_ of the I/O tagset is actually
defined by the parameters for the connect command; we cannot grow
beyond that. Which means that we should be allocate the I/O tagset
right at the start, and only modify the number of queue which we _use_
based on the information we get back from the controller.
Currently the io tagset allocation is tied to the number of queues
for the controller, and trying to modify that is an even worse hack
than suppressing the namespace scan.

Anyway,it's quite a different patchset which is basically unrelated
to the secure concatenation work.

I do have a preliminary patchset for this if there's interest.

>>
>>>> Once that is done we can reset the controller directly
>>>> after the ->create_ctrl() callback.
>>>
>>> Why not set opts->nr_io_queues = 0 for secure concatenation and
>>> setting it to the original value before resetting?
>>
>> See above. The io tagset is allocated in the init path _only_.
> 
> ok. So? What is the issue here? is this because ns scanning is checking
> the existence of a ctrl->tagset? I predicted that this check will be 
> problematic to assume the ctrl state in the future.
> 
> We can add a controller flag for this.
> 
Problem is that namespace scanning will create block devices, which will
create uevent, which will cause udev to start scan for signatures etc.
At the same time we're resetting the queue, causing all sorts of race 
conditions and timing issues for the outstanding I/Os.
And all of that for namespaces which have a questionable existence
as they are only valid for _encrypted_ connections.

>> And creating of the io tagset is tied with connecting the io queues.
> 
> Where exactly?
> 
Check the 'new' parameter for nvme_configure_io_queues().
That is being called _only_ when we have more than 1 queue.
And is doing both, allocating the io tagset _and_ issue a 'connect' call
on each queue. We'd need to split that into allocating the io tagset
(which should happen always) and the 'connect' call (which should be
done for non-concat connection or concat connections with TLS enabled).

Cheers,

Hannes

>>
>> The reset code requires the tagset to be present; it just reconnects
>> the io queues.
> 
> it connects io_queues if ctrl->queue_count > 1
> 
and allocates the io tagset ...

>> So either you do some trickery here like skipping connecting io queues
>> (or, indeed, skipping namespace scanning),
>> or you end up with a massive reshuffling of the init code path trying
>> to separate tagset creation from io queue connections.
> 
> Hmm, I'm not exactly sure what separation exactly is required here.

The tagset will always need to be allocated, but the 'connect' command 
should be skipped for non-TLS enabled secure concat connections.

Cheers,

Hannes