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

[Taehee Yoo <ap420073@gmail.com>]

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