[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[Israel Rukshin]

nvmet_rdma_ib_client .remove callback is using nvmet_rdma_delete_wq.
Therefore nvmet_rdma_delete_wq must be destroyed only after calling
to ib_unregister_client().
The bug may happens when unloading the module with open connections.

Fixes: 2acf70a ("nvmet-rdma: use a private workqueue for delete")
Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/rdma.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index bd265ac..9d75eb6 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1642,35 +1642,33 @@ static int __init nvmet_rdma_init(void)
 {
 	int ret;
 
+	nvmet_rdma_delete_wq = alloc_workqueue("nvmet-rdma-delete-wq",
+			WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
+	if (!nvmet_rdma_delete_wq)
+		return -ENOMEM;
+
 	ret = ib_register_client(&nvmet_rdma_ib_client);
 	if (ret)
-		return ret;
+		goto err_destroy_workqueue;
 
 	ret = nvmet_register_transport(&nvmet_rdma_ops);
 	if (ret)
 		goto err_ib_client;
 
-	nvmet_rdma_delete_wq = alloc_workqueue("nvmet-rdma-delete-wq",
-			WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
-	if (!nvmet_rdma_delete_wq) {
-		ret = -ENOMEM;
-		goto err_unreg_transport;
-	}
-
 	return 0;
 
-err_unreg_transport:
-	nvmet_unregister_transport(&nvmet_rdma_ops);
 err_ib_client:
 	ib_unregister_client(&nvmet_rdma_ib_client);
+err_destroy_workqueue:
+	destroy_workqueue(nvmet_rdma_delete_wq);
 	return ret;
 }
 
 static void __exit nvmet_rdma_exit(void)
 {
-	destroy_workqueue(nvmet_rdma_delete_wq);
 	nvmet_unregister_transport(&nvmet_rdma_ops);
 	ib_unregister_client(&nvmet_rdma_ib_client);
+	destroy_workqueue(nvmet_rdma_delete_wq);
 	WARN_ON_ONCE(!list_empty(&nvmet_rdma_queue_list));
 	ida_destroy(&nvmet_rdma_queue_ida);
 }
-- 
1.8.3.1

[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[Max Gurtovoy]

Sagi/Christoph/Keith,

please note that the problematic commit? "nvmet-rdma: use a private 
workqueue for delete" is only merged to nvme-4.20 so this fix can be 
squashed to it IMO.

-Max.

On 11/5/2018 6:23 PM, Israel Rukshin wrote:
> nvmet_rdma_ib_client .remove callback is using nvmet_rdma_delete_wq.
> Therefore nvmet_rdma_delete_wq must be destroyed only after calling
> to ib_unregister_client().
> The bug may happens when unloading the module with open connections.
>
> Fixes: 2acf70a ("nvmet-rdma: use a private workqueue for delete")
> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>   drivers/nvme/target/rdma.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index bd265ac..9d75eb6 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1642,35 +1642,33 @@ static int __init nvmet_rdma_init(void)
>   {
>   	int ret;
>   
> +	nvmet_rdma_delete_wq = alloc_workqueue("nvmet-rdma-delete-wq",
> +			WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
> +	if (!nvmet_rdma_delete_wq)
> +		return -ENOMEM;
> +
>   	ret = ib_register_client(&nvmet_rdma_ib_client);
>   	if (ret)
> -		return ret;
> +		goto err_destroy_workqueue;
>   
>   	ret = nvmet_register_transport(&nvmet_rdma_ops);
>   	if (ret)
>   		goto err_ib_client;
>   
> -	nvmet_rdma_delete_wq = alloc_workqueue("nvmet-rdma-delete-wq",
> -			WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
> -	if (!nvmet_rdma_delete_wq) {
> -		ret = -ENOMEM;
> -		goto err_unreg_transport;
> -	}
> -
>   	return 0;
>   
> -err_unreg_transport:
> -	nvmet_unregister_transport(&nvmet_rdma_ops);
>   err_ib_client:
>   	ib_unregister_client(&nvmet_rdma_ib_client);
> +err_destroy_workqueue:
> +	destroy_workqueue(nvmet_rdma_delete_wq);
>   	return ret;
>   }
>   
>   static void __exit nvmet_rdma_exit(void)
>   {
> -	destroy_workqueue(nvmet_rdma_delete_wq);
>   	nvmet_unregister_transport(&nvmet_rdma_ops);
>   	ib_unregister_client(&nvmet_rdma_ib_client);
> +	destroy_workqueue(nvmet_rdma_delete_wq);
>   	WARN_ON_ONCE(!list_empty(&nvmet_rdma_queue_list));
>   	ida_destroy(&nvmet_rdma_queue_ida);
>   }

[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[Sagi Grimberg]

> Sagi/Christoph/Keith,
> 
> please note that the problematic commit? "nvmet-rdma: use a private 
> workqueue for delete" is only merged to nvme-4.20 so this fix can be 
> squashed to it IMO.

I think we need to revert 2acf70a ("nvmet-rdma: use a private workqueue 
for delete") altogether because it never made any difference..

[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[Christoph Hellwig]

On Tue, Nov 06, 2018@07:15:31PM -0800, Sagi Grimberg wrote:
>
>> Sagi/Christoph/Keith,
>>
>> please note that the problematic commit? "nvmet-rdma: use a private 
>> workqueue for delete" is only merged to nvme-4.20 so this fix can be 
>> squashed to it IMO.
>
> I think we need to revert 2acf70a ("nvmet-rdma: use a private workqueue for 
> delete") altogether because it never made any difference..

Agreed.  And nvme-4.20 has been merged to mainline already, so we can't
squash anything.  I'll plan to revert it for now.

[PATCH] nvmet-rdma: Fix using unallocated nvmet_rdma_delete_wq

[Max Gurtovoy]

On 11/7/2018 10:20 AM, Christoph Hellwig wrote:
> On Tue, Nov 06, 2018@07:15:31PM -0800, Sagi Grimberg wrote:
>>> Sagi/Christoph/Keith,
>>>
>>> please note that the problematic commit? "nvmet-rdma: use a private
>>> workqueue for delete" is only merged to nvme-4.20 so this fix can be
>>> squashed to it IMO.
>> I think we need to revert 2acf70a ("nvmet-rdma: use a private workqueue for
>> delete") altogether because it never made any difference..
> Agreed.  And nvme-4.20 has been merged to mainline already, so we can't
> squash anything.  I'll plan to revert it for now.


so how we overcome the circular lock that was reported by Bart ?