From 25ae21a10112875763c18b385624df713a288a05 Mon Sep 17 00:00:00 2001 From: Sean Hefty Date: Wed, 23 Feb 2011 08:11:32 -0800 Subject: RDMA/cma: Fix crash in request handlers Doug Ledford and Red Hat reported a crash when running the rdma_cm on a real-time OS. The crash has the following call trace: cm_process_work cma_req_handler cma_disable_callback rdma_create_id kzalloc init_completion cma_get_net_info cma_save_net_info cma_any_addr cma_zero_addr rdma_translate_ip rdma_copy_addr cma_acquire_dev rdma_addr_get_sgid ib_find_cached_gid cma_attach_to_dev ucma_event_handler kzalloc ib_copy_ah_attr_to_user cma_comp [ preempted ] cma_write copy_from_user ucma_destroy_id copy_from_user _ucma_find_context ucma_put_ctx ucma_free_ctx rdma_destroy_id cma_exch cma_cancel_operation rdma_node_get_transport rt_mutex_slowunlock bad_area_nosemaphore oops_enter They were able to reproduce the crash multiple times with the following details: Crash seems to always happen on the: mutex_unlock(&conn_id->handler_mutex); as conn_id looks to have been freed during this code path. An examination of the code shows that a race exists in the request handlers. When a new connection request is received, the rdma_cm allocates a new connection identifier. This identifier has a single reference count on it. If a user calls rdma_destroy_id() from another thread after receiving a callback, rdma_destroy_id will proceed to destroy the id and free the associated memory. However, the request handlers may still be in the process of running. When control returns to the request handlers, they can attempt to access the newly created identifiers. Fix this by holding a reference on the newly created rdma_cm_id until the request handler is through accessing it. Signed-off-by: Sean Hefty Acked-by: Doug Ledford Cc: Signed-off-by: Roland Dreier --- drivers/infiniband/core/cma.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'drivers/infiniband/core/cma.c') diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 6884da24fde1..e450c5a87727 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -1210,6 +1210,11 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event) cm_id->context = conn_id; cm_id->cm_handler = cma_ib_handler; + /* + * Protect against the user destroying conn_id from another thread + * until we're done accessing it. + */ + atomic_inc(&conn_id->refcount); ret = conn_id->id.event_handler(&conn_id->id, &event); if (!ret) { /* @@ -1222,8 +1227,10 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event) ib_send_cm_mra(cm_id, CMA_CM_MRA_SETTING, NULL, 0); mutex_unlock(&lock); mutex_unlock(&conn_id->handler_mutex); + cma_deref_id(conn_id); goto out; } + cma_deref_id(conn_id); /* Destroy the CM ID by returning a non-zero value. */ conn_id->cm_id.ib = NULL; @@ -1425,17 +1432,25 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id, event.param.conn.private_data_len = iw_event->private_data_len; event.param.conn.initiator_depth = attr.max_qp_init_rd_atom; event.param.conn.responder_resources = attr.max_qp_rd_atom; + + /* + * Protect against the user destroying conn_id from another thread + * until we're done accessing it. + */ + atomic_inc(&conn_id->refcount); ret = conn_id->id.event_handler(&conn_id->id, &event); if (ret) { /* User wants to destroy the CM ID */ conn_id->cm_id.iw = NULL; cma_exch(conn_id, CMA_DESTROYING); mutex_unlock(&conn_id->handler_mutex); + cma_deref_id(conn_id); rdma_destroy_id(&conn_id->id); goto out; } mutex_unlock(&conn_id->handler_mutex); + cma_deref_id(conn_id); out: if (dev) -- cgit v1.2.3 From a396d43a35fb91f2d4920a4700d25ecc5ec92404 Mon Sep 17 00:00:00 2001 From: Sean Hefty Date: Wed, 23 Feb 2011 09:05:39 -0800 Subject: RDMA/cma: Replace global lock in rdma_destroy_id() with id-specific one rdma_destroy_id currently uses the global rdma cm 'lock' to test if an rdma_cm_id has been bound to a device. This prevents an active address resolution callback handler from assigning a device to the rdma_cm_id after rdma_destroy_id checks for one. Instead, we can replace the use of the global lock around the check to the rdma_cm_id device pointer by setting the id state to destroying, then flushing all active callbacks. The latter is accomplished by acquiring and releasing the handler_mutex. Any active handler will complete first, and any newly scheduled handlers will find the rdma_cm_id in an invalid state. In addition to optimizing the current locking scheme, the use of the rdma_cm_id mutex is a more intuitive synchronization mechanism than that of the global lock. These changes are based on feedback from Doug Ledford while he was trying to debug a crash in the rdma cm destroy path. Signed-off-by: Sean Hefty Signed-off-by: Roland Dreier --- drivers/infiniband/core/cma.c | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) (limited to 'drivers/infiniband/core/cma.c') diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index e450c5a87727..5ed9d25d021a 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -308,11 +308,13 @@ static inline void release_mc(struct kref *kref) kfree(mc); } -static void cma_detach_from_dev(struct rdma_id_private *id_priv) +static void cma_release_dev(struct rdma_id_private *id_priv) { + mutex_lock(&lock); list_del(&id_priv->list); cma_deref_dev(id_priv->cma_dev); id_priv->cma_dev = NULL; + mutex_unlock(&lock); } static int cma_set_qkey(struct rdma_id_private *id_priv) @@ -373,6 +375,7 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv) enum rdma_link_layer dev_ll = dev_addr->dev_type == ARPHRD_INFINIBAND ? IB_LINK_LAYER_INFINIBAND : IB_LINK_LAYER_ETHERNET; + mutex_lock(&lock); iboe_addr_get_sgid(dev_addr, &iboe_gid); memcpy(&gid, dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr), sizeof gid); @@ -398,6 +401,7 @@ out: if (!ret) cma_attach_to_dev(id_priv, cma_dev); + mutex_unlock(&lock); return ret; } @@ -904,9 +908,14 @@ void rdma_destroy_id(struct rdma_cm_id *id) state = cma_exch(id_priv, CMA_DESTROYING); cma_cancel_operation(id_priv, state); - mutex_lock(&lock); + /* + * Wait for any active callback to finish. New callbacks will find + * the id_priv state set to destroying and abort. + */ + mutex_lock(&id_priv->handler_mutex); + mutex_unlock(&id_priv->handler_mutex); + if (id_priv->cma_dev) { - mutex_unlock(&lock); switch (rdma_node_get_transport(id_priv->id.device->node_type)) { case RDMA_TRANSPORT_IB: if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib)) @@ -920,10 +929,8 @@ void rdma_destroy_id(struct rdma_cm_id *id) break; } cma_leave_mc_groups(id_priv); - mutex_lock(&lock); - cma_detach_from_dev(id_priv); + cma_release_dev(id_priv); } - mutex_unlock(&lock); cma_release_port(id_priv); cma_deref_id(id_priv); @@ -1200,9 +1207,7 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event) } mutex_lock_nested(&conn_id->handler_mutex, SINGLE_DEPTH_NESTING); - mutex_lock(&lock); ret = cma_acquire_dev(conn_id); - mutex_unlock(&lock); if (ret) goto release_conn_id; @@ -1401,9 +1406,7 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id, goto out; } - mutex_lock(&lock); ret = cma_acquire_dev(conn_id); - mutex_unlock(&lock); if (ret) { mutex_unlock(&conn_id->handler_mutex); rdma_destroy_id(new_cm_id); @@ -1966,20 +1969,11 @@ static void addr_handler(int status, struct sockaddr *src_addr, memset(&event, 0, sizeof event); mutex_lock(&id_priv->handler_mutex); - - /* - * Grab mutex to block rdma_destroy_id() from removing the device while - * we're trying to acquire it. - */ - mutex_lock(&lock); - if (!cma_comp_exch(id_priv, CMA_ADDR_QUERY, CMA_ADDR_RESOLVED)) { - mutex_unlock(&lock); + if (!cma_comp_exch(id_priv, CMA_ADDR_QUERY, CMA_ADDR_RESOLVED)) goto out; - } if (!status && !id_priv->cma_dev) status = cma_acquire_dev(id_priv); - mutex_unlock(&lock); if (status) { if (!cma_comp_exch(id_priv, CMA_ADDR_RESOLVED, CMA_ADDR_BOUND)) @@ -2280,9 +2274,7 @@ int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr) if (ret) goto err1; - mutex_lock(&lock); ret = cma_acquire_dev(id_priv); - mutex_unlock(&lock); if (ret) goto err1; } @@ -2294,11 +2286,8 @@ int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr) return 0; err2: - if (id_priv->cma_dev) { - mutex_lock(&lock); - cma_detach_from_dev(id_priv); - mutex_unlock(&lock); - } + if (id_priv->cma_dev) + cma_release_dev(id_priv); err1: cma_comp_exch(id_priv, CMA_ADDR_BOUND, CMA_IDLE); return ret; -- cgit v1.2.3