Hi Sagi,

Regarding the first issue, you are right.
we missed that ctrl->queues array allocation is capped by opts->nr_io_queues,

However, we do believe that the second issue does exist:
As part of create_ctrl
queue_count == nr_io_queues + 1 == 49 (in our example 48 = num_online_cpus())

As part of nvme_tcp_setup_ctrl,
alloc_io_queues calls set_queue_count where nr_io_queues is set to 8 (in our case, target ctrl supports up to 8 io queues)
ctrl->queue_count == 8 + 1

Timeout occurs and reconnect:
After reconnect, target ctrl is able to support up to 128 queues.
alloc_io_queues set nr_io_queues to 48  and ctrl->queue_count == 49

after 8 iterations, start_io_queues  calls nvme_tcp_start_queue with qid 9
nvmf_connect_io_queue is being called with qid 9
__nvme_submit_sync_cmd is calling the block layer (blk_mq_alloc_request_hctx) with qid 9 which returns with an error

Thanks,
Amit

-----Original Message-----
From: Sagi Grimberg <sagi@grimberg.me> 
Sent: Wednesday, December 8, 2021 1:33 PM
To: Engel, Amit; linux-nvme@lists.infradead.org
Cc: Anner, Ran
Subject: Re: nvme-tcp host potential bugs


[EXTERNAL EMAIL] 

Hello Sagi,

We would like to share and hear your inputs regarding 2 host nvme-tcp 
issues that we have encountered in kernel version 
4.18.0-348.2.1.el8_5.x86_64

First issue:
As part of nvme_tcp_create_ctrl, ctrl->queues is being allocated per 
ctrl in size of queue_count Ctrl queue_count is being set as part of 
nvme_tcp_alloc_io_queues per nr_io_queues

ctrl->queues array is being allocated just once, in create_ctrl, and is
capped by opts->nr_io_queues, and should never exceed this size.

We see a potential issue in the following scenario:
A connection is being established with x I/O queues and ctrl->queues 
is allocated to be of size x + 1

This means that opts->nr_io_queues == x

Assuming there is a reconnection (due to a timeout or any other 
reason) The new connection is being established with y I/O queues 
(where y > x)

That should not be possible. It can be that the controller refused to accept all the queues that the host asked for, which means that user wants z queues, controller accepted the first go x queues and in the second go y queues where z <= y < x (according to your example).

ctrl->queues size is z
first round the host connected x of these queues, and in the second round the host connected y of the queues.

In this case, ctrl->queues was previously allocated with queue_count x 
+ 1 But now queue_count is being updated to y + 1 As part of 
nvme_tcp_alloc_queue, we have struct nvme_tcp_queue *queue = 
&ctrl->queues[qid]; which might lead to access an out of range memory 
location (When qid > x + 1]) again, ctrl->queues was allocated with 
queue_count == x + 1 and not y

To prove the above theory, we added some debug prints when using x == 8 and y == 48:

#creating 8 I/O queues, queue_count == 9, queues points to 
00000000fd0a0f0f

Yes, but what is the array size?

Nov 30 14:02:25 nc9127122.drm.lab.emc.com kernel: nvme nvme15: 
creating 8 I/O queues. queues 00000000fd0a0f0f queue_count 9 Nov 30 14:02:25 nc9127122.drm.lab.emc.com kernel: nvme nvme15: mapped 8/0/0 default/read/poll queues.
Nov 30 14:02:25 nc9127122.drm.lab.emc.com kernel: nvme nvme15: 
Successfully reconnected (1 attempt)

#Timeout occurs that leads to reconnecting:
Nov 30 14:02:42 nc9127122.drm.lab.emc.com kernel: nvme nvme15: queue 
0: timeout request 0x0 type 4 Nov 30 14:02:42 
nc9127122.drm.lab.emc.com kernel: nvme nvme15: starting error recovery 
Nov 30 14:02:42 nc9127122.drm.lab.emc.com kernel: nvme nvme15: failed nvme_keep_alive_end_io error=10 Nov 30 14:02:42 nc9127122.drm.lab.emc.com kernel: nvme nvme15: Reconnecting in 10 seconds...

#Creating 48 I/O queues, queue_count == 49, queues points again to 
00000000fd0a0f0f

again, what is the array size?

Nov 30 14:02:52 nc9127122.drm.lab.emc.com kernel: nvme nvme15: 
creating 48 I/O queues. queues 00000000fd0a0f0f queue_count 49

Second issue:
With the same above example , where x <  y As part of the reconnection 
process, nvme_tcp_configure_io_queues is being called In this 
function, nvme_tcp_start_io_queues is being called with the new (y) 
queue_count Which will lead to an error (when sending the IO connect 
command to the block layer)

But the host won't connect x queues, it will only connect y queues.
Maybe I'm missing something?

Thanks,
Amit


Internal Use - Confidential

I'm assuming that this is not confidential as you are posting this to Linux-nvme, so please drop this notice from your upstream mails.