Re: [PATCH 1/1] nvme-rdma: destroy cm id before destroy qp to avoid use after free

From: Max Gurtovoy <mgurtovoy@nvidia.com>
To: Christoph Hellwig <hch@infradead.org>, Ruozhu Li <liruozhu@huawei.com>
Cc: <linux-nvme@lists.infradead.org>, <sagi@grimberg.me>
Subject: Re: [PATCH 1/1] nvme-rdma: destroy cm id before destroy qp to avoid use after free
Date: Thu, 9 Sep 2021 20:32:38 +0300	[thread overview]
Message-ID: <90356fc2-837c-0ba2-5fcf-d9e9dcc06dba@nvidia.com> (raw)
In-Reply-To: <YToUji9WujpL9vnt@infradead.org>

On 9/9/2021 5:05 PM, Christoph Hellwig wrote:
> This looks reasonable to me.  Sagi, Max: any comments?
>
> On Mon, Sep 06, 2021 at 11:51:34AM +0800, Ruozhu Li wrote:
>> We should always destroy cm_id before destroy qp to avoid to get cma
>> event after qp was destroyed, which may lead to use after free.

Do you really encounter use-after-free ?

I think the code looks correct but from experience with RDMA-CM I would 
like to know which tests did you run with this code ?

interesting tests are: port toggling with/without IO, unload 
mlx5/other_rdma_hw drivers during connection establishment or during IO, 
reboot target, reboot target during connection establishment, reboot 
target during IO.

Thanks.

>> In RDMA connection establishment error flow, don't destroy qp in cm
>> event handler.Just report cm_error to upper level, qp will be destroy
>> in nvme_rdma_alloc_queue() after destroy cm id.
>>
>> Signed-off-by: Ruozhu Li <liruozhu@huawei.com>
>> ---
>>   drivers/nvme/host/rdma.c | 16 +++-------------
>>   1 file changed, 3 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index a68704e39084..042c594bc57e 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -656,8 +656,8 @@ static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue)
>>   	if (!test_and_clear_bit(NVME_RDMA_Q_ALLOCATED, &queue->flags))
>>   		return;
>>   
>> -	nvme_rdma_destroy_queue_ib(queue);
>>   	rdma_destroy_id(queue->cm_id);
>> +	nvme_rdma_destroy_queue_ib(queue);
>>   	mutex_destroy(&queue->queue_lock);
>>   }
>>   
>> @@ -1815,14 +1815,10 @@ static int nvme_rdma_conn_established(struct nvme_rdma_queue *queue)
>>   	for (i = 0; i < queue->queue_size; i++) {
>>   		ret = nvme_rdma_post_recv(queue, &queue->rsp_ring[i]);
>>   		if (ret)
>> -			goto out_destroy_queue_ib;
>> +			return ret;
>>   	}
>>   
>>   	return 0;
>> -
>> -out_destroy_queue_ib:
>> -	nvme_rdma_destroy_queue_ib(queue);
>> -	return ret;
>>   }
>>   
>>   static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
>> @@ -1916,14 +1912,10 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
>>   	if (ret) {
>>   		dev_err(ctrl->ctrl.device,
>>   			"rdma_connect_locked failed (%d).\n", ret);
>> -		goto out_destroy_queue_ib;
>> +		return ret;
>>   	}
>>   
>>   	return 0;
>> -
>> -out_destroy_queue_ib:
>> -	nvme_rdma_destroy_queue_ib(queue);
>> -	return ret;
>>   }
>>   
>>   static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
>> @@ -1954,8 +1946,6 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
>>   	case RDMA_CM_EVENT_ROUTE_ERROR:
>>   	case RDMA_CM_EVENT_CONNECT_ERROR:
>>   	case RDMA_CM_EVENT_UNREACHABLE:
>> -		nvme_rdma_destroy_queue_ib(queue);
>> -		fallthrough;
>>   	case RDMA_CM_EVENT_ADDR_ERROR:
>>   		dev_dbg(queue->ctrl->ctrl.device,
>>   			"CM error event %d\n", ev->event);
>> -- 
>> 2.16.4
>>
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme
> ---end quoted text---

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