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

[Israel Rukshin <israelr@mellanox.com>]

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().

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.


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