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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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 ?

[...]

>> +++ 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 ..

-ck