Re: [PATCH 1/4] nvme: fix delete uninitialized controller

Taehee Yoo
2023-01-04T02:43:07Z

Hi Sagi and Chaitanya.
Thank you so much for the reviews!

On 1/4/23 09:24, Chaitanya Kulkarni wrote:
> On 1/3/23 02:30, Sagi Grimberg wrote:
>>> nvme-fabric controllers can be deleted by
>>> /sys/class/nvme/nvme<NS>/delete_controller
>>> echo 1 > /sys/class/nvme/nvme<NS>/delete_controller
>>> The above command will call nvme_delete_ctrl_sync().
>>> This function internally tries to change ctrl->state to
>>> NVME_CTRL_DELETING.
>>> NVME_CTRL_LIVE, NVME_CTRL_RESETTING, and NVME_CTRL_CONNECTING
states can
>>> be changed to NVME_CTRL_DELETING.
>>> If the state is successfully changed, nvme_do_delete_ctrl() is called,
>>> which is the actual delete logic of controller.
>>>
>>> controller initialization logic changes ctrl->state.
>>> NEW -> CONNECTING -> LIVE.
>>> NVME_CTRL_CONNECTING state doesn't ensure that initialization is done.
>>>
>>> So, delete logic can be called before the finish of controller
>>> initialization.
>>> So kernel panic would occur because nvme_do_delete_ctrl() dereferences
>>> uninitialized values.
>
> thanks for discovering this, do you perhaps have sequence of commands to
> reproduce this ?
>

Yes, the below reproducer would be helpful.

#TARGET
sudo modprobe nvme_tcp
sudo modprobe nvmet
sudo modprobe nvmet-tcp
sudo mkdir /sys/kernel/config/nvmet/subsystems/nvmet-test-2
cd /sys/kernel/config/nvmet/subsystems/nvmet-test-2
echo 1 |sudo tee -a attr_allow_any_host > /dev/null
sudo mkdir namespaces/2
cd namespaces/2/
echo -n /dev/<NVME DEVICE> device_path
echo 1 > enable
sudo mkdir /sys/kernel/config/nvmet/ports/2
cd /sys/kernel/config/nvmet/ports/2
echo <TARGET IP> |sudo tee -a addr_traddr > /dev/null
echo tcp|sudo tee -a addr_trtype > /dev/null
echo 4002 |sudo tee -a addr_trsvcid > /dev/null
echo ipv4|sudo tee -a addr_adrfam > /dev/null
sudo ln -s /sys/kernel/config/nvmet/subsystems/nvmet-test-2/
/sys/kernel/config/nvmet/ports/2/subsystems/nvmet-t

#HOST SHELL1
while :
do
nvme discover -t tcp -a <TARGET ADDRESS> -s 4002
nvme connect -t tcp -n nvmet-test-2 -a <TARGET ADDRESS> -s 4002
done

#HOST SHELL2
while :
do
echo 1 > /sys/class/nvme/nvme0/delete_controller
done

#HOST SHELL3
#This additional test script is to reproduce the second patch issue.
while :
do
echo 1 > /sys/class/nvme/nvme0/reset_controller
done

> [...]
>
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -243,7 +243,8 @@ static void nvme_delete_ctrl_sync(struct nvme_ctrl
>>> *ctrl)
>>> * since ->delete_ctrl can free the controller.
>>> */
>>> nvme_get_ctrl(ctrl);
>>> - if (nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
>>> + if (test_bit(NVME_CTRL_STARTED_ONCE, &ctrl->flags) &&
>>> + nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
>>> nvme_do_delete_ctrl(ctrl);
>>
>> So what is the outcome now? if the controller kept on dangling? what
>> triggers the controller deletion?
>>
>>> nvme_put_ctrl(ctrl);
>>> }
>>
>> I don't think this is the correct approach.
>> the delete should fully fence the initialization and then delete
>> the controller.
>>
>> In this case, the transport driver should not quiesce a non-existent
>> queue.
>>
>> If further synchronization is needed, then it should be added so that
>> delete will fully fence the initialization.
>
> as stated here I'd add complete fencing for the initialization and
> delete transition ..

I thought that delete/reset logic should not be allowed before the
finish of initialization and I didn't consider that delete/reset logic
fence initialization logic.
That was my approach, but after review, I think this approach is not fit
for the nvme subsystem.
I think this case will be fixed by Chaitanya correctly.
I hope the above reproducer is helpful.

So, I will drop 1, and 2 patches in the v2 patchset.

>
> -ck
>

Thanks a lot!
Taehee Yoo