linux-kernel - Re: [PATCH] RDMA/device: Fix a race between mad_client and cm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240103184804.GB50608@ziepe.ca>
Date: Wed, 3 Jan 2024 14:48:04 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Shifeng Li <lishifeng@...gfor.com.cn>
Cc: leon@...nel.org, wenglianfa@...wei.com, gustavoars@...nel.org,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
	Shifeng Li <lishifeng1992@....com>
Subject: Re: [PATCH] RDMA/device: Fix a race between mad_client and cm_client
 init

On Mon, Jan 01, 2024 at 07:43:35PM -0800, Shifeng Li wrote:
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 67bcea7a153c..85782786993d 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1315,12 +1315,6 @@ static int enable_device_and_get(struct ib_device *device)
>  	down_write(&devices_rwsem);
>  	xa_set_mark(&devices, device->index, DEVICE_REGISTERED);
>  
> -	/*
> -	 * By using downgrade_write() we ensure that no other thread can clear
> -	 * DEVICE_REGISTERED while we are completing the client setup.
> -	 */
> -	downgrade_write(&devices_rwsem);
> -
>  	if (device->ops.enable_driver) {
>  		ret = device->ops.enable_driver(device);
>  		if (ret)
> @@ -1337,7 +1331,7 @@ static int enable_device_and_get(struct ib_device *device)
>  	if (!ret)
>  		ret = add_compat_devs(device);
>  out:
> -	up_read(&devices_rwsem);
> +	up_write(&devices_rwsem);
>  	return ret;
>  }

I don't think messing with the devices_rwsem here is a great idea, it
would be better to address this on the clients_rwsem side like:

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 67bcea7a153c6a..b956c9f8e62d34 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1730,7 +1730,7 @@ static int assign_client_id(struct ib_client *client)
 {
 	int ret;
 
-	down_write(&clients_rwsem);
+	lockdep_assert_held(&clients_rwsem);
 	/*
 	 * The add/remove callbacks must be called in FIFO/LIFO order. To
 	 * achieve this we assign client_ids so they are sorted in
@@ -1739,14 +1739,11 @@ static int assign_client_id(struct ib_client *client)
 	client->client_id = highest_client_id;
 	ret = xa_insert(&clients, client->client_id, client, GFP_KERNEL);
 	if (ret)
-		goto out;
+		return ret;
 
 	highest_client_id++;
 	xa_set_mark(&clients, client->client_id, CLIENT_REGISTERED);
-
-out:
-	up_write(&clients_rwsem);
-	return ret;
+	return 0;
 }
 
 static void remove_client_id(struct ib_client *client)
@@ -1776,25 +1773,31 @@ int ib_register_client(struct ib_client *client)
 {
 	struct ib_device *device;
 	unsigned long index;
+	bool need_unreg = false;
 	int ret;
 
 	refcount_set(&client->uses, 1);
 	init_completion(&client->uses_zero);
-	ret = assign_client_id(client);
-	if (ret)
-		return ret;
 
 	down_read(&devices_rwsem);
+	down_write(&clients_rwsem);
+	ret = assign_client_id(client);
+	if (ret)
+		goto out;
+
+	need_unreg = true;
 	xa_for_each_marked (&devices, index, device, DEVICE_REGISTERED) {
 		ret = add_client_context(device, client);
-		if (ret) {
-			up_read(&devices_rwsem);
-			ib_unregister_client(client);
-			return ret;
-		}
+		if (ret)
+			goto out;
 	}
+	ret = 0;
+out:
+	up_write(&clients_rwsem);
 	up_read(&devices_rwsem);
-	return 0;
+	if (need_unreg && ret)
+		ib_unregister_client(client);
+	return ret;
 }
 EXPORT_SYMBOL(ib_register_client);