Re: [PATCH] nvmet-rdma: Fix double free of rdma queue

[Sagi Grimberg <sagi@grimberg.me>]

On 3/30/20 2:37 AM, Israel Rukshin wrote:
> On 3/30/2020 11:56 AM, Sagi Grimberg wrote:
>>
>>>>> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
>>>>> index 37d262a..59209e3 100644
>>>>> --- a/drivers/nvme/target/rdma.c
>>>>> +++ b/drivers/nvme/target/rdma.c
>>>>> @@ -1380,13 +1380,14 @@ static void 
>>>>> nvmet_rdma_queue_connect_fail(struct rdma_cm_id *cm_id,
>>>>>   {
>>>>>       WARN_ON_ONCE(queue->state != NVMET_RDMA_Q_CONNECTING);
>>>>>   +    pr_err("failed to connect queue %d\n", queue->idx);
>>>>> +
>>>>>       mutex_lock(&nvmet_rdma_queue_mutex);
>>>>> -    if (!list_empty(&queue->queue_list))
>>>>> +    if (!list_empty(&queue->queue_list)) {
>>>>>           list_del_init(&queue->queue_list);
>>>>> +        schedule_work(&queue->release_work);
>>>>
>>>> This has a hidden assumption that the connect handler already
>>>> scheduled the release.
>>>>
>>>> Why don't we simply not queue the release_work in the accept
>>>> failure and return a negative status code to implicitly remove the
>>>> cm_id? this way we will never see any cm events and we don't
>>>> need to handle it.
>>>
>>> This changes the flow but I guess we can check this out.
>>>
>>> But still, this flow can be called from 3 different events 
>>> (RDMA_CM_EVENT_REJECTED, RDMA_CM_EVENT_UNREACHABLE, 
>>> RDMA_CM_EVENT_CONNECT_ERROR) so I prefer to locate the schedule_work 
>>> under the "if".
>>
>> This if is only checked in connect error. Anyway, if you feel that
>> this flow is racy, perhaps implement a proper serialization, instead
>> of checking a random "if" that makes the reader think why are they
>> even related.
> 
> This "if" is exactly like we are doing at nvmet_rdma_queue_disconnect().

You're right.

> All the other places before calling __nvmet_rdma_queue_disconnect() 
> delete the queue from the list.
> 
> So I guess my change also protect us from races with 
> nvmet_rdma_delete_ctrl/nvmet_rdma_remove_one.
> 
> Beside that, why do we need to check if the list is not empty before 
> removing it from the list at nvmet_rdma_queue_connect_fail()?
> 
> I don't see a reason why to remove only the queue from the list without 
> schedule the release work.

That is fine with me, assuming we have a proper comment.

But if we take a step back, nvmet_rdma_create_queue_ib does not create
the cm_id, so why should destroy_queue_ib destroy it?

What if we made destroying the cm_id in release_work (out of 
nvmet_rdma_free_queue) and have the accept error path return a normal
negative ret to implicitly destroy the cm_id?

In a sense, that would make the behavior symmetric. Thoughts?

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