Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Sagi Grimberg <sagi@grimberg.me>]

>>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>>> index d6022fa..57f9031 100644
>>>>> --- a/drivers/nvme/host/rdma.c
>>>>> +++ b/drivers/nvme/host/rdma.c
>>>>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>>>>       if (!changed) {
>>>>> -        /* state change failure is ok if we're in DELETING state */
>>>>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>>>>> +        /*
>>>>> +         * state change failure is ok if we're in DELETING state,
>>>>> +         * unless we're during creation of a new controller to
>>>>> +         * avoid use after free (ctrl refcount is not taken yet).
>>>>> +         */
>>>>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
>>>>
>>>> What state are we in if not DELETING in this case?
>>>
>>> We are in DELETING state.
>>>
>>> With this change any state failure triggers a warning (including 
>>> DELETING) if new is true.
>>>
>>> In my test I was in DELETING state and with new == true
>>>
>>> Trying to change state from DELETING to LIVE is not allowed at the 
>>> state machine.
>>
>> Why do we need a warning on that?
> 
> First, the comment "state change failure is ok if we're in DELETING 
> state" is wrong.

The WARN_ON_ONCE is designed to indicate that we failed to transition
the state but we don't understand why. Is that the case here?

> State change failure should not happen when creating a new controller, 
> because it may lead to controller use after free.

So we need to fix that, but the warning should be printed only when
we get an unexpected behavior from the flow perspective.

> Second, the warning already exists so I am only fixing it.

I don't understand what this is fixing.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme