[ofa-general] [PATCH 1/2] IB/core: handle race between elements in qork queues after event

[Moni Shoua]

Thanks for the comment and example. Please take a look below. The last paragraph  in the patch documentation
refers to the race you pointed. The test is made after sm_ah is copied to the mad so there is no risk of  it being
NULL in the mad structure. 

---------------------------------------

This patch solves a race between work elements that are carried out after an
event occurs. When SM address handle becomes invalid and needs an update it is
handled by a work in the global workqueue. On the other hand this event is also
handled in ib_ipoib by queuing a work in the ipoib_workqueue that does mcast join.
Although queuing is in the right order, it is done to 2 different workqueues and so
there is no guarantee that the first to be queued is the first to be executed.

The patch sets the SM address handle  to NULL and until update_sm_ah() is called,
any request that needs sm_ah is replied with -EAGAIN return status.

For consumers, the patch doesn't make things worse. Before the patch, MADS are sent to the
wrong SM so the request gets lost. Consumers can be improved if they examine the return
code and respond to EAGAIN properly but even without an improvement the situation
is not getting worse and in some cases it gets better.

If ib_sa_event() is called in during consumers work (e.g. ib_sa_path_rec_get()) and after
the check for NULL SM address handle the result would be as before the patch and without a
rist of  dereferencing a NULL  pinter.

Signed-off-by: Moni Levy  <monil at voltaire.com>
Signed-off-by: Moni Shoua <monis at voltaire.com>

---

 drivers/infiniband/core/sa_query.c |   49 +++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index cf474ec..8170381 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -413,9 +413,20 @@ static void ib_sa_event(struct ib_event_handler *handler, struct ib_event *event
 	    event->event == IB_EVENT_PKEY_CHANGE ||
 	    event->event == IB_EVENT_SM_CHANGE   ||
 	    event->event == IB_EVENT_CLIENT_REREGISTER) {
-		struct ib_sa_device *sa_dev;
-		sa_dev = container_of(handler, typeof(*sa_dev), event_handler);
-
+		unsigned long flags;
+		struct ib_sa_device *sa_dev =
+			container_of(handler, typeof(*sa_dev), event_handler);
+		struct ib_sa_port *port =
+			&sa_dev->port[event->element.port_num - sa_dev->start_port];
+		struct ib_sa_sm_ah *sm_ah;
+
+		spin_lock_irqsave(&port->ah_lock, flags);
+		sm_ah = port->sm_ah;
+		port->sm_ah = NULL;
+		spin_unlock_irqrestore(&port->ah_lock, flags);
+
+		if (sm_ah)
+			kref_put(&sm_ah->ref, free_sm_ah);
 		schedule_work(&sa_dev->port[event->element.port_num -
 					    sa_dev->start_port].update_task);
 	}
@@ -673,6 +684,10 @@ int ib_sa_path_rec_get(struct ib_sa_client *client,
 	ret = alloc_mad(&query->sa_query, gfp_mask);
 	if (ret)
 		goto err1;
+	if (!port->sm_ah) {
+		ret = -EAGAIN;
+		goto err2;
+	}
 
 	ib_sa_client_get(client);
 	query->sa_query.client = client;
@@ -694,13 +709,14 @@ int ib_sa_path_rec_get(struct ib_sa_client *client,
 
 	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
-		goto err2;
+		goto err3;
 
 	return ret;
 
-err2:
+err3:
 	*sa_query = NULL;
 	ib_sa_client_put(query->sa_query.client);
+err2:
 	free_mad(&query->sa_query);
 
 err1:
@@ -780,6 +796,7 @@ int ib_sa_service_rec_query(struct ib_sa_client *client,
 		return -ENODEV;
 
 	port  = &sa_dev->port[port_num - sa_dev->start_port];
+
 	agent = port->agent;
 
 	if (method != IB_MGMT_METHOD_GET &&
@@ -795,6 +812,10 @@ int ib_sa_service_rec_query(struct ib_sa_client *client,
 	ret = alloc_mad(&query->sa_query, gfp_mask);
 	if (ret)
 		goto err1;
+	if (!port->sm_ah) {
+		ret = -EAGAIN;
+		goto err2;
+	}
 
 	ib_sa_client_get(client);
 	query->sa_query.client = client;
@@ -817,15 +838,15 @@ int ib_sa_service_rec_query(struct ib_sa_client *client,
 
 	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
-		goto err2;
+		goto err3;
 
 	return ret;
 
-err2:
+err3:
 	*sa_query = NULL;
 	ib_sa_client_put(query->sa_query.client);
+err2:
 	free_mad(&query->sa_query);
-
 err1:
 	kfree(query);
 	return ret;
@@ -877,8 +898,8 @@ int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
 		return -ENODEV;
 
 	port  = &sa_dev->port[port_num - sa_dev->start_port];
-	agent = port->agent;
 
+	agent = port->agent;
 	query = kmalloc(sizeof *query, gfp_mask);
 	if (!query)
 		return -ENOMEM;
@@ -887,6 +908,10 @@ int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
 	ret = alloc_mad(&query->sa_query, gfp_mask);
 	if (ret)
 		goto err1;
+	if (!port->sm_ah) {
+		ret = -EAGAIN;
+		goto err2;
+	}
 
 	ib_sa_client_get(client);
 	query->sa_query.client = client;
@@ -909,15 +934,15 @@ int ib_sa_mcmember_rec_query(struct ib_sa_client *client,
 
 	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
-		goto err2;
+		goto err3;
 
 	return ret;
 
-err2:
+err3:
 	*sa_query = NULL;
 	ib_sa_client_put(query->sa_query.client);
+err2:
 	free_mad(&query->sa_query);
-
 err1:
 	kfree(query);
 	return ret;