* [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-28 18:25 ` Jason Gunthorpe
2020-01-26 14:26 ` [PATCH rdma-next 2/7] RDMA/cma: Use helper function to enqueue resolve work item Leon Romanovsky
` (6 subsequent siblings)
7 siblings, 1 reply; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Below cited commit missed to consider AF_IB and loopback code flow in
rdma_resolve_addr().
This leads to unbalanced cm_id refcount in cma_work_handler() which
puts the refcount which was not incremented in rdma_resolve_addr().
A call trace is observed with such code flow.
BUG: unable to handle kernel NULL pointer dereference at (null)
[<ffffffff96b67e16>] __mutex_lock_slowpath+0x166/0x1d0
[<ffffffff96b6715f>] mutex_lock+0x1f/0x2f
[<ffffffffc0beabb5>] cma_work_handler+0x25/0xa0
[<ffffffff964b9ebf>] process_one_work+0x17f/0x440
[<ffffffff964baf56>] worker_thread+0x126/0x3c0
Hence, hold the cm_id reference when scheduling resolve work item.
Fixes: 722c7b2bfead ("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()")
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 55a9afacfedd..72f032160c4b 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -3148,6 +3148,7 @@ static int cma_resolve_loopback(struct rdma_id_private *id_priv)
rdma_addr_get_sgid(&id_priv->id.route.addr.dev_addr, &gid);
rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, &gid);
+ atomic_inc(&id_priv->refcount);
cma_init_resolve_addr_work(work, id_priv);
queue_work(cma_wq, &work->work);
return 0;
@@ -3174,6 +3175,7 @@ static int cma_resolve_ib_addr(struct rdma_id_private *id_priv)
rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, (union ib_gid *)
&(((struct sockaddr_ib *) &id_priv->id.route.addr.dst_addr)->sib_addr));
+ atomic_inc(&id_priv->refcount);
cma_init_resolve_addr_work(work, id_priv);
queue_work(cma_wq, &work->work);
return 0;
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve
2020-01-26 14:26 ` [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve Leon Romanovsky
@ 2020-01-28 18:25 ` Jason Gunthorpe
0 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2020-01-28 18:25 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Parav Pandit
On Sun, Jan 26, 2020 at 04:26:46PM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav@mellanox.com>
>
> Below cited commit missed to consider AF_IB and loopback code flow in
> rdma_resolve_addr().
> This leads to unbalanced cm_id refcount in cma_work_handler() which
> puts the refcount which was not incremented in rdma_resolve_addr().
>
> A call trace is observed with such code flow.
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> [<ffffffff96b67e16>] __mutex_lock_slowpath+0x166/0x1d0
> [<ffffffff96b6715f>] mutex_lock+0x1f/0x2f
> [<ffffffffc0beabb5>] cma_work_handler+0x25/0xa0
> [<ffffffff964b9ebf>] process_one_work+0x17f/0x440
> [<ffffffff964baf56>] worker_thread+0x126/0x3c0
>
> Hence, hold the cm_id reference when scheduling resolve work item.
>
> Fixes: 722c7b2bfead ("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()")
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/cma.c | 2 ++
> 1 file changed, 2 insertions(+)
It is very hard to read this flow around the 'work', the incr of the
refcount in rdma_resolve_route() seems very poorly placed.
But this looks correct, so applied to for-next
Thanks,
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH rdma-next 2/7] RDMA/cma: Use helper function to enqueue resolve work item
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 3/7] RDMA/cma: Use RDMA device port iterator Leon Romanovsky
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
To avoid errors, to attach ownership of work item and its cm_id
refcount which is decremented in work handler, tie them up in single
helper function. Also avoid code duplication.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 72f032160c4b..8f16ebb413c2 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -2687,14 +2687,18 @@ static void cma_init_resolve_route_work(struct cma_work *work,
work->event.event = RDMA_CM_EVENT_ROUTE_RESOLVED;
}
-static void cma_init_resolve_addr_work(struct cma_work *work,
- struct rdma_id_private *id_priv)
+static void enqueue_resolve_addr_work(struct cma_work *work,
+ struct rdma_id_private *id_priv)
{
+ atomic_inc(&id_priv->refcount);
+
work->id = id_priv;
INIT_WORK(&work->work, cma_work_handler);
work->old_state = RDMA_CM_ADDR_QUERY;
work->new_state = RDMA_CM_ADDR_RESOLVED;
work->event.event = RDMA_CM_EVENT_ADDR_RESOLVED;
+
+ queue_work(cma_wq, &work->work);
}
static int cma_resolve_ib_route(struct rdma_id_private *id_priv,
@@ -3148,9 +3152,7 @@ static int cma_resolve_loopback(struct rdma_id_private *id_priv)
rdma_addr_get_sgid(&id_priv->id.route.addr.dev_addr, &gid);
rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, &gid);
- atomic_inc(&id_priv->refcount);
- cma_init_resolve_addr_work(work, id_priv);
- queue_work(cma_wq, &work->work);
+ enqueue_resolve_addr_work(work, id_priv);
return 0;
err:
kfree(work);
@@ -3175,9 +3177,7 @@ static int cma_resolve_ib_addr(struct rdma_id_private *id_priv)
rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, (union ib_gid *)
&(((struct sockaddr_ib *) &id_priv->id.route.addr.dst_addr)->sib_addr));
- atomic_inc(&id_priv->refcount);
- cma_init_resolve_addr_work(work, id_priv);
- queue_work(cma_wq, &work->work);
+ enqueue_resolve_addr_work(work, id_priv);
return 0;
err:
kfree(work);
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH rdma-next 3/7] RDMA/cma: Use RDMA device port iterator
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 2/7] RDMA/cma: Use helper function to enqueue resolve work item Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 4/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put Leon Romanovsky
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Use RDMA device port iterator and avoid open coding.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 8f16ebb413c2..34c62eae08d8 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -728,8 +728,8 @@ static int cma_iw_acquire_dev(struct rdma_id_private *id_priv,
struct cma_device *cma_dev;
enum ib_gid_type gid_type;
int ret = -ENODEV;
+ unsigned int port;
union ib_gid gid;
- u8 port;
if (dev_addr->dev_type != ARPHRD_INFINIBAND &&
id_priv->id.ps == RDMA_PS_IPOIB)
@@ -753,7 +753,8 @@ static int cma_iw_acquire_dev(struct rdma_id_private *id_priv,
}
list_for_each_entry(cma_dev, &dev_list, list) {
- for (port = 1; port <= cma_dev->device->phys_port_cnt; ++port) {
+ rdma_for_each_port (cma_dev->device, port) {
+
if (listen_id_priv->cma_dev == cma_dev &&
listen_id_priv->id.port_num == port)
continue;
@@ -786,8 +787,8 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
struct cma_device *cma_dev, *cur_dev;
struct sockaddr_ib *addr;
union ib_gid gid, sgid, *dgid;
+ unsigned int p;
u16 pkey, index;
- u8 p;
enum ib_port_state port_state;
int i;
@@ -798,7 +799,7 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
mutex_lock(&lock);
list_for_each_entry(cur_dev, &dev_list, list) {
- for (p = 1; p <= cur_dev->device->phys_port_cnt; ++p) {
+ rdma_for_each_port (cur_dev->device, p) {
if (!rdma_cap_af_ib(cur_dev->device, p))
continue;
@@ -3029,9 +3030,9 @@ static int cma_bind_loopback(struct rdma_id_private *id_priv)
struct cma_device *cma_dev, *cur_dev;
union ib_gid gid;
enum ib_port_state port_state;
+ unsigned int p;
u16 pkey;
int ret;
- u8 p;
cma_dev = NULL;
mutex_lock(&lock);
@@ -3043,7 +3044,7 @@ static int cma_bind_loopback(struct rdma_id_private *id_priv)
if (!cma_dev)
cma_dev = cur_dev;
- for (p = 1; p <= cur_dev->device->phys_port_cnt; ++p) {
+ rdma_for_each_port (cur_dev->device, p) {
if (!ib_get_cached_port_state(cur_dev->device, p, &port_state) &&
port_state == IB_PORT_ACTIVE) {
cma_dev = cur_dev;
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH rdma-next 4/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
` (2 preceding siblings ...)
2020-01-26 14:26 ` [PATCH rdma-next 3/7] RDMA/cma: Use RDMA device port iterator Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 5/7] RDMA/cma: Use refcount API to reflect refcount Leon Romanovsky
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Helper functions which increment/decrement reference count of the
structure are read better when they are named with get/put suffix.
Hence, rename cma_ref/deref_dev() to cma_dev_get/put().
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 22 +++++++++++-----------
drivers/infiniband/core/cma_configfs.c | 6 +++---
drivers/infiniband/core/cma_priv.h | 4 ++--
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 34c62eae08d8..7e16d1b001ff 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -247,11 +247,17 @@ enum {
CMA_OPTION_AFONLY,
};
-void cma_ref_dev(struct cma_device *cma_dev)
+void cma_dev_get(struct cma_device *cma_dev)
{
atomic_inc(&cma_dev->refcount);
}
+void cma_dev_put(struct cma_device *cma_dev)
+{
+ if (atomic_dec_and_test(&cma_dev->refcount))
+ complete(&cma_dev->comp);
+}
+
struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter filter,
void *cookie)
{
@@ -267,7 +273,7 @@ struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter filter,
}
if (found_cma_dev)
- cma_ref_dev(found_cma_dev);
+ cma_dev_get(found_cma_dev);
mutex_unlock(&lock);
return found_cma_dev;
}
@@ -463,7 +469,7 @@ static int cma_igmp_send(struct net_device *ndev, union ib_gid *mgid, bool join)
static void _cma_attach_to_dev(struct rdma_id_private *id_priv,
struct cma_device *cma_dev)
{
- cma_ref_dev(cma_dev);
+ cma_dev_get(cma_dev);
id_priv->cma_dev = cma_dev;
id_priv->id.device = cma_dev->device;
id_priv->id.route.addr.dev_addr.transport =
@@ -484,12 +490,6 @@ static void cma_attach_to_dev(struct rdma_id_private *id_priv,
rdma_start_port(cma_dev->device)];
}
-void cma_deref_dev(struct cma_device *cma_dev)
-{
- if (atomic_dec_and_test(&cma_dev->refcount))
- complete(&cma_dev->comp);
-}
-
static inline void release_mc(struct kref *kref)
{
struct cma_multicast *mc = container_of(kref, struct cma_multicast, mcref);
@@ -502,7 +502,7 @@ 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);
+ cma_dev_put(id_priv->cma_dev);
id_priv->cma_dev = NULL;
mutex_unlock(&lock);
}
@@ -4728,7 +4728,7 @@ static void cma_process_remove(struct cma_device *cma_dev)
}
mutex_unlock(&lock);
- cma_deref_dev(cma_dev);
+ cma_dev_put(cma_dev);
wait_for_completion(&cma_dev->comp);
}
diff --git a/drivers/infiniband/core/cma_configfs.c b/drivers/infiniband/core/cma_configfs.c
index 8b0b5ae22e4c..c672a4978bfd 100644
--- a/drivers/infiniband/core/cma_configfs.c
+++ b/drivers/infiniband/core/cma_configfs.c
@@ -94,7 +94,7 @@ static int cma_configfs_params_get(struct config_item *item,
static void cma_configfs_params_put(struct cma_device *cma_dev)
{
- cma_deref_dev(cma_dev);
+ cma_dev_put(cma_dev);
}
static ssize_t default_roce_mode_show(struct config_item *item,
@@ -312,12 +312,12 @@ static struct config_group *make_cma_dev(struct config_group *group,
configfs_add_default_group(&cma_dev_group->ports_group,
&cma_dev_group->device_group);
- cma_deref_dev(cma_dev);
+ cma_dev_put(cma_dev);
return &cma_dev_group->device_group;
fail:
if (cma_dev)
- cma_deref_dev(cma_dev);
+ cma_dev_put(cma_dev);
kfree(cma_dev_group);
return ERR_PTR(err);
}
diff --git a/drivers/infiniband/core/cma_priv.h b/drivers/infiniband/core/cma_priv.h
index ca7307277518..4e04c442ff86 100644
--- a/drivers/infiniband/core/cma_priv.h
+++ b/drivers/infiniband/core/cma_priv.h
@@ -111,8 +111,8 @@ static inline void cma_configfs_exit(void)
}
#endif
-void cma_ref_dev(struct cma_device *dev);
-void cma_deref_dev(struct cma_device *dev);
+void cma_dev_get(struct cma_device *dev);
+void cma_dev_put(struct cma_device *dev);
typedef bool (*cma_device_filter)(struct ib_device *, void *);
struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter filter,
void *cookie);
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH rdma-next 5/7] RDMA/cma: Use refcount API to reflect refcount
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
` (3 preceding siblings ...)
2020-01-26 14:26 ` [PATCH rdma-next 4/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 6/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put Leon Romanovsky
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Use the refcount variant to capture the reference counting of the cma
device structure.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 7e16d1b001ff..a5fd44194d77 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -199,7 +199,7 @@ struct cma_device {
struct list_head list;
struct ib_device *device;
struct completion comp;
- atomic_t refcount;
+ refcount_t refcount;
struct list_head id_list;
enum ib_gid_type *default_gid_type;
u8 *default_roce_tos;
@@ -249,12 +249,12 @@ enum {
void cma_dev_get(struct cma_device *cma_dev)
{
- atomic_inc(&cma_dev->refcount);
+ refcount_inc(&cma_dev->refcount);
}
void cma_dev_put(struct cma_device *cma_dev)
{
- if (atomic_dec_and_test(&cma_dev->refcount))
+ if (refcount_dec_and_test(&cma_dev->refcount))
complete(&cma_dev->comp);
}
@@ -4657,7 +4657,7 @@ static void cma_add_one(struct ib_device *device)
}
init_completion(&cma_dev->comp);
- atomic_set(&cma_dev->refcount, 1);
+ refcount_set(&cma_dev->refcount, 1);
INIT_LIST_HEAD(&cma_dev->id_list);
ib_set_client_data(device, &cma_client, cma_dev);
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH rdma-next 6/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
` (4 preceding siblings ...)
2020-01-26 14:26 ` [PATCH rdma-next 5/7] RDMA/cma: Use refcount API to reflect refcount Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-01-26 14:26 ` [PATCH rdma-next 7/7] RDMA/cma: Use refcount API to reflect refcount Leon Romanovsky
2020-02-11 18:01 ` [PATCH rdma-next 0/7] CMA fix and small improvements Jason Gunthorpe
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Helper functions which increment/decrement reference count of the
structure are read better when they are named with get/put suffix.
Hence, rename cma_ref/deref_id() to cma_id_get/put().
Also use cma_get_id() wrapper to find the balancing put() calls.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 42 ++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index a5fd44194d77..d6355e21cc87 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -841,7 +841,12 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
return 0;
}
-static void cma_deref_id(struct rdma_id_private *id_priv)
+static void cma_id_get(struct rdma_id_private *id_priv)
+{
+ atomic_inc(&id_priv->refcount);
+}
+
+static void cma_id_put(struct rdma_id_private *id_priv)
{
if (atomic_dec_and_test(&id_priv->refcount))
complete(&id_priv->comp);
@@ -1847,11 +1852,11 @@ void rdma_destroy_id(struct rdma_cm_id *id)
}
cma_release_port(id_priv);
- cma_deref_id(id_priv);
+ cma_id_put(id_priv);
wait_for_completion(&id_priv->comp);
if (id_priv->internal_id)
- cma_deref_id(id_priv->id.context);
+ cma_id_put(id_priv->id.context);
kfree(id_priv->id.route.path_rec);
@@ -2188,7 +2193,7 @@ static int cma_ib_req_handler(struct ib_cm_id *cm_id,
* Protect against the user destroying conn_id from another thread
* until we're done accessing it.
*/
- atomic_inc(&conn_id->refcount);
+ cma_id_get(conn_id);
ret = cma_cm_event_handler(conn_id, &event);
if (ret)
goto err3;
@@ -2205,13 +2210,13 @@ static int cma_ib_req_handler(struct ib_cm_id *cm_id,
mutex_unlock(&lock);
mutex_unlock(&conn_id->handler_mutex);
mutex_unlock(&listen_id->handler_mutex);
- cma_deref_id(conn_id);
+ cma_id_put(conn_id);
if (net_dev)
dev_put(net_dev);
return 0;
err3:
- cma_deref_id(conn_id);
+ cma_id_put(conn_id);
/* Destroy the CM ID by returning a non-zero value. */
conn_id->cm_id.ib = NULL;
err2:
@@ -2392,7 +2397,7 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
* Protect against the user destroying conn_id from another thread
* until we're done accessing it.
*/
- atomic_inc(&conn_id->refcount);
+ cma_id_get(conn_id);
ret = cma_cm_event_handler(conn_id, &event);
if (ret) {
/* User wants to destroy the CM ID */
@@ -2400,13 +2405,13 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
cma_exch(conn_id, RDMA_CM_DESTROYING);
mutex_unlock(&conn_id->handler_mutex);
mutex_unlock(&listen_id->handler_mutex);
- cma_deref_id(conn_id);
+ cma_id_put(conn_id);
rdma_destroy_id(&conn_id->id);
return ret;
}
mutex_unlock(&conn_id->handler_mutex);
- cma_deref_id(conn_id);
+ cma_id_put(conn_id);
out:
mutex_unlock(&listen_id->handler_mutex);
@@ -2493,7 +2498,7 @@ static void cma_listen_on_dev(struct rdma_id_private *id_priv,
_cma_attach_to_dev(dev_id_priv, cma_dev);
list_add_tail(&dev_id_priv->listen_list, &id_priv->listen_list);
- atomic_inc(&id_priv->refcount);
+ cma_id_get(id_priv);
dev_id_priv->internal_id = 1;
dev_id_priv->afonly = id_priv->afonly;
dev_id_priv->tos_set = id_priv->tos_set;
@@ -2648,7 +2653,7 @@ static void cma_work_handler(struct work_struct *_work)
}
out:
mutex_unlock(&id_priv->handler_mutex);
- cma_deref_id(id_priv);
+ cma_id_put(id_priv);
if (destroy)
rdma_destroy_id(&id_priv->id);
kfree(work);
@@ -2672,7 +2677,7 @@ static void cma_ndev_work_handler(struct work_struct *_work)
out:
mutex_unlock(&id_priv->handler_mutex);
- cma_deref_id(id_priv);
+ cma_id_put(id_priv);
if (destroy)
rdma_destroy_id(&id_priv->id);
kfree(work);
@@ -2691,7 +2696,8 @@ static void cma_init_resolve_route_work(struct cma_work *work,
static void enqueue_resolve_addr_work(struct cma_work *work,
struct rdma_id_private *id_priv)
{
- atomic_inc(&id_priv->refcount);
+ /* Balances with cma_id_put() in cma_work_handler */
+ cma_id_get(id_priv);
work->id = id_priv;
INIT_WORK(&work->work, cma_work_handler);
@@ -2987,7 +2993,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_RESOLVED, RDMA_CM_ROUTE_QUERY))
return -EINVAL;
- atomic_inc(&id_priv->refcount);
+ cma_id_get(id_priv);
if (rdma_cap_ib_sa(id->device, id->port_num))
ret = cma_resolve_ib_route(id_priv, timeout_ms);
else if (rdma_protocol_roce(id->device, id->port_num))
@@ -3003,7 +3009,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
return 0;
err:
cma_comp_exch(id_priv, RDMA_CM_ROUTE_QUERY, RDMA_CM_ADDR_RESOLVED);
- cma_deref_id(id_priv);
+ cma_id_put(id_priv);
return ret;
}
EXPORT_SYMBOL(rdma_resolve_route);
@@ -4582,7 +4588,7 @@ static int cma_netdev_change(struct net_device *ndev, struct rdma_id_private *id
INIT_WORK(&work->work, cma_ndev_work_handler);
work->id = id_priv;
work->event.event = RDMA_CM_EVENT_ADDR_CHANGE;
- atomic_inc(&id_priv->refcount);
+ cma_id_get(id_priv);
queue_work(cma_wq, &work->work);
}
@@ -4716,11 +4722,11 @@ static void cma_process_remove(struct cma_device *cma_dev)
list_del(&id_priv->listen_list);
list_del_init(&id_priv->list);
- atomic_inc(&id_priv->refcount);
+ cma_id_get(id_priv);
mutex_unlock(&lock);
ret = id_priv->internal_id ? 1 : cma_remove_id_dev(id_priv);
- cma_deref_id(id_priv);
+ cma_id_put(id_priv);
if (ret)
rdma_destroy_id(&id_priv->id);
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH rdma-next 7/7] RDMA/cma: Use refcount API to reflect refcount
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
` (5 preceding siblings ...)
2020-01-26 14:26 ` [PATCH rdma-next 6/7] RDMA/cma: Rename cma_device ref/deref helpers to to get/put Leon Romanovsky
@ 2020-01-26 14:26 ` Leon Romanovsky
2020-02-11 18:01 ` [PATCH rdma-next 0/7] CMA fix and small improvements Jason Gunthorpe
7 siblings, 0 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-01-26 14:26 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Parav Pandit
From: Parav Pandit <parav@mellanox.com>
Use the refcount variant to capture the reference counting of the
cm_id structure.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/cma.c | 6 +++---
drivers/infiniband/core/cma_priv.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index d6355e21cc87..af96675e31c4 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -843,12 +843,12 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
static void cma_id_get(struct rdma_id_private *id_priv)
{
- atomic_inc(&id_priv->refcount);
+ refcount_inc(&id_priv->refcount);
}
static void cma_id_put(struct rdma_id_private *id_priv)
{
- if (atomic_dec_and_test(&id_priv->refcount))
+ if (refcount_dec_and_test(&id_priv->refcount))
complete(&id_priv->comp);
}
@@ -876,7 +876,7 @@ struct rdma_cm_id *__rdma_create_id(struct net *net,
spin_lock_init(&id_priv->lock);
mutex_init(&id_priv->qp_mutex);
init_completion(&id_priv->comp);
- atomic_set(&id_priv->refcount, 1);
+ refcount_set(&id_priv->refcount, 1);
mutex_init(&id_priv->handler_mutex);
INIT_LIST_HEAD(&id_priv->listen_list);
INIT_LIST_HEAD(&id_priv->mc_list);
diff --git a/drivers/infiniband/core/cma_priv.h b/drivers/infiniband/core/cma_priv.h
index 4e04c442ff86..5edcf44a9307 100644
--- a/drivers/infiniband/core/cma_priv.h
+++ b/drivers/infiniband/core/cma_priv.h
@@ -66,7 +66,7 @@ struct rdma_id_private {
struct mutex qp_mutex;
struct completion comp;
- atomic_t refcount;
+ refcount_t refcount;
struct mutex handler_mutex;
int backlog;
--
2.24.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH rdma-next 0/7] CMA fix and small improvements
2020-01-26 14:26 [PATCH rdma-next 0/7] CMA fix and small improvements Leon Romanovsky
` (6 preceding siblings ...)
2020-01-26 14:26 ` [PATCH rdma-next 7/7] RDMA/cma: Use refcount API to reflect refcount Leon Romanovsky
@ 2020-02-11 18:01 ` Jason Gunthorpe
7 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2020-02-11 18:01 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Parav Pandit
On Sun, Jan 26, 2020 at 04:26:45PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> >From Parav,
>
> This series covers a fix for a reference count leak and few small
> code improvements to the RDMA CM code as below.
>
> Patch-1: Fixes a reference count leak where reference count
> increment was missing.
> Patch-2: Uses helper function to hold refcount and to enqueue
> work to avoid errors.
> Patch-3: Uses RDMA port iterator API and avoids open coding.
> Patch-4: Renames cma device's cma_ref/deref_dev() to cma_dev_get/put()
> to align it to rest of kernel for similar use.
> Patch-5: Uses refcount APIs to get/put reference to CMA device.
> Patch-6: Renames cma cm_id's ref helpers to cma_id_get/put() to align
> to rest of the kernel for similar use.
> Patch-7: Uses refcount APIs to get/put reference to CM id.
>
> Thanks
>
> Parav Pandit (7):
> RDMA/cma: Use helper function to enqueue resolve work item
> RDMA/cma: Use RDMA device port iterator
> RDMA/cma: Rename cma_device ref/deref helpers to to get/put
> RDMA/cma: Use refcount API to reflect refcount
> RDMA/cma: Rename cma_device ref/deref helpers to to get/put
> RDMA/cma: Use refcount API to reflect refcount
Applied to for-next
Thanks,
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread