IB/core: lock client data with lists_rwsem

An ib_client callback that is called with the lists_rwsem locked only for
read is protected from changes to the IB client lists, but not from
ib_unregister_device() freeing its client data. This is because
ib_unregister_device() will remove the device from the device list with
lists_rwsem locked for write, but perform the rest of the cleanup,
including the call to remove() without that lock.

Mark client data that is undergoing de-registration with a new going_down
flag in the client data context. Lock the client data list with lists_rwsem
for write in addition to using the spinlock, so that functions calling the
callback would be able to lock only lists_rwsem for read and let callbacks
sleep.

Since ib_unregister_client() now marks the client data context, no need for
remove() to search the context again, so pass the client data directly to
remove() callbacks.

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 871da832d016a7a9b6305047f6e512bc2a936b68..c93af66cc091a8e9faa354e32538268e6217d0c7 100644 (file)
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -394,7 +394,7 @@ err:
        kfree(device->cache.lmc_cache);
 }
 
-static void ib_cache_cleanup_one(struct ib_device *device)
+static void ib_cache_cleanup_one(struct ib_device *device, void *client_data)
 {
        int p;
 

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 3a972ebf3c0d1170efe280aa7bcf781c831fa98f..82d5c4362aa856645e148f0a80fff401b6bd0653 100644 (file)
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -58,7 +58,7 @@ MODULE_DESCRIPTION("InfiniBand CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
 static void cm_add_one(struct ib_device *device);
-static void cm_remove_one(struct ib_device *device);
+static void cm_remove_one(struct ib_device *device, void *client_data);
 
 static struct ib_client cm_client = {
        .name   = "cm",
@@ -3886,9 +3886,9 @@ free:
        kfree(cm_dev);
 }
 
-static void cm_remove_one(struct ib_device *ib_device)
+static void cm_remove_one(struct ib_device *ib_device, void *client_data)
 {
-       struct cm_device *cm_dev;
+       struct cm_device *cm_dev = client_data;
        struct cm_port *port;
        struct ib_port_modify port_modify = {
                .clr_port_cap_mask = IB_PORT_CM_SUP
@@ -3896,7 +3896,6 @@ static void cm_remove_one(struct ib_device *ib_device)
        unsigned long flags;
        int i;
 
-       cm_dev = ib_get_client_data(ib_device, &cm_client);
        if (!cm_dev)
                return;
 

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 4e72e4c16cfe7fff467ec5b5b21b985b8acb74e1..9664131c4eeb6c1126e0488dac0dcc56760fa8ad 100644 (file)
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -94,7 +94,7 @@ const char *rdma_event_msg(enum rdma_cm_event_type event)
 EXPORT_SYMBOL(rdma_event_msg);
 
 static void cma_add_one(struct ib_device *device);
-static void cma_remove_one(struct ib_device *device);
+static void cma_remove_one(struct ib_device *device, void *client_data);
 
 static struct ib_client cma_client = {
        .name   = "cma",
@@ -3554,11 +3554,10 @@ static void cma_process_remove(struct cma_device *cma_dev)
        wait_for_completion(&cma_dev->comp);
 }
 
-static void cma_remove_one(struct ib_device *device)
+static void cma_remove_one(struct ib_device *device, void *client_data)
 {
-       struct cma_device *cma_dev;
+       struct cma_device *cma_dev = client_data;
 
-       cma_dev = ib_get_client_data(device, &cma_client);
        if (!cma_dev)
                return;
 

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 0c8fa781538bbcc2d4b1b66ae660bf6a41c90e9b..ce317e623862250cb2d5627a2fc886f119bc086f 100644 (file)
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -50,6 +50,9 @@ struct ib_client_data {
        struct list_head  list;
        struct ib_client *client;
        void *            data;
+       /* The device or client is going down. Do not call client or device
+        * callbacks other than remove(). */
+       bool              going_down;
 };
 
 struct workqueue_struct *ib_wq;
@@ -69,6 +72,8 @@ static LIST_HEAD(client_list);
  * to the lists must be done with a write lock. A special case is when the
  * device_mutex is locked. In this case locking the lists for read access is
  * not necessary as the device_mutex implies it.
+ *
+ * lists_rwsem also protects access to the client data list.
  */
 static DEFINE_MUTEX(device_mutex);
 static DECLARE_RWSEM(lists_rwsem);
@@ -210,10 +215,13 @@ static int add_client_context(struct ib_device *device, struct ib_client *client
 
        context->client = client;
        context->data   = NULL;
+       context->going_down = false;
 
+       down_write(&lists_rwsem);
        spin_lock_irqsave(&device->client_data_lock, flags);
        list_add(&context->list, &device->client_data_list);
        spin_unlock_irqrestore(&device->client_data_lock, flags);
+       up_write(&lists_rwsem);
 
        return 0;
 }
@@ -339,7 +347,6 @@ EXPORT_SYMBOL(ib_register_device);
  */
 void ib_unregister_device(struct ib_device *device)
 {
-       struct ib_client *client;
        struct ib_client_data *context, *tmp;
        unsigned long flags;
 
@@ -347,20 +354,29 @@ void ib_unregister_device(struct ib_device *device)
 
        down_write(&lists_rwsem);
        list_del(&device->core_list);
-       up_write(&lists_rwsem);
+       spin_lock_irqsave(&device->client_data_lock, flags);
+       list_for_each_entry_safe(context, tmp, &device->client_data_list, list)
+               context->going_down = true;
+       spin_unlock_irqrestore(&device->client_data_lock, flags);
+       downgrade_write(&lists_rwsem);
 
-       list_for_each_entry_reverse(client, &client_list, list)
-               if (client->remove)
-                       client->remove(device);
+       list_for_each_entry_safe(context, tmp, &device->client_data_list,
+                                list) {
+               if (context->client->remove)
+                       context->client->remove(device, context->data);
+       }
+       up_read(&lists_rwsem);
 
        mutex_unlock(&device_mutex);
 
        ib_device_unregister_sysfs(device);
 
+       down_write(&lists_rwsem);
        spin_lock_irqsave(&device->client_data_lock, flags);
        list_for_each_entry_safe(context, tmp, &device->client_data_list, list)
                kfree(context);
        spin_unlock_irqrestore(&device->client_data_lock, flags);
+       up_write(&lists_rwsem);
 
        device->reg_state = IB_DEV_UNREGISTERED;
 }
@@ -420,16 +436,35 @@ void ib_unregister_client(struct ib_client *client)
        up_write(&lists_rwsem);
 
        list_for_each_entry(device, &device_list, core_list) {
-               if (client->remove)
-                       client->remove(device);
+               struct ib_client_data *found_context = NULL;
 
+               down_write(&lists_rwsem);
                spin_lock_irqsave(&device->client_data_lock, flags);
                list_for_each_entry_safe(context, tmp, &device->client_data_list, list)
                        if (context->client == client) {
-                               list_del(&context->list);
-                               kfree(context);
+                               context->going_down = true;
+                               found_context = context;
+                               break;
                        }
                spin_unlock_irqrestore(&device->client_data_lock, flags);
+               up_write(&lists_rwsem);
+
+               if (client->remove)
+                       client->remove(device, found_context ?
+                                              found_context->data : NULL);
+
+               if (!found_context) {
+                       pr_warn("No client context found for %s/%s\n",
+                               device->name, client->name);
+                       continue;
+               }
+
+               down_write(&lists_rwsem);
+               spin_lock_irqsave(&device->client_data_lock, flags);
+               list_del(&found_context->list);
+               kfree(found_context);
+               spin_unlock_irqrestore(&device->client_data_lock, flags);
+               up_write(&lists_rwsem);
        }
 
        mutex_unlock(&device_mutex);

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 786fc51bf04b22b0d9b0fc371f3fdb25ec4c811b..66b4b3eb8f67b21078855fc9b8bfb25bc5a17371 100644 (file)
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -3335,7 +3335,7 @@ error:
        }
 }
 
-static void ib_mad_remove_device(struct ib_device *device)
+static void ib_mad_remove_device(struct ib_device *device, void *client_data)
 {
        int i;
 

diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c
index 2cb865c7ce7a98773f338b1b8c09ffc66db4cebf..d38d8b2b2979ddc2bebb243b98b79a04644fc929 100644 (file)
--- a/drivers/infiniband/core/multicast.c
+++ b/drivers/infiniband/core/multicast.c
@@ -43,7 +43,7 @@
 #include "sa.h"
 
 static void mcast_add_one(struct ib_device *device);
-static void mcast_remove_one(struct ib_device *device);
+static void mcast_remove_one(struct ib_device *device, void *client_data);
 
 static struct ib_client mcast_client = {
        .name   = "ib_multicast",
@@ -840,13 +840,12 @@ static void mcast_add_one(struct ib_device *device)
        ib_register_event_handler(&dev->event_handler);
 }
 
-static void mcast_remove_one(struct ib_device *device)
+static void mcast_remove_one(struct ib_device *device, void *client_data)
 {
-       struct mcast_device *dev;
+       struct mcast_device *dev = client_data;
        struct mcast_port *port;
        int i;
 
-       dev = ib_get_client_data(device, &mcast_client);
        if (!dev)
                return;
 

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index ca919f4296664f070f0c63b1765542375818f0aa..d40be3673b79e8399024d0658486ca19d7698be9 100644 (file)
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -107,7 +107,7 @@ struct ib_sa_mcmember_query {
 };
 
 static void ib_sa_add_one(struct ib_device *device);
-static void ib_sa_remove_one(struct ib_device *device);
+static void ib_sa_remove_one(struct ib_device *device, void *client_data);
 
 static struct ib_client sa_client = {
        .name   = "sa",
@@ -1221,9 +1221,9 @@ free:
        return;
 }
 
-static void ib_sa_remove_one(struct ib_device *device)
+static void ib_sa_remove_one(struct ib_device *device, void *client_data)
 {
-       struct ib_sa_device *sa_dev = ib_get_client_data(device, &sa_client);
+       struct ib_sa_device *sa_dev = client_data;
        int i;
 
        if (!sa_dev)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 00948107364466cafe28e95557be1fcf829e988c..8cde48b96f1945d8e12d62995712c31884234146 100644 (file)
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -109,7 +109,7 @@ enum {
 #define IB_UCM_BASE_DEV MKDEV(IB_UCM_MAJOR, IB_UCM_BASE_MINOR)
 
 static void ib_ucm_add_one(struct ib_device *device);
-static void ib_ucm_remove_one(struct ib_device *device);
+static void ib_ucm_remove_one(struct ib_device *device, void *client_data);
 
 static struct ib_client ucm_client = {
        .name   = "ucm",
@@ -1310,9 +1310,9 @@ err:
        return;
 }
 
-static void ib_ucm_remove_one(struct ib_device *device)
+static void ib_ucm_remove_one(struct ib_device *device, void *client_data)
 {
-       struct ib_ucm_device *ucm_dev = ib_get_client_data(device, &ucm_client);
+       struct ib_ucm_device *ucm_dev = client_data;
 
        if (!ucm_dev)
                return;

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 35567fffaa4e330cf7e1b4963f86daf2132b5574..57f281f8d686224b7a40488330b78ac38943270c 100644 (file)
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -133,7 +133,7 @@ static DEFINE_SPINLOCK(port_lock);
 static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 
 static void ib_umad_add_one(struct ib_device *device);
-static void ib_umad_remove_one(struct ib_device *device);
+static void ib_umad_remove_one(struct ib_device *device, void *client_data);
 
 static void ib_umad_release_dev(struct kobject *kobj)
 {
@@ -1322,9 +1322,9 @@ free:
        kobject_put(&umad_dev->kobj);
 }
 
-static void ib_umad_remove_one(struct ib_device *device)
+static void ib_umad_remove_one(struct ib_device *device, void *client_data)
 {
-       struct ib_umad_device *umad_dev = ib_get_client_data(device, &umad_client);
+       struct ib_umad_device *umad_dev = client_data;
        int i;
 
        if (!umad_dev)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index f6eef2da7097980b7066c62f1746d7722064f8f3..46c92294afa515be02b76b00b9ad34b3075b14ed 100644 (file)
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -128,7 +128,7 @@ static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
 };
 
 static void ib_uverbs_add_one(struct ib_device *device);
-static void ib_uverbs_remove_one(struct ib_device *device);
+static void ib_uverbs_remove_one(struct ib_device *device, void *client_data);
 
 static void ib_uverbs_release_dev(struct kref *ref)
 {
@@ -948,9 +948,9 @@ err:
        return;
 }
 
-static void ib_uverbs_remove_one(struct ib_device *device)
+static void ib_uverbs_remove_one(struct ib_device *device, void *client_data)
 {
-       struct ib_uverbs_device *uverbs_dev = ib_get_client_data(device, &uverbs_client);
+       struct ib_uverbs_device *uverbs_dev = client_data;
 
        if (!uverbs_dev)
                return;

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index b2943c84a5dda0aecdd8904917f2ebbb02b9b013..cca1a0c91ec4363925e1fa3a03d43e1a912e1200 100644 (file)
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -89,7 +89,7 @@ struct workqueue_struct *ipoib_workqueue;
 struct ib_sa_client ipoib_sa_client;
 
 static void ipoib_add_one(struct ib_device *device);
-static void ipoib_remove_one(struct ib_device *device);
+static void ipoib_remove_one(struct ib_device *device, void *client_data);
 static void ipoib_neigh_reclaim(struct rcu_head *rp);
 
 static struct ib_client ipoib_client = {
@@ -1715,12 +1715,11 @@ static void ipoib_add_one(struct ib_device *device)
        ib_set_client_data(device, &ipoib_client, dev_list);
 }
 
-static void ipoib_remove_one(struct ib_device *device)
+static void ipoib_remove_one(struct ib_device *device, void *client_data)
 {
        struct ipoib_dev_priv *priv, *tmp;
-       struct list_head *dev_list;
+       struct list_head *dev_list = client_data;
 
-       dev_list = ib_get_client_data(device, &ipoib_client);
        if (!dev_list)
                return;
 

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 31a20b462266611299aeeae5cd51fd19b69b635e..7755df444cfdabfaed16b99a82813852ce679f5b 100644 (file)
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -131,7 +131,7 @@ MODULE_PARM_DESC(ch_count,
                 "Number of RDMA channels to use for communication with an SRP target. Using more than one channel improves performance if the HCA supports multiple completion vectors. The default value is the minimum of four times the number of online CPU sockets and the number of completion vectors supported by the HCA.");
 
 static void srp_add_one(struct ib_device *device);
-static void srp_remove_one(struct ib_device *device);
+static void srp_remove_one(struct ib_device *device, void *client_data);
 static void srp_recv_completion(struct ib_cq *cq, void *ch_ptr);
 static void srp_send_completion(struct ib_cq *cq, void *ch_ptr);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
@@ -3460,13 +3460,13 @@ free_attr:
        kfree(dev_attr);
 }
 
-static void srp_remove_one(struct ib_device *device)
+static void srp_remove_one(struct ib_device *device, void *client_data)
 {
        struct srp_device *srp_dev;
        struct srp_host *host, *tmp_host;
        struct srp_target_port *target;
 
-       srp_dev = ib_get_client_data(device, &srp_client);
+       srp_dev = client_data;
        if (!srp_dev)
                return;
 

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 60ff0a2390e5f02f7cffabb9a5154cc0c4b734e7..4c59ceb40fffcf94768b02276962f54afb1e8d5e 100644 (file)
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3326,12 +3326,11 @@ err:
 /**
  * srpt_remove_one() - InfiniBand device removal callback function.
  */
-static void srpt_remove_one(struct ib_device *device)
+static void srpt_remove_one(struct ib_device *device, void *client_data)
 {
-       struct srpt_device *sdev;
+       struct srpt_device *sdev = client_data;
        int i;
 
-       sdev = ib_get_client_data(device, &srpt_client);
        if (!sdev) {
                pr_info("%s(%s): nothing to do.\n", __func__, device->name);
                return;

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 7448a2740287cd538aefef774186dbb8f193e983..449609b70928039fc573c4ae1483cd23cf957274 100644 (file)
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1550,6 +1550,8 @@ struct ib_device {
 
        spinlock_t                    client_data_lock;
        struct list_head              core_list;
+       /* Access to the client_data_list is protected by the client_data_lock
+        * spinlock and the lists_rwsem read-write semaphore */
        struct list_head              client_data_list;
 
        struct ib_cache               cache;
@@ -1761,7 +1763,7 @@ struct ib_device {
 struct ib_client {
        char  *name;
        void (*add)   (struct ib_device *);
-       void (*remove)(struct ib_device *);
+       void (*remove)(struct ib_device *, void *client_data);
 
        struct list_head list;
 };

diff --git a/net/rds/ib.c b/net/rds/ib.c
index ba2dffeff60876ca669993d1863dcbb6cb76a740..348ac37c11616dd4e5c6cfff4b87dbd1e6adb9a9 100644 (file)
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -230,11 +230,10 @@ struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device)
  *
  * This can be called at any time and can be racing with any other RDS path.
  */
-static void rds_ib_remove_one(struct ib_device *device)
+static void rds_ib_remove_one(struct ib_device *device, void *client_data)
 {
-       struct rds_ib_device *rds_ibdev;
+       struct rds_ib_device *rds_ibdev = client_data;
 
-       rds_ibdev = ib_get_client_data(device, &rds_ib_client);
        if (!rds_ibdev)
                return;
 

diff --git a/net/rds/iw.c b/net/rds/iw.c
index 589935661d667d81b2f6159eb69c237f95329a63..7cc2f32a0cb3842393a28da38d276120c95adffe 100644 (file)
--- a/net/rds/iw.c
+++ b/net/rds/iw.c
@@ -125,12 +125,11 @@ free_attr:
        kfree(dev_attr);
 }
 
-static void rds_iw_remove_one(struct ib_device *device)
+static void rds_iw_remove_one(struct ib_device *device, void *client_data)
 {
-       struct rds_iw_device *rds_iwdev;
+       struct rds_iw_device *rds_iwdev = client_data;
        struct rds_iw_cm_id *i_cm_id, *next;
 
-       rds_iwdev = ib_get_client_data(device, &rds_iw_client);
        if (!rds_iwdev)
                return;