[openib-general] Fwd: issues in ipoib

Michael S. Tsirkin mst at mellanox.co.il
Thu Aug 3 15:13:13 PDT 2006


Quoting r. Roland Dreier <rdreier at cisco.com>:
> Subject: Re: Fwd: issues in ipoib
> 
>     Michael> No, I think there's no problem - user callback has
>     Michael> finished running and that's all we care about.
> 
> I don't think so -- because then the unregister call can return and
> the client module free the client struct before the wake_up() runs,
> which leads to use-after-free.

OK, here's a version using completion which should address this issue.
How does this look?

--

Require registration with SA module, to prevent module text from going away
while sa query callback is still running, and update all users.

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index d6f99d5..bf668b3 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -60,6 +60,10 @@ static struct ib_client cma_client = {
 	.remove = cma_remove_one
 };
 
+static struct ib_sa_client cma_sa_client = {
+	.name   = "cma"
+};
+
 static LIST_HEAD(dev_list);
 static LIST_HEAD(listen_any_list);
 static DEFINE_MUTEX(lock);
@@ -1140,7 +1144,7 @@ static int cma_query_ib_route(struct rdm
 	path_rec.pkey = cpu_to_be16(ib_addr_get_pkey(addr));
 	path_rec.numb_path = 1;
 
-	id_priv->query_id = ib_sa_path_rec_get(id_priv->id.device,
+	id_priv->query_id = ib_sa_path_rec_get(&cma_sa_client, id_priv->id.device,
 				id_priv->id.port_num, &path_rec,
 				IB_SA_PATH_REC_DGID | IB_SA_PATH_REC_SGID |
 				IB_SA_PATH_REC_PKEY | IB_SA_PATH_REC_NUMB_PATH,
@@ -1910,6 +1914,8 @@ static int cma_init(void)
 	ret = ib_register_client(&cma_client);
 	if (ret)
 		goto err;
+
+	ib_sa_register_client(&cma_sa_client);
 	return 0;
 
 err:
@@ -1919,6 +1925,7 @@ err:
 
 static void cma_cleanup(void)
 {
+	ib_sa_unregister_client(&cma_sa_client);
 	ib_unregister_client(&cma_client);
 	destroy_workqueue(cma_wq);
 	idr_destroy(&sdp_ps);
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index aeda484..ea03677 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -78,6 +78,7 @@ struct ib_sa_query {
 	struct ib_sa_port      *port;
 	struct ib_mad_send_buf *mad_buf;
 	struct ib_sa_sm_ah     *sm_ah;
+	struct ib_sa_client    *client;
 	int			id;
 };
 
@@ -532,6 +533,17 @@ retry:
 	return ret ? ret : id;
 }
 
+static inline void ib_sa_client_get(struct ib_sa_client *client)
+{
+	atomic_inc(&client->users);
+}
+
+static inline void ib_sa_client_put(struct ib_sa_client *client)
+{
+	if (atomic_dec_and_test(&client->users))
+		complete(&client->completion);
+}
+
 static void ib_sa_path_rec_callback(struct ib_sa_query *sa_query,
 				    int status,
 				    struct ib_sa_mad *mad)
@@ -539,6 +551,7 @@ static void ib_sa_path_rec_callback(stru
 	struct ib_sa_path_query *query =
 		container_of(sa_query, struct ib_sa_path_query, sa_query);
 
+	ib_sa_client_get(sa_query->client);
 	if (mad) {
 		struct ib_sa_path_rec rec;
 
@@ -547,6 +560,7 @@ static void ib_sa_path_rec_callback(stru
 		query->callback(status, &rec, query->context);
 	} else
 		query->callback(status, NULL, query->context);
+	ib_sa_client_put(sa_query->client);
 }
 
 static void ib_sa_path_rec_release(struct ib_sa_query *sa_query)
@@ -556,6 +570,7 @@ static void ib_sa_path_rec_release(struc
 
 /**
  * ib_sa_path_rec_get - Start a Path get query
+ * @client:client object used to track the query
  * @device:device to send query on
  * @port_num: port number to send query on
  * @rec:Path Record to send in query
@@ -578,7 +593,8 @@ static void ib_sa_path_rec_release(struc
  * error code.  Otherwise it is a query ID that can be used to cancel
  * the query.
  */
-int ib_sa_path_rec_get(struct ib_device *device, u8 port_num,
+int ib_sa_path_rec_get(struct ib_sa_client *client,
+		       struct ib_device *device, u8 port_num,
 		       struct ib_sa_path_rec *rec,
 		       ib_sa_comp_mask comp_mask,
 		       int timeout_ms, gfp_t gfp_mask,
@@ -619,6 +635,7 @@ int ib_sa_path_rec_get(struct ib_device 
 	mad = query->sa_query.mad_buf->mad;
 	init_mad(mad, agent);
 
+	query->sa_query.client   = client;
 	query->sa_query.callback = callback ? ib_sa_path_rec_callback : NULL;
 	query->sa_query.release  = ib_sa_path_rec_release;
 	query->sa_query.port     = port;
@@ -653,6 +670,7 @@ static void ib_sa_service_rec_callback(s
 	struct ib_sa_service_query *query =
 		container_of(sa_query, struct ib_sa_service_query, sa_query);
 
+	ib_sa_client_get(sa_query->client);
 	if (mad) {
 		struct ib_sa_service_rec rec;
 
@@ -661,6 +679,7 @@ static void ib_sa_service_rec_callback(s
 		query->callback(status, &rec, query->context);
 	} else
 		query->callback(status, NULL, query->context);
+	ib_sa_client_put(sa_query->client);
 }
 
 static void ib_sa_service_rec_release(struct ib_sa_query *sa_query)
@@ -670,6 +689,7 @@ static void ib_sa_service_rec_release(st
 
 /**
  * ib_sa_service_rec_query - Start Service Record operation
+ * @client:client object used to track the query
  * @device:device to send request on
  * @port_num: port number to send request on
  * @method:SA method - should be get, set, or delete
@@ -694,7 +714,8 @@ static void ib_sa_service_rec_release(st
  * error code.  Otherwise it is a request ID that can be used to cancel
  * the query.
  */
-int ib_sa_service_rec_query(struct ib_device *device, u8 port_num, u8 method,
+int ib_sa_service_rec_query(struct ib_sa_client *client,
+			    struct ib_device *device, u8 port_num, u8 method,
 			    struct ib_sa_service_rec *rec,
 			    ib_sa_comp_mask comp_mask,
 			    int timeout_ms, gfp_t gfp_mask,
@@ -740,6 +761,7 @@ int ib_sa_service_rec_query(struct ib_de
 	mad = query->sa_query.mad_buf->mad;
 	init_mad(mad, agent);
 
+	query->sa_query.client   = client;
 	query->sa_query.callback = callback ? ib_sa_service_rec_callback : NULL;
 	query->sa_query.release  = ib_sa_service_rec_release;
 	query->sa_query.port     = port;
@@ -775,6 +797,7 @@ static void ib_sa_mcmember_rec_callback(
 	struct ib_sa_mcmember_query *query =
 		container_of(sa_query, struct ib_sa_mcmember_query, sa_query);
 
+	ib_sa_client_get(sa_query->client);
 	if (mad) {
 		struct ib_sa_mcmember_rec rec;
 
@@ -783,6 +806,7 @@ static void ib_sa_mcmember_rec_callback(
 		query->callback(status, &rec, query->context);
 	} else
 		query->callback(status, NULL, query->context);
+	ib_sa_client_put(sa_query->client);
 }
 
 static void ib_sa_mcmember_rec_release(struct ib_sa_query *sa_query)
@@ -790,7 +814,8 @@ static void ib_sa_mcmember_rec_release(s
 	kfree(container_of(sa_query, struct ib_sa_mcmember_query, sa_query));
 }
 
-int ib_sa_mcmember_rec_query(struct ib_device *device, u8 port_num,
+int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
+			     struct ib_device *device, u8 port_num,
 			     u8 method,
 			     struct ib_sa_mcmember_rec *rec,
 			     ib_sa_comp_mask comp_mask,
@@ -832,6 +857,7 @@ int ib_sa_mcmember_rec_query(struct ib_d
 	mad = query->sa_query.mad_buf->mad;
 	init_mad(mad, agent);
 
+	query->sa_query.client   = client;
 	query->sa_query.callback = callback ? ib_sa_mcmember_rec_callback : NULL;
 	query->sa_query.release  = ib_sa_mcmember_rec_release;
 	query->sa_query.port     = port;
@@ -866,6 +892,7 @@ static void send_handler(struct ib_mad_a
 	struct ib_sa_query *query = mad_send_wc->send_buf->context[0];
 	unsigned long flags;
 
+	ib_sa_client_get(query->client);
 	if (query->callback)
 		switch (mad_send_wc->status) {
 		case IB_WC_SUCCESS:
@@ -881,6 +908,7 @@ static void send_handler(struct ib_mad_a
 			query->callback(query, -EIO, NULL);
 			break;
 		}
+	ib_sa_client_put(query->client);
 
 	spin_lock_irqsave(&idr_lock, flags);
 	idr_remove(&query_idr, query->id);
@@ -900,6 +928,7 @@ static void recv_handler(struct ib_mad_a
 	mad_buf = (void *) (unsigned long) mad_recv_wc->wc->wr_id;
 	query = mad_buf->context[0];
 
+	ib_sa_client_get(query->client);
 	if (query->callback) {
 		if (mad_recv_wc->wc->status == IB_WC_SUCCESS)
 			query->callback(query,
@@ -909,6 +938,7 @@ static void recv_handler(struct ib_mad_a
 		else
 			query->callback(query, -EIO, NULL);
 	}
+	ib_sa_client_put(query->client);
 
 	ib_free_recv_mad(mad_recv_wc);
 }
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 474aa21..28a9f0f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -390,4 +390,5 @@ #define IPOIB_GID_RAW_ARG(gid)	((u8 *)(g
 
 #define IPOIB_GID_ARG(gid)	IPOIB_GID_RAW_ARG((gid).raw)
 
+extern struct ib_sa_client ipoib_sa_client;
 #endif /* _IPOIB_H */
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index cf71d2a..ca10724 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -91,6 +91,10 @@ static struct ib_client ipoib_client = {
 	.remove = ipoib_remove_one
 };
 
+struct ib_sa_client ipoib_sa_client = {
+	.name   = "ipoib"
+};
+
 int ipoib_open(struct net_device *dev)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
@@ -459,7 +463,7 @@ static int path_rec_start(struct net_dev
 	init_completion(&path->done);
 
 	path->query_id =
-		ib_sa_path_rec_get(priv->ca, priv->port,
+		ib_sa_path_rec_get(&ipoib_sa_client, priv->ca, priv->port,
 				   &path->pathrec,
 				   IB_SA_PATH_REC_DGID		|
 				   IB_SA_PATH_REC_SGID		|
@@ -1185,6 +1189,8 @@ static int __init ipoib_init_module(void
 	if (ret)
 		goto err_wq;
 
+	ib_sa_register_client(&ipoib_sa_client);
+
 	return 0;
 
 err_wq:
@@ -1198,6 +1204,7 @@ err_fs:
 
 static void __exit ipoib_cleanup_module(void)
 {
+	ib_sa_unregister_client(&ipoib_sa_client);
 	ib_unregister_client(&ipoib_client);
 	ipoib_unregister_debugfs();
 	destroy_workqueue(ipoib_workqueue);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index b5e6a7b..f688323 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -360,7 +360,7 @@ #endif
 
 	init_completion(&mcast->done);
 
-	ret = ib_sa_mcmember_rec_set(priv->ca, priv->port, &rec,
+	ret = ib_sa_mcmember_rec_set(&ipoib_sa_client, priv->ca, priv->port, &rec,
 				     IB_SA_MCMEMBER_REC_MGID		|
 				     IB_SA_MCMEMBER_REC_PORT_GID	|
 				     IB_SA_MCMEMBER_REC_PKEY		|
@@ -484,8 +484,8 @@ static void ipoib_mcast_join(struct net_
 
 	init_completion(&mcast->done);
 
-	ret = ib_sa_mcmember_rec_set(priv->ca, priv->port, &rec, comp_mask,
-				     mcast->backoff * 1000, GFP_ATOMIC,
+	ret = ib_sa_mcmember_rec_set(&ipoib_sa_client, priv->ca, priv->port, &rec,
+				     comp_mask, mcast->backoff * 1000, GFP_ATOMIC,
 				     ipoib_mcast_join_complete,
 				     mcast, &mcast->query);
 
@@ -680,7 +680,7 @@ static int ipoib_mcast_leave(struct net_
 	 * Just make one shot at leaving and don't wait for a reply;
 	 * if we fail, too bad.
 	 */
-	ret = ib_sa_mcmember_rec_delete(priv->ca, priv->port, &rec,
+	ret = ib_sa_mcmember_rec_delete(&ipoib_sa_client, priv->ca, priv->port, &rec,
 					IB_SA_MCMEMBER_REC_MGID		|
 					IB_SA_MCMEMBER_REC_PORT_GID	|
 					IB_SA_MCMEMBER_REC_PKEY		|
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8f472e7..0856d78 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -88,6 +88,10 @@ static struct ib_client srp_client = {
 	.remove = srp_remove_one
 };
 
+static struct ib_sa_client srp_sa_client = {
+	.name   = "srp"
+};
+
 static inline struct srp_target_port *host_to_target(struct Scsi_Host *host)
 {
 	return (struct srp_target_port *) host->hostdata;
@@ -259,7 +263,8 @@ static int srp_lookup_path(struct srp_ta
 
 	init_completion(&target->done);
 
-	target->path_query_id = ib_sa_path_rec_get(target->srp_host->dev->dev,
+	target->path_query_id = ib_sa_path_rec_get(&srp_sa_client,
+						   target->srp_host->dev->dev,
 						   target->srp_host->port,
 						   &target->path,
 						   IB_SA_PATH_REC_DGID		|
diff --git a/include/rdma/ib_sa.h b/include/rdma/ib_sa.h
index c99e442..084baf4 100644
--- a/include/rdma/ib_sa.h
+++ b/include/rdma/ib_sa.h
@@ -37,6 +37,8 @@ #ifndef IB_SA_H
 #define IB_SA_H
 
 #include <linux/compiler.h>
+#include <linux/completion.h>
+#include <asm/atomic.h>
 
 #include <rdma/ib_verbs.h>
 #include <rdma/ib_mad.h>
@@ -250,11 +252,18 @@ struct ib_sa_service_rec {
 	u64		data64[2];
 };
 
+struct ib_sa_client {
+	char *name;
+	atomic_t users;
+	struct completion completion;
+};
+
 struct ib_sa_query;
 
 void ib_sa_cancel_query(int id, struct ib_sa_query *query);
 
-int ib_sa_path_rec_get(struct ib_device *device, u8 port_num,
+int ib_sa_path_rec_get(struct ib_sa_client *client,
+		       struct ib_device *device, u8 port_num,
 		       struct ib_sa_path_rec *rec,
 		       ib_sa_comp_mask comp_mask,
 		       int timeout_ms, gfp_t gfp_mask,
@@ -264,7 +273,8 @@ int ib_sa_path_rec_get(struct ib_device 
 		       void *context,
 		       struct ib_sa_query **query);
 
-int ib_sa_mcmember_rec_query(struct ib_device *device, u8 port_num,
+int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
+			     struct ib_device *device, u8 port_num,
 			     u8 method,
 			     struct ib_sa_mcmember_rec *rec,
 			     ib_sa_comp_mask comp_mask,
@@ -275,7 +285,8 @@ int ib_sa_mcmember_rec_query(struct ib_d
 			     void *context,
 			     struct ib_sa_query **query);
 
-int ib_sa_service_rec_query(struct ib_device *device, u8 port_num,
+int ib_sa_service_rec_query(struct ib_sa_client *client,
+			 struct ib_device *device, u8 port_num,
 			 u8 method,
 			 struct ib_sa_service_rec *rec,
 			 ib_sa_comp_mask comp_mask,
@@ -288,6 +299,7 @@ int ib_sa_service_rec_query(struct ib_de
 
 /**
  * ib_sa_mcmember_rec_set - Start an MCMember set query
+ * @client:client object used to track the query
  * @device:device to send query on
  * @port_num: port number to send query on
  * @rec:MCMember Record to send in query
@@ -311,7 +323,8 @@ int ib_sa_service_rec_query(struct ib_de
  * cancel the query.
  */
 static inline int
-ib_sa_mcmember_rec_set(struct ib_device *device, u8 port_num,
+ib_sa_mcmember_rec_set(struct ib_sa_client *client,
+		       struct ib_device *device, u8 port_num,
 		       struct ib_sa_mcmember_rec *rec,
 		       ib_sa_comp_mask comp_mask,
 		       int timeout_ms, gfp_t gfp_mask,
@@ -321,7 +334,7 @@ ib_sa_mcmember_rec_set(struct ib_device 
 		       void *context,
 		       struct ib_sa_query **query)
 {
-	return ib_sa_mcmember_rec_query(device, port_num,
+	return ib_sa_mcmember_rec_query(client, device, port_num,
 					IB_MGMT_METHOD_SET,
 					rec, comp_mask,
 					timeout_ms, gfp_mask, callback,
@@ -330,6 +343,7 @@ ib_sa_mcmember_rec_set(struct ib_device 
 
 /**
  * ib_sa_mcmember_rec_delete - Start an MCMember delete query
+ * @client:client object used to track the query
  * @device:device to send query on
  * @port_num: port number to send query on
  * @rec:MCMember Record to send in query
@@ -353,7 +367,8 @@ ib_sa_mcmember_rec_set(struct ib_device 
  * cancel the query.
  */
 static inline int
-ib_sa_mcmember_rec_delete(struct ib_device *device, u8 port_num,
+ib_sa_mcmember_rec_delete(struct ib_sa_client *client,
+			  struct ib_device *device, u8 port_num,
 			  struct ib_sa_mcmember_rec *rec,
 			  ib_sa_comp_mask comp_mask,
 			  int timeout_ms, gfp_t gfp_mask,
@@ -363,7 +378,7 @@ ib_sa_mcmember_rec_delete(struct ib_devi
 			  void *context,
 			  struct ib_sa_query **query)
 {
-	return ib_sa_mcmember_rec_query(device, port_num,
+	return ib_sa_mcmember_rec_query(client, device, port_num,
 					IB_SA_METHOD_DELETE,
 					rec, comp_mask,
 					timeout_ms, gfp_mask, callback,
@@ -378,4 +393,25 @@ int ib_init_ah_from_path(struct ib_devic
 			 struct ib_sa_path_rec *rec,
 			 struct ib_ah_attr *ah_attr);
 
+/**
+ * ib_sa_register_client - register SA client object
+ * @client:client object used to track queries
+ */
+static inline void ib_sa_register_client(struct ib_sa_client *client)
+{
+	atomic_set(&client->users, 1);
+	init_completion(&client->completion);
+}
+
+/**
+ * ib_sa_unregister_client - unregister SA client object
+ * @client:client object used to track queries
+ */
+static inline void ib_sa_unregister_client(struct ib_sa_client *client)
+{
+	if (atomic_dec_and_test(&client->users))
+		complete(&client->completion);
+	wait_for_completion(&client->completion);
+}
+
 #endif /* IB_SA_H */

-- 
MST




More information about the general mailing list