Re: [PATCH 6/7] nvme-rdma: Add warning on state change failure at nvme_rdma_setup_ctrl

[Israel Rukshin <israelr@mellanox.com>]

On 3/25/2020 2:19 AM, Sagi Grimberg wrote:
>
>> The transition to LIVE state should not fail in case of a new 
>> controller.
>> Moving to DELETING state before nvme_tcp_create_ctrl() allocates all the
>> resources may leads to NULL dereference at teardown flow (e.g., IO 
>> tagset,
>> admin_q, connect_q).
>>
>> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
>> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/nvme/host/rdma.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index c99a882..3ae3011 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1022,8 +1022,13 @@ 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 */
>> +        /*
>> +         * state change failure is ok if we're in DELETING state,
>> +         * unless we're during creation of a new controller to
>> +         * avoid races with teardown flow.
>> +         */
>>           WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>> +        WARN_ON_ONCE(new);
>>           ret = -EINVAL;
>
> I still don't understand this. Why are we warning on new controller?
>
> Are you saying that a the state change must succeed in case its
> a new controller? because if its expected to not succeed in case
> of a failure then there is no point of the warning.
>
> I am missing something here..
Yes, the state change must succeed in case of a new controller.

Moving to DELETING state on new controller is forbidden, to avoid races 
with delete flow.

The state machine allows us to move to DELETING state from CONNECTING state,

but the io tagest for example may not be allocated yet which may cause a 
crash at the delete function.


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