[PATCH for-next 0/6] RDMA: Use refcount_t for reference counting

[PATCH for-next 0/6] RDMA: Use refcount_t for reference counting

[Weihang Li]

There are some refcnt in type of atomic_t in RDMA subsystem, almost all of
them is wrote before the refcount_t is acheived in kernel. refcount_t is
better than integer for reference counting, it will WARN on
overflow/underflow and avoid use-after-free risks.

Weihang Li (6):
  RDMA/core: Use refcount_t instead of atomic_t for reference counting
  RDMA/hns: Use refcount_t instead of atomic_t for reference counting
  RDMA/hns: Use refcount_t APIs for HEM
  RDMA/cxgb4: Use refcount_t instead of atomic_t for reference counting
  RDMA/i40iw: Use refcount_t instead of atomic_t for reference counting
  RDMA/ipoib: Use refcount_t instead of atomic_t for reference counting

 drivers/infiniband/core/iwcm.c              |  9 +++--
 drivers/infiniband/core/iwcm.h              |  2 +-
 drivers/infiniband/core/iwpm_util.c         | 12 ++++---
 drivers/infiniband/core/iwpm_util.h         |  2 +-
 drivers/infiniband/core/mad_priv.h          |  2 +-
 drivers/infiniband/core/multicast.c         | 30 ++++++++--------
 drivers/infiniband/core/uverbs.h            |  2 +-
 drivers/infiniband/core/uverbs_main.c       | 12 +++----
 drivers/infiniband/hw/cxgb4/cq.c            |  6 ++--
 drivers/infiniband/hw/cxgb4/ev.c            |  8 ++---
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h      |  2 +-
 drivers/infiniband/hw/hns/hns_roce_cq.c     |  8 ++---
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 ++--
 drivers/infiniband/hw/hns/hns_roce_hem.c    | 33 +++++++++---------
 drivers/infiniband/hw/hns/hns_roce_hem.h    |  4 +--
 drivers/infiniband/hw/hns/hns_roce_qp.c     | 12 +++----
 drivers/infiniband/hw/hns/hns_roce_srq.c    |  8 ++---
 drivers/infiniband/hw/i40iw/i40iw.h         |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_cm.c      | 54 ++++++++++++++---------------
 drivers/infiniband/hw/i40iw/i40iw_cm.h      |  4 +--
 drivers/infiniband/hw/i40iw/i40iw_main.c    |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_puda.h    |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_utils.c   | 10 +++---
 drivers/infiniband/ulp/ipoib/ipoib.h        |  4 +--
 drivers/infiniband/ulp/ipoib/ipoib_main.c   |  8 ++---
 25 files changed, 123 insertions(+), 121 deletions(-)

-- 
2.7.4

[PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[Weihang Li]

The refcount_t API will WARN on underflow and overflow of a reference
counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
regarded as there is a risk about use-after-free. So it should be set to 1
directly during initialization.

Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/core/iwcm.c        |  9 ++++-----
 drivers/infiniband/core/iwcm.h        |  2 +-
 drivers/infiniband/core/iwpm_util.c   | 12 ++++++++----
 drivers/infiniband/core/iwpm_util.h   |  2 +-
 drivers/infiniband/core/mad_priv.h    |  2 +-
 drivers/infiniband/core/multicast.c   | 30 +++++++++++++++---------------
 drivers/infiniband/core/uverbs.h      |  2 +-
 drivers/infiniband/core/uverbs_main.c | 12 ++++++------
 8 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index da8adad..4226115 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -211,8 +211,7 @@ static void free_cm_id(struct iwcm_id_private *cm_id_priv)
  */
 static int iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
 {
-	BUG_ON(atomic_read(&cm_id_priv->refcount)==0);
-	if (atomic_dec_and_test(&cm_id_priv->refcount)) {
+	if (refcount_dec_and_test(&cm_id_priv->refcount)) {
 		BUG_ON(!list_empty(&cm_id_priv->work_list));
 		free_cm_id(cm_id_priv);
 		return 1;
@@ -225,7 +224,7 @@ static void add_ref(struct iw_cm_id *cm_id)
 {
 	struct iwcm_id_private *cm_id_priv;
 	cm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
-	atomic_inc(&cm_id_priv->refcount);
+	refcount_inc(&cm_id_priv->refcount);
 }
 
 static void rem_ref(struct iw_cm_id *cm_id)
@@ -257,7 +256,7 @@ struct iw_cm_id *iw_create_cm_id(struct ib_device *device,
 	cm_id_priv->id.add_ref = add_ref;
 	cm_id_priv->id.rem_ref = rem_ref;
 	spin_lock_init(&cm_id_priv->lock);
-	atomic_set(&cm_id_priv->refcount, 1);
+	refcount_set(&cm_id_priv->refcount, 1);
 	init_waitqueue_head(&cm_id_priv->connect_wait);
 	init_completion(&cm_id_priv->destroy_comp);
 	INIT_LIST_HEAD(&cm_id_priv->work_list);
@@ -1094,7 +1093,7 @@ static int cm_event_handler(struct iw_cm_id *cm_id,
 		}
 	}
 
-	atomic_inc(&cm_id_priv->refcount);
+	refcount_inc(&cm_id_priv->refcount);
 	if (list_empty(&cm_id_priv->work_list)) {
 		list_add_tail(&work->list, &cm_id_priv->work_list);
 		queue_work(iwcm_wq, &work->work);
diff --git a/drivers/infiniband/core/iwcm.h b/drivers/infiniband/core/iwcm.h
index 82c2cd1..bf74639 100644
--- a/drivers/infiniband/core/iwcm.h
+++ b/drivers/infiniband/core/iwcm.h
@@ -52,7 +52,7 @@ struct iwcm_id_private {
 	wait_queue_head_t connect_wait;
 	struct list_head work_list;
 	spinlock_t lock;
-	atomic_t refcount;
+	refcount_t refcount;
 	struct list_head work_free_list;
 };
 
diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c
index f80e555..b8f40e6 100644
--- a/drivers/infiniband/core/iwpm_util.c
+++ b/drivers/infiniband/core/iwpm_util.c
@@ -61,7 +61,7 @@ int iwpm_init(u8 nl_client)
 {
 	int ret = 0;
 	mutex_lock(&iwpm_admin_lock);
-	if (atomic_read(&iwpm_admin.refcount) == 0) {
+	if (!refcount_read(&iwpm_admin.refcount)) {
 		iwpm_hash_bucket = kcalloc(IWPM_MAPINFO_HASH_SIZE,
 					   sizeof(struct hlist_head),
 					   GFP_KERNEL);
@@ -77,8 +77,12 @@ int iwpm_init(u8 nl_client)
 			ret = -ENOMEM;
 			goto init_exit;
 		}
+
+		refcount_set(&iwpm_admin.refcount, 1);
+	} else {
+		refcount_inc(&iwpm_admin.refcount);
 	}
-	atomic_inc(&iwpm_admin.refcount);
+
 init_exit:
 	mutex_unlock(&iwpm_admin_lock);
 	if (!ret) {
@@ -105,12 +109,12 @@ int iwpm_exit(u8 nl_client)
 	if (!iwpm_valid_client(nl_client))
 		return -EINVAL;
 	mutex_lock(&iwpm_admin_lock);
-	if (atomic_read(&iwpm_admin.refcount) == 0) {
+	if (!refcount_read(&iwpm_admin.refcount)) {
 		mutex_unlock(&iwpm_admin_lock);
 		pr_err("%s Incorrect usage - negative refcount\n", __func__);
 		return -EINVAL;
 	}
-	if (atomic_dec_and_test(&iwpm_admin.refcount)) {
+	if (refcount_dec_and_test(&iwpm_admin.refcount)) {
 		free_hash_bucket();
 		free_reminfo_bucket();
 		pr_debug("%s: Resources are destroyed\n", __func__);
diff --git a/drivers/infiniband/core/iwpm_util.h b/drivers/infiniband/core/iwpm_util.h
index eeb8e60..5002ac6 100644
--- a/drivers/infiniband/core/iwpm_util.h
+++ b/drivers/infiniband/core/iwpm_util.h
@@ -90,7 +90,7 @@ struct iwpm_remote_info {
 };
 
 struct iwpm_admin_data {
-	atomic_t refcount;
+	refcount_t refcount;
 	atomic_t nlmsg_seq;
 	int      client_list[RDMA_NL_NUM_CLIENTS];
 	u32      reg_list[RDMA_NL_NUM_CLIENTS];
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 4aa16b3..25e573d 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -115,7 +115,7 @@ struct ib_mad_snoop_private {
 	struct ib_mad_qp_info *qp_info;
 	int snoop_index;
 	int mad_snoop_flags;
-	atomic_t refcount;
+	refcount_t refcount;
 	struct completion comp;
 };
 
diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c
index a5dd4b7..54bbe65 100644
--- a/drivers/infiniband/core/multicast.c
+++ b/drivers/infiniband/core/multicast.c
@@ -61,7 +61,7 @@ struct mcast_port {
 	struct mcast_device	*dev;
 	spinlock_t		lock;
 	struct rb_root		table;
-	atomic_t		refcount;
+	refcount_t		refcount;
 	struct completion	comp;
 	u32			port_num;
 };
@@ -103,7 +103,7 @@ struct mcast_group {
 	struct list_head	active_list;
 	struct mcast_member	*last_join;
 	int			members[NUM_JOIN_MEMBERSHIP_TYPES];
-	atomic_t		refcount;
+	refcount_t		refcount;
 	enum mcast_group_state	state;
 	struct ib_sa_query	*query;
 	u16			pkey_index;
@@ -117,7 +117,7 @@ struct mcast_member {
 	struct mcast_group	*group;
 	struct list_head	list;
 	enum mcast_state	state;
-	atomic_t		refcount;
+	refcount_t		refcount;
 	struct completion	comp;
 };
 
@@ -178,7 +178,7 @@ static struct mcast_group *mcast_insert(struct mcast_port *port,
 
 static void deref_port(struct mcast_port *port)
 {
-	if (atomic_dec_and_test(&port->refcount))
+	if (refcount_dec_and_test(&port->refcount))
 		complete(&port->comp);
 }
 
@@ -188,7 +188,7 @@ static void release_group(struct mcast_group *group)
 	unsigned long flags;
 
 	spin_lock_irqsave(&port->lock, flags);
-	if (atomic_dec_and_test(&group->refcount)) {
+	if (refcount_dec_and_test(&group->refcount)) {
 		rb_erase(&group->node, &port->table);
 		spin_unlock_irqrestore(&port->lock, flags);
 		kfree(group);
@@ -199,7 +199,7 @@ static void release_group(struct mcast_group *group)
 
 static void deref_member(struct mcast_member *member)
 {
-	if (atomic_dec_and_test(&member->refcount))
+	if (refcount_dec_and_test(&member->refcount))
 		complete(&member->comp);
 }
 
@@ -212,7 +212,7 @@ static void queue_join(struct mcast_member *member)
 	list_add_tail(&member->list, &group->pending_list);
 	if (group->state == MCAST_IDLE) {
 		group->state = MCAST_BUSY;
-		atomic_inc(&group->refcount);
+		refcount_inc(&group->refcount);
 		queue_work(mcast_wq, &group->work);
 	}
 	spin_unlock_irqrestore(&group->lock, flags);
@@ -401,7 +401,7 @@ static void process_group_error(struct mcast_group *group)
 	while (!list_empty(&group->active_list)) {
 		member = list_entry(group->active_list.next,
 				    struct mcast_member, list);
-		atomic_inc(&member->refcount);
+		refcount_inc(&member->refcount);
 		list_del_init(&member->list);
 		adjust_membership(group, member->multicast.rec.join_state, -1);
 		member->state = MCAST_ERROR;
@@ -445,7 +445,7 @@ static void mcast_work_handler(struct work_struct *work)
 				    struct mcast_member, list);
 		multicast = &member->multicast;
 		join_state = multicast->rec.join_state;
-		atomic_inc(&member->refcount);
+		refcount_inc(&member->refcount);
 
 		if (join_state == (group->rec.join_state & join_state)) {
 			status = cmp_rec(&group->rec, &multicast->rec,
@@ -497,7 +497,7 @@ static void process_join_error(struct mcast_group *group, int status)
 	member = list_entry(group->pending_list.next,
 			    struct mcast_member, list);
 	if (group->last_join == member) {
-		atomic_inc(&member->refcount);
+		refcount_inc(&member->refcount);
 		list_del_init(&member->list);
 		spin_unlock_irq(&group->lock);
 		ret = member->multicast.callback(status, &member->multicast);
@@ -589,9 +589,9 @@ static struct mcast_group *acquire_group(struct mcast_port *port,
 		kfree(group);
 		group = cur_group;
 	} else
-		atomic_inc(&port->refcount);
+		refcount_inc(&port->refcount);
 found:
-	atomic_inc(&group->refcount);
+	refcount_inc(&group->refcount);
 	spin_unlock_irqrestore(&port->lock, flags);
 	return group;
 }
@@ -632,7 +632,7 @@ ib_sa_join_multicast(struct ib_sa_client *client,
 	member->multicast.callback = callback;
 	member->multicast.context = context;
 	init_completion(&member->comp);
-	atomic_set(&member->refcount, 1);
+	refcount_set(&member->refcount, 1);
 	member->state = MCAST_JOINING;
 
 	member->group = acquire_group(&dev->port[port_num - dev->start_port],
@@ -780,7 +780,7 @@ static void mcast_groups_event(struct mcast_port *port,
 		group = rb_entry(node, struct mcast_group, node);
 		spin_lock(&group->lock);
 		if (group->state == MCAST_IDLE) {
-			atomic_inc(&group->refcount);
+			refcount_inc(&group->refcount);
 			queue_work(mcast_wq, &group->work);
 		}
 		if (group->state != MCAST_GROUP_ERROR)
@@ -840,7 +840,7 @@ static int mcast_add_one(struct ib_device *device)
 		spin_lock_init(&port->lock);
 		port->table = RB_ROOT;
 		init_completion(&port->comp);
-		atomic_set(&port->refcount, 1);
+		refcount_set(&port->refcount, 1);
 		++count;
 	}
 
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 53a1047..821d93c 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -97,7 +97,7 @@ ib_uverbs_init_udata_buf_or_null(struct ib_udata *udata,
  */
 
 struct ib_uverbs_device {
-	atomic_t				refcount;
+	refcount_t				refcount;
 	u32					num_comp_vectors;
 	struct completion			comp;
 	struct device				dev;
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index f173ecd..d544340 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -197,7 +197,7 @@ void ib_uverbs_release_file(struct kref *ref)
 		module_put(ib_dev->ops.owner);
 	srcu_read_unlock(&file->device->disassociate_srcu, srcu_key);
 
-	if (atomic_dec_and_test(&file->device->refcount))
+	if (refcount_dec_and_test(&file->device->refcount))
 		ib_uverbs_comp_dev(file->device);
 
 	if (file->default_async_file)
@@ -891,7 +891,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp)
 	int srcu_key;
 
 	dev = container_of(inode->i_cdev, struct ib_uverbs_device, cdev);
-	if (!atomic_inc_not_zero(&dev->refcount))
+	if (!refcount_inc_not_zero(&dev->refcount))
 		return -ENXIO;
 
 	get_device(&dev->dev);
@@ -955,7 +955,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp)
 err:
 	mutex_unlock(&dev->lists_mutex);
 	srcu_read_unlock(&dev->disassociate_srcu, srcu_key);
-	if (atomic_dec_and_test(&dev->refcount))
+	if (refcount_dec_and_test(&dev->refcount))
 		ib_uverbs_comp_dev(dev);
 
 	put_device(&dev->dev);
@@ -1124,7 +1124,7 @@ static int ib_uverbs_add_one(struct ib_device *device)
 	uverbs_dev->dev.release = ib_uverbs_release_dev;
 	uverbs_dev->groups[0] = &dev_attr_group;
 	uverbs_dev->dev.groups = uverbs_dev->groups;
-	atomic_set(&uverbs_dev->refcount, 1);
+	refcount_set(&uverbs_dev->refcount, 1);
 	init_completion(&uverbs_dev->comp);
 	uverbs_dev->xrcd_tree = RB_ROOT;
 	mutex_init(&uverbs_dev->xrcd_tree_mutex);
@@ -1166,7 +1166,7 @@ static int ib_uverbs_add_one(struct ib_device *device)
 err_uapi:
 	ida_free(&uverbs_ida, devnum);
 err:
-	if (atomic_dec_and_test(&uverbs_dev->refcount))
+	if (refcount_dec_and_test(&uverbs_dev->refcount))
 		ib_uverbs_comp_dev(uverbs_dev);
 	wait_for_completion(&uverbs_dev->comp);
 	put_device(&uverbs_dev->dev);
@@ -1229,7 +1229,7 @@ static void ib_uverbs_remove_one(struct ib_device *device, void *client_data)
 		wait_clients = 0;
 	}
 
-	if (atomic_dec_and_test(&uverbs_dev->refcount))
+	if (refcount_dec_and_test(&uverbs_dev->refcount))
 		ib_uverbs_comp_dev(uverbs_dev);
 	if (wait_clients)
 		wait_for_completion(&uverbs_dev->comp);
-- 
2.7.4

[PATCH for-next 2/6] RDMA/hns: Use refcount_t instead of atomic_t for reference counting

[Weihang Li]

The refcount_t API will WARN on underflow and overflow of a reference
counter, and avoid use-after-free risks, so the type of refcnt in
QP/CQ/SRQ is changed from atomic_t to refcount_t.

Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_cq.c     |  8 ++++----
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 +++---
 drivers/infiniband/hw/hns/hns_roce_qp.c     | 12 ++++++------
 drivers/infiniband/hw/hns/hns_roce_srq.c    |  8 ++++----
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_cq.c b/drivers/infiniband/hw/hns/hns_roce_cq.c
index 800884b..0763082 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cq.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cq.c
@@ -154,7 +154,7 @@ static int alloc_cqc(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq)
 	hr_cq->cons_index = 0;
 	hr_cq->arm_sn = 1;
 
-	atomic_set(&hr_cq->refcount, 1);
+	refcount_set(&hr_cq->refcount, 1);
 	init_completion(&hr_cq->free);
 
 	return 0;
@@ -188,7 +188,7 @@ static void free_cqc(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq)
 	synchronize_irq(hr_dev->eq_table.eq[hr_cq->vector].irq);
 
 	/* wait for all interrupt processed */
-	if (atomic_dec_and_test(&hr_cq->refcount))
+	if (refcount_dec_and_test(&hr_cq->refcount))
 		complete(&hr_cq->free);
 	wait_for_completion(&hr_cq->free);
 
@@ -481,7 +481,7 @@ void hns_roce_cq_event(struct hns_roce_dev *hr_dev, u32 cqn, int event_type)
 		return;
 	}
 
-	atomic_inc(&hr_cq->refcount);
+	refcount_inc(&hr_cq->refcount);
 
 	ibcq = &hr_cq->ib_cq;
 	if (ibcq->event_handler) {
@@ -491,7 +491,7 @@ void hns_roce_cq_event(struct hns_roce_dev *hr_dev, u32 cqn, int event_type)
 		ibcq->event_handler(&event, ibcq->cq_context);
 	}
 
-	if (atomic_dec_and_test(&hr_cq->refcount))
+	if (refcount_dec_and_test(&hr_cq->refcount))
 		complete(&hr_cq->free);
 }
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 97800d2..94c1268 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -446,7 +446,7 @@ struct hns_roce_cq {
 	int				cqe_size;
 	unsigned long			cqn;
 	u32				vector;
-	atomic_t			refcount;
+	refcount_t			refcount;
 	struct completion		free;
 	struct list_head		sq_list; /* all qps on this send cq */
 	struct list_head		rq_list; /* all qps on this recv cq */
@@ -473,7 +473,7 @@ struct hns_roce_srq {
 	u32			xrcdn;
 	void __iomem		*db_reg;
 
-	atomic_t		refcount;
+	refcount_t		refcount;
 	struct completion	free;
 
 	struct hns_roce_mtr	buf_mtr;
@@ -642,7 +642,7 @@ struct hns_roce_qp {
 
 	u32			xrcdn;
 
-	atomic_t		refcount;
+	refcount_t		refcount;
 	struct completion	free;
 
 	struct hns_roce_sge	sge;
diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index 230a909..6988ffa 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -65,7 +65,7 @@ static void flush_work_handle(struct work_struct *work)
 	 * make sure we signal QP destroy leg that flush QP was completed
 	 * so that it can safely proceed ahead now and destroy QP
 	 */
-	if (atomic_dec_and_test(&hr_qp->refcount))
+	if (refcount_dec_and_test(&hr_qp->refcount))
 		complete(&hr_qp->free);
 }
 
@@ -75,7 +75,7 @@ void init_flush_work(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
 
 	flush_work->hr_dev = hr_dev;
 	INIT_WORK(&flush_work->work, flush_work_handle);
-	atomic_inc(&hr_qp->refcount);
+	refcount_inc(&hr_qp->refcount);
 	queue_work(hr_dev->irq_workq, &flush_work->work);
 }
 
@@ -87,7 +87,7 @@ void hns_roce_qp_event(struct hns_roce_dev *hr_dev, u32 qpn, int event_type)
 	xa_lock(&hr_dev->qp_table_xa);
 	qp = __hns_roce_qp_lookup(hr_dev, qpn);
 	if (qp)
-		atomic_inc(&qp->refcount);
+		refcount_inc(&qp->refcount);
 	xa_unlock(&hr_dev->qp_table_xa);
 
 	if (!qp) {
@@ -108,7 +108,7 @@ void hns_roce_qp_event(struct hns_roce_dev *hr_dev, u32 qpn, int event_type)
 
 	qp->event(qp, (enum hns_roce_event)event_type);
 
-	if (atomic_dec_and_test(&qp->refcount))
+	if (refcount_dec_and_test(&qp->refcount))
 		complete(&qp->free);
 }
 
@@ -1076,7 +1076,7 @@ static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
 
 	hr_qp->ibqp.qp_num = hr_qp->qpn;
 	hr_qp->event = hns_roce_ib_qp_event;
-	atomic_set(&hr_qp->refcount, 1);
+	refcount_set(&hr_qp->refcount, 1);
 	init_completion(&hr_qp->free);
 
 	return 0;
@@ -1099,7 +1099,7 @@ static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
 void hns_roce_qp_destroy(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
 			 struct ib_udata *udata)
 {
-	if (atomic_dec_and_test(&hr_qp->refcount))
+	if (refcount_dec_and_test(&hr_qp->refcount))
 		complete(&hr_qp->free);
 	wait_for_completion(&hr_qp->free);
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_srq.c b/drivers/infiniband/hw/hns/hns_roce_srq.c
index 546d182..327f7aa 100644
--- a/drivers/infiniband/hw/hns/hns_roce_srq.c
+++ b/drivers/infiniband/hw/hns/hns_roce_srq.c
@@ -17,7 +17,7 @@ void hns_roce_srq_event(struct hns_roce_dev *hr_dev, u32 srqn, int event_type)
 	xa_lock(&srq_table->xa);
 	srq = xa_load(&srq_table->xa, srqn & (hr_dev->caps.num_srqs - 1));
 	if (srq)
-		atomic_inc(&srq->refcount);
+		refcount_inc(&srq->refcount);
 	xa_unlock(&srq_table->xa);
 
 	if (!srq) {
@@ -27,7 +27,7 @@ void hns_roce_srq_event(struct hns_roce_dev *hr_dev, u32 srqn, int event_type)
 
 	srq->event(srq, event_type);
 
-	if (atomic_dec_and_test(&srq->refcount))
+	if (refcount_dec_and_test(&srq->refcount))
 		complete(&srq->free);
 }
 
@@ -149,7 +149,7 @@ static void free_srqc(struct hns_roce_dev *hr_dev, struct hns_roce_srq *srq)
 
 	xa_erase(&srq_table->xa, srq->srqn);
 
-	if (atomic_dec_and_test(&srq->refcount))
+	if (refcount_dec_and_test(&srq->refcount))
 		complete(&srq->free);
 	wait_for_completion(&srq->free);
 
@@ -417,7 +417,7 @@ int hns_roce_create_srq(struct ib_srq *ib_srq,
 
 	srq->db_reg = hr_dev->reg_base + SRQ_DB_REG;
 	srq->event = hns_roce_ib_srq_event;
-	atomic_set(&srq->refcount, 1);
+	refcount_set(&srq->refcount, 1);
 	init_completion(&srq->free);
 
 	return 0;
-- 
2.7.4

[PATCH for-next 3/6] RDMA/hns: Use refcount_t APIs for HEM

[Weihang Li]

refcount_t is better than integer for reference counting, it will WARN on
overflow/underflow and avoid use-after-free risks.

Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hem.c | 33 ++++++++++++++++----------------
 drivers/infiniband/hw/hns/hns_roce_hem.h |  4 ++--
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
index cfd2e1b..84f3cd1 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hem.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
@@ -271,7 +271,7 @@ static struct hns_roce_hem *hns_roce_alloc_hem(struct hns_roce_dev *hr_dev,
 	if (!hem)
 		return NULL;
 
-	hem->refcount = 0;
+	refcount_set(&hem->refcount, 0);
 	INIT_LIST_HEAD(&hem->chunk_list);
 
 	order = get_order(hem_alloc_size);
@@ -618,7 +618,7 @@ static int hns_roce_table_mhop_get(struct hns_roce_dev *hr_dev,
 
 	mutex_lock(&table->mutex);
 	if (table->hem[index.buf]) {
-		++table->hem[index.buf]->refcount;
+		refcount_inc(&table->hem[index.buf]->refcount);
 		goto out;
 	}
 
@@ -637,7 +637,7 @@ static int hns_roce_table_mhop_get(struct hns_roce_dev *hr_dev,
 		}
 	}
 
-	++table->hem[index.buf]->refcount;
+	refcount_set(&table->hem[index.buf]->refcount, 1);
 	goto out;
 
 err_alloc:
@@ -663,7 +663,7 @@ int hns_roce_table_get(struct hns_roce_dev *hr_dev,
 	mutex_lock(&table->mutex);
 
 	if (table->hem[i]) {
-		++table->hem[i]->refcount;
+		refcount_inc(&table->hem[i]->refcount);
 		goto out;
 	}
 
@@ -686,7 +686,7 @@ int hns_roce_table_get(struct hns_roce_dev *hr_dev,
 		goto out;
 	}
 
-	++table->hem[i]->refcount;
+	refcount_set(&table->hem[i]->refcount, 1);
 out:
 	mutex_unlock(&table->mutex);
 	return ret;
@@ -753,11 +753,11 @@ static void hns_roce_table_mhop_put(struct hns_roce_dev *hr_dev,
 		return;
 	}
 
-	mutex_lock(&table->mutex);
-	if (check_refcount && (--table->hem[index.buf]->refcount > 0)) {
-		mutex_unlock(&table->mutex);
+	if (!check_refcount)
+		mutex_lock(&table->mutex);
+	else if (!refcount_dec_and_mutex_lock(&table->hem[index.buf]->refcount,
+					      &table->mutex))
 		return;
-	}
 
 	clear_mhop_hem(hr_dev, table, obj, &mhop, &index);
 	free_mhop_hem(hr_dev, table, &mhop, &index);
@@ -779,16 +779,15 @@ void hns_roce_table_put(struct hns_roce_dev *hr_dev,
 	i = (obj & (table->num_obj - 1)) /
 	    (table->table_chunk_size / table->obj_size);
 
-	mutex_lock(&table->mutex);
+	if (!refcount_dec_and_mutex_lock(&table->hem[i]->refcount,
+					 &table->mutex))
+		return;
 
-	if (--table->hem[i]->refcount == 0) {
-		/* Clear HEM base address */
-		if (hr_dev->hw->clear_hem(hr_dev, table, obj, 0))
-			dev_warn(dev, "Clear HEM base address failed.\n");
+	if (hr_dev->hw->clear_hem(hr_dev, table, obj, 0))
+		dev_warn(dev, "failed to clear HEM base address.\n");
 
-		hns_roce_free_hem(hr_dev, table->hem[i]);
-		table->hem[i] = NULL;
-	}
+	hns_roce_free_hem(hr_dev, table->hem[i]);
+	table->hem[i] = NULL;
 
 	mutex_unlock(&table->mutex);
 }
diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.h b/drivers/infiniband/hw/hns/hns_roce_hem.h
index 13fdeb3..ffa65e8 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hem.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hem.h
@@ -88,8 +88,8 @@ struct hns_roce_hem_chunk {
 };
 
 struct hns_roce_hem {
-	struct list_head	 chunk_list;
-	int			 refcount;
+	struct list_head chunk_list;
+	refcount_t refcount;
 };
 
 struct hns_roce_hem_iter {
-- 
2.7.4

[PATCH for-next 4/6] RDMA/cxgb4: Use refcount_t instead of atomic_t for reference counting

[Weihang Li]

The refcount_t API will WARN on underflow and overflow of a reference
counter, and avoid use-after-free risks.

Cc: Potnuri Bharat Teja <bharat@chelsio.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/cxgb4/cq.c       | 6 +++---
 drivers/infiniband/hw/cxgb4/ev.c       | 8 ++++----
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index 44c2416..6c8c910 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -976,8 +976,8 @@ int c4iw_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
 	chp = to_c4iw_cq(ib_cq);
 
 	xa_erase_irq(&chp->rhp->cqs, chp->cq.cqid);
-	atomic_dec(&chp->refcnt);
-	wait_event(chp->wait, !atomic_read(&chp->refcnt));
+	refcount_dec(&chp->refcnt);
+	wait_event(chp->wait, !refcount_read(&chp->refcnt));
 
 	ucontext = rdma_udata_to_drv_context(udata, struct c4iw_ucontext,
 					     ibucontext);
@@ -1080,7 +1080,7 @@ int c4iw_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	chp->ibcq.cqe = entries - 2;
 	spin_lock_init(&chp->lock);
 	spin_lock_init(&chp->comp_handler_lock);
-	atomic_set(&chp->refcnt, 1);
+	refcount_set(&chp->refcnt, 1);
 	init_waitqueue_head(&chp->wait);
 	ret = xa_insert_irq(&rhp->cqs, chp->cq.cqid, chp, GFP_KERNEL);
 	if (ret)
diff --git a/drivers/infiniband/hw/cxgb4/ev.c b/drivers/infiniband/hw/cxgb4/ev.c
index 4cd877b..7798d090 100644
--- a/drivers/infiniband/hw/cxgb4/ev.c
+++ b/drivers/infiniband/hw/cxgb4/ev.c
@@ -151,7 +151,7 @@ void c4iw_ev_dispatch(struct c4iw_dev *dev, struct t4_cqe *err_cqe)
 	}
 
 	c4iw_qp_add_ref(&qhp->ibqp);
-	atomic_inc(&chp->refcnt);
+	refcount_inc(&chp->refcnt);
 	xa_unlock_irq(&dev->qps);
 
 	/* Bad incoming write */
@@ -213,7 +213,7 @@ void c4iw_ev_dispatch(struct c4iw_dev *dev, struct t4_cqe *err_cqe)
 		break;
 	}
 done:
-	if (atomic_dec_and_test(&chp->refcnt))
+	if (refcount_dec_and_test(&chp->refcnt))
 		wake_up(&chp->wait);
 	c4iw_qp_rem_ref(&qhp->ibqp);
 out:
@@ -228,13 +228,13 @@ int c4iw_ev_handler(struct c4iw_dev *dev, u32 qid)
 	xa_lock_irqsave(&dev->cqs, flag);
 	chp = xa_load(&dev->cqs, qid);
 	if (chp) {
-		atomic_inc(&chp->refcnt);
+		refcount_inc(&chp->refcnt);
 		xa_unlock_irqrestore(&dev->cqs, flag);
 		t4_clear_cq_armed(&chp->cq);
 		spin_lock_irqsave(&chp->comp_handler_lock, flag);
 		(*chp->ibcq.comp_handler)(&chp->ibcq, chp->ibcq.cq_context);
 		spin_unlock_irqrestore(&chp->comp_handler_lock, flag);
-		if (atomic_dec_and_test(&chp->refcnt))
+		if (refcount_dec_and_test(&chp->refcnt))
 			wake_up(&chp->wait);
 	} else {
 		pr_debug("unknown cqid 0x%x\n", qid);
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index cdec5de..3883af3 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -427,7 +427,7 @@ struct c4iw_cq {
 	struct t4_cq cq;
 	spinlock_t lock;
 	spinlock_t comp_handler_lock;
-	atomic_t refcnt;
+	refcount_t refcnt;
 	wait_queue_head_t wait;
 	struct c4iw_wr_wait *wr_waitp;
 };
-- 
2.7.4

[PATCH for-next 5/6] RDMA/i40iw: Use refcount_t instead of atomic_t for reference counting

[Weihang Li]

The refcount_t API will WARN on underflow and overflow of a reference
counter, and avoid use-after-free risks.

Cc: Faisal Latif <faisal.latif@intel.com>
Cc: Shiraz Saleem <shiraz.saleem@intel.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/i40iw/i40iw.h       |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_cm.c    | 54 +++++++++++++++----------------
 drivers/infiniband/hw/i40iw/i40iw_cm.h    |  4 +--
 drivers/infiniband/hw/i40iw/i40iw_main.c  |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_puda.h  |  2 +-
 drivers/infiniband/hw/i40iw/i40iw_utils.c | 10 +++---
 6 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw.h b/drivers/infiniband/hw/i40iw/i40iw.h
index be4094a..15c5dd6 100644
--- a/drivers/infiniband/hw/i40iw/i40iw.h
+++ b/drivers/infiniband/hw/i40iw/i40iw.h
@@ -137,7 +137,7 @@ struct i40iw_cqp_request {
 	struct cqp_commands_info info;
 	wait_queue_head_t waitq;
 	struct list_head list;
-	atomic_t refcount;
+	refcount_t refcount;
 	void (*callback_fcn)(struct i40iw_cqp_request*, u32);
 	void *param;
 	struct i40iw_cqp_compl_info compl_info;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index 2450b7d..caab0c1 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -77,7 +77,7 @@ void i40iw_free_sqbuf(struct i40iw_sc_vsi *vsi, void *bufp)
 	struct i40iw_puda_buf *buf = (struct i40iw_puda_buf *)bufp;
 	struct i40iw_puda_rsrc *ilq = vsi->ilq;
 
-	if (!atomic_dec_return(&buf->refcount))
+	if (refcount_dec_and_test(&buf->refcount))
 		i40iw_puda_ret_bufpool(ilq, buf);
 }
 
@@ -344,7 +344,7 @@ static void i40iw_free_retrans_entry(struct i40iw_cm_node *cm_node)
 		cm_node->send_entry = NULL;
 		i40iw_free_sqbuf(&iwdev->vsi, (void *)send_entry->sqbuf);
 		kfree(send_entry);
-		atomic_dec(&cm_node->ref_count);
+		refcount_dec(&cm_node->ref_count);
 	}
 }
 
@@ -531,7 +531,7 @@ static struct i40iw_puda_buf *i40iw_form_cm_frame(struct i40iw_cm_node *cm_node,
 	if (pdata && pdata->addr)
 		memcpy(buf, pdata->addr, pdata->size);
 
-	atomic_set(&sqbuf->refcount, 1);
+	refcount_set(&sqbuf->refcount, 1);
 
 	return sqbuf;
 }
@@ -570,7 +570,7 @@ static void i40iw_active_open_err(struct i40iw_cm_node *cm_node, bool reset)
 			    __func__,
 			    cm_node,
 			    cm_node->state);
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 	}
 
@@ -1092,11 +1092,11 @@ int i40iw_schedule_cm_timer(struct i40iw_cm_node *cm_node,
 	if (type == I40IW_TIMER_TYPE_SEND) {
 		spin_lock_irqsave(&cm_node->retrans_list_lock, flags);
 		cm_node->send_entry = new_send;
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		spin_unlock_irqrestore(&cm_node->retrans_list_lock, flags);
 		new_send->timetosend = jiffies + I40IW_RETRY_TIMEOUT;
 
-		atomic_inc(&sqbuf->refcount);
+		refcount_inc(&sqbuf->refcount);
 		i40iw_puda_send_buf(vsi->ilq, sqbuf);
 		if (!send_retrans) {
 			i40iw_cleanup_retrans_entry(cm_node);
@@ -1140,7 +1140,7 @@ static void i40iw_retrans_expired(struct i40iw_cm_node *cm_node)
 		i40iw_send_reset(cm_node);
 		break;
 	default:
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 		i40iw_create_event(cm_node, I40IW_CM_EVENT_ABORTED);
 		break;
@@ -1198,7 +1198,7 @@ static void i40iw_build_timer_list(struct list_head *timer_list,
 	list_for_each_safe(list_node, list_core_temp, hte) {
 		cm_node = container_of(list_node, struct i40iw_cm_node, list);
 		if (cm_node->close_entry || cm_node->send_entry) {
-			atomic_inc(&cm_node->ref_count);
+			refcount_inc(&cm_node->ref_count);
 			list_add(&cm_node->timer_entry, timer_list);
 		}
 	}
@@ -1286,7 +1286,7 @@ static void i40iw_cm_timer_tick(struct timer_list *t)
 		vsi = &cm_node->iwdev->vsi;
 
 		if (!cm_node->ack_rcvd) {
-			atomic_inc(&send_entry->sqbuf->refcount);
+			refcount_inc(&send_entry->sqbuf->refcount);
 			i40iw_puda_send_buf(vsi->ilq, send_entry->sqbuf);
 			cm_node->cm_core->stats_pkt_retrans++;
 		}
@@ -1448,7 +1448,7 @@ struct i40iw_cm_node *i40iw_find_node(struct i40iw_cm_core *cm_core,
 		    !memcmp(cm_node->rem_addr, rem_addr, sizeof(cm_node->rem_addr)) &&
 		    (cm_node->rem_port == rem_port)) {
 			if (add_refcnt)
-				atomic_inc(&cm_node->ref_count);
+				refcount_inc(&cm_node->ref_count);
 			spin_unlock_irqrestore(&cm_core->ht_lock, flags);
 			return cm_node;
 		}
@@ -1491,7 +1491,7 @@ static struct i40iw_cm_listener *i40iw_find_listener(
 		     !memcmp(listen_addr, ip_zero, sizeof(listen_addr))) &&
 		     (listen_port == dst_port) &&
 		     (listener_state & listen_node->listener_state)) {
-			atomic_inc(&listen_node->ref_count);
+			refcount_inc(&listen_node->ref_count);
 			spin_unlock_irqrestore(&cm_core->listen_list_lock, flags);
 			return listen_node;
 		}
@@ -1864,7 +1864,7 @@ static int i40iw_dec_refcnt_listen(struct i40iw_cm_core *cm_core,
 			cm_node = container_of(list_pos, struct i40iw_cm_node, list);
 			if ((cm_node->listener == listener) &&
 			    !cm_node->accelerated) {
-				atomic_inc(&cm_node->ref_count);
+				refcount_inc(&cm_node->ref_count);
 				list_add(&cm_node->reset_entry, &reset_list);
 			}
 		}
@@ -1901,7 +1901,7 @@ static int i40iw_dec_refcnt_listen(struct i40iw_cm_core *cm_core,
 				event.cm_info.loc_port = loopback->loc_port;
 				event.cm_info.cm_id = loopback->cm_id;
 				event.cm_info.ipv4 = loopback->ipv4;
-				atomic_inc(&loopback->ref_count);
+				refcount_inc(&loopback->ref_count);
 				loopback->state = I40IW_CM_STATE_CLOSED;
 				i40iw_event_connect_error(&event);
 				cm_node->state = I40IW_CM_STATE_LISTENER_DESTROYED;
@@ -1910,7 +1910,7 @@ static int i40iw_dec_refcnt_listen(struct i40iw_cm_core *cm_core,
 		}
 	}
 
-	if (!atomic_dec_return(&listener->ref_count)) {
+	if (refcount_dec_and_test(&listener->ref_count)) {
 		spin_lock_irqsave(&cm_core->listen_list_lock, flags);
 		list_del(&listener->list);
 		spin_unlock_irqrestore(&cm_core->listen_list_lock, flags);
@@ -2206,7 +2206,7 @@ static struct i40iw_cm_node *i40iw_make_cm_node(
 	spin_lock_init(&cm_node->retrans_list_lock);
 	cm_node->ack_rcvd = false;
 
-	atomic_set(&cm_node->ref_count, 1);
+	refcount_set(&cm_node->ref_count, 1);
 	/* associate our parent CM core */
 	cm_node->cm_core = cm_core;
 	cm_node->tcp_cntxt.loc_id = I40IW_CM_DEF_LOCAL_ID;
@@ -2288,7 +2288,7 @@ static void i40iw_rem_ref_cm_node(struct i40iw_cm_node *cm_node)
 	unsigned long flags;
 
 	spin_lock_irqsave(&cm_node->cm_core->ht_lock, flags);
-	if (atomic_dec_return(&cm_node->ref_count)) {
+	if (!refcount_dec_and_test(&cm_node->ref_count)) {
 		spin_unlock_irqrestore(&cm_node->cm_core->ht_lock, flags);
 		return;
 	}
@@ -2366,7 +2366,7 @@ static void i40iw_handle_fin_pkt(struct i40iw_cm_node *cm_node)
 		cm_node->tcp_cntxt.rcv_nxt++;
 		i40iw_cleanup_retrans_entry(cm_node);
 		cm_node->state = I40IW_CM_STATE_CLOSED;
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 		break;
 	case I40IW_CM_STATE_FIN_WAIT1:
@@ -2627,7 +2627,7 @@ static void i40iw_handle_syn_pkt(struct i40iw_cm_node *cm_node,
 		break;
 	case I40IW_CM_STATE_CLOSED:
 		i40iw_cleanup_retrans_entry(cm_node);
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 		break;
 	case I40IW_CM_STATE_OFFLOADED:
@@ -2701,7 +2701,7 @@ static void i40iw_handle_synack_pkt(struct i40iw_cm_node *cm_node,
 	case I40IW_CM_STATE_CLOSED:
 		cm_node->tcp_cntxt.loc_seq_num = ntohl(tcph->ack_seq);
 		i40iw_cleanup_retrans_entry(cm_node);
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 		break;
 	case I40IW_CM_STATE_ESTABLISHED:
@@ -2774,7 +2774,7 @@ static int i40iw_handle_ack_pkt(struct i40iw_cm_node *cm_node,
 		break;
 	case I40IW_CM_STATE_CLOSED:
 		i40iw_cleanup_retrans_entry(cm_node);
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 		i40iw_send_reset(cm_node);
 		break;
 	case I40IW_CM_STATE_LAST_ACK:
@@ -2870,7 +2870,7 @@ static struct i40iw_cm_listener *i40iw_make_listen_node(
 				       I40IW_CM_LISTENER_EITHER_STATE);
 	if (listener &&
 	    (listener->listener_state == I40IW_CM_LISTENER_ACTIVE_STATE)) {
-		atomic_dec(&listener->ref_count);
+		refcount_dec(&listener->ref_count);
 		i40iw_debug(cm_core->dev,
 			    I40IW_DEBUG_CM,
 			    "Not creating listener since it already exists\n");
@@ -2888,7 +2888,7 @@ static struct i40iw_cm_listener *i40iw_make_listen_node(
 
 		INIT_LIST_HEAD(&listener->child_listen_list);
 
-		atomic_set(&listener->ref_count, 1);
+		refcount_set(&listener->ref_count, 1);
 	} else {
 		listener->reused_node = 1;
 	}
@@ -3213,7 +3213,7 @@ void i40iw_receive_ilq(struct i40iw_sc_vsi *vsi, struct i40iw_puda_buf *rbuf)
 				    I40IW_DEBUG_CM,
 				    "%s allocate node failed\n",
 				    __func__);
-			atomic_dec(&listener->ref_count);
+			refcount_dec(&listener->ref_count);
 			return;
 		}
 		if (!tcph->rst && !tcph->fin) {
@@ -3222,7 +3222,7 @@ void i40iw_receive_ilq(struct i40iw_sc_vsi *vsi, struct i40iw_puda_buf *rbuf)
 			i40iw_rem_ref_cm_node(cm_node);
 			return;
 		}
-		atomic_inc(&cm_node->ref_count);
+		refcount_inc(&cm_node->ref_count);
 	} else if (cm_node->state == I40IW_CM_STATE_OFFLOADED) {
 		i40iw_rem_ref_cm_node(cm_node);
 		return;
@@ -4228,7 +4228,7 @@ static void i40iw_cm_event_handler(struct work_struct *work)
  */
 static void i40iw_cm_post_event(struct i40iw_cm_event *event)
 {
-	atomic_inc(&event->cm_node->ref_count);
+	refcount_inc(&event->cm_node->ref_count);
 	event->cm_info.cm_id->add_ref(event->cm_info.cm_id);
 	INIT_WORK(&event->event_work, i40iw_cm_event_handler);
 
@@ -4331,7 +4331,7 @@ void i40iw_cm_teardown_connections(struct i40iw_device *iwdev, u32 *ipaddr,
 		    (nfo->vlan_id == cm_node->vlan_id &&
 		    (!memcmp(cm_node->loc_addr, ipaddr, nfo->ipv4 ? 4 : 16) ||
 		     !memcmp(cm_node->rem_addr, ipaddr, nfo->ipv4 ? 4 : 16)))) {
-			atomic_inc(&cm_node->ref_count);
+			refcount_inc(&cm_node->ref_count);
 			list_add(&cm_node->teardown_entry, &teardown_list);
 		}
 	}
@@ -4342,7 +4342,7 @@ void i40iw_cm_teardown_connections(struct i40iw_device *iwdev, u32 *ipaddr,
 		    (nfo->vlan_id == cm_node->vlan_id &&
 		    (!memcmp(cm_node->loc_addr, ipaddr, nfo->ipv4 ? 4 : 16) ||
 		     !memcmp(cm_node->rem_addr, ipaddr, nfo->ipv4 ? 4 : 16)))) {
-			atomic_inc(&cm_node->ref_count);
+			refcount_inc(&cm_node->ref_count);
 			list_add(&cm_node->teardown_entry, &teardown_list);
 		}
 	}
diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.h b/drivers/infiniband/hw/i40iw/i40iw_cm.h
index 6e43e4d..3cfe3d2 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.h
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.h
@@ -291,7 +291,7 @@ struct i40iw_cm_listener {
 	u32 loc_addr[4];
 	u16 loc_port;
 	struct iw_cm_id *cm_id;
-	atomic_t ref_count;
+	refcount_t ref_count;
 	struct i40iw_device *iwdev;
 	atomic_t pend_accepts_cnt;
 	int backlog;
@@ -319,7 +319,7 @@ struct i40iw_cm_node {
 	enum i40iw_cm_node_state state;
 	u8 loc_mac[ETH_ALEN];
 	u8 rem_mac[ETH_ALEN];
-	atomic_t ref_count;
+	refcount_t ref_count;
 	struct i40iw_qp *iwqp;
 	struct i40iw_device *iwdev;
 	struct i40iw_sc_dev *dev;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_main.c b/drivers/infiniband/hw/i40iw/i40iw_main.c
index b496f30..fc48555 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_main.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_main.c
@@ -1125,7 +1125,7 @@ static enum i40iw_status_code i40iw_alloc_local_mac_ipaddr_entry(struct i40iw_de
 	}
 
 	/* increment refcount, because we need the cqp request ret value */
-	atomic_inc(&cqp_request->refcount);
+	refcount_inc(&cqp_request->refcount);
 
 	cqp_info = &cqp_request->info;
 	cqp_info->cqp_cmd = OP_ALLOC_LOCAL_MAC_IPADDR_ENTRY;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_puda.h b/drivers/infiniband/hw/i40iw/i40iw_puda.h
index 53a7d58..5996626 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_puda.h
+++ b/drivers/infiniband/hw/i40iw/i40iw_puda.h
@@ -90,7 +90,7 @@ struct i40iw_puda_buf {
 	u8 tcphlen;		/* tcp length in bytes */
 	u8 maclen;		/* mac length in bytes */
 	u32 totallen;		/* machlen+iphlen+tcphlen+datalen */
-	atomic_t refcount;
+	refcount_t refcount;
 	u8 hdrlen;
 	bool ipv4;
 	u32 seqnum;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
index 9ff825f..32ff432b 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
@@ -384,10 +384,10 @@ struct i40iw_cqp_request *i40iw_get_cqp_request(struct i40iw_cqp *cqp, bool wait
 	}
 
 	if (wait) {
-		atomic_set(&cqp_request->refcount, 2);
+		refcount_set(&cqp_request->refcount, 2);
 		cqp_request->waiting = true;
 	} else {
-		atomic_set(&cqp_request->refcount, 1);
+		refcount_set(&cqp_request->refcount, 1);
 	}
 	return cqp_request;
 }
@@ -424,7 +424,7 @@ void i40iw_free_cqp_request(struct i40iw_cqp *cqp, struct i40iw_cqp_request *cqp
 void i40iw_put_cqp_request(struct i40iw_cqp *cqp,
 			   struct i40iw_cqp_request *cqp_request)
 {
-	if (atomic_dec_and_test(&cqp_request->refcount))
+	if (refcount_dec_and_test(&cqp_request->refcount))
 		i40iw_free_cqp_request(cqp, cqp_request);
 }
 
@@ -445,7 +445,7 @@ static void i40iw_free_pending_cqp_request(struct i40iw_cqp *cqp,
 	}
 	i40iw_put_cqp_request(cqp, cqp_request);
 	wait_event_timeout(iwdev->close_wq,
-			   !atomic_read(&cqp_request->refcount),
+			   !refcount_read(&cqp_request->refcount),
 			   1000);
 }
 
@@ -1005,7 +1005,7 @@ static void i40iw_cqp_manage_hmc_fcn_callback(struct i40iw_cqp_request *cqp_requ
 
 	if (hmcfcninfo && hmcfcninfo->callback_fcn) {
 		i40iw_debug(&iwdev->sc_dev, I40IW_DEBUG_HMC, "%s1\n", __func__);
-		atomic_inc(&cqp_request->refcount);
+		refcount_inc(&cqp_request->refcount);
 		work = &iwdev->virtchnl_w[hmcfcninfo->iw_vf_idx];
 		work->cqp_request = cqp_request;
 		INIT_WORK(&work->work, i40iw_cqp_manage_hmc_fcn_worker);
-- 
2.7.4

[PATCH for-next 6/6] RDMA/ipoib: Use refcount_t instead of atomic_t for reference counting

[Weihang Li]

The refcount_t API will WARN on underflow and overflow of a reference
counter, and avoid use-after-free risks.

Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/ulp/ipoib/ipoib.h      | 4 ++--
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 75cd447..44d8d15 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -454,7 +454,7 @@ struct ipoib_neigh {
 	struct list_head    list;
 	struct ipoib_neigh __rcu *hnext;
 	struct rcu_head     rcu;
-	atomic_t	    refcnt;
+	refcount_t	    refcnt;
 	unsigned long       alive;
 };
 
@@ -464,7 +464,7 @@ struct ipoib_neigh {
 void ipoib_neigh_dtor(struct ipoib_neigh *neigh);
 static inline void ipoib_neigh_put(struct ipoib_neigh *neigh)
 {
-	if (atomic_dec_and_test(&neigh->refcnt))
+	if (refcount_dec_and_test(&neigh->refcnt))
 		ipoib_neigh_dtor(neigh);
 }
 struct ipoib_neigh *ipoib_neigh_get(struct net_device *dev, u8 *daddr);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index bbb1808..ac39610 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1287,7 +1287,7 @@ struct ipoib_neigh *ipoib_neigh_get(struct net_device *dev, u8 *daddr)
 	     neigh = rcu_dereference_bh(neigh->hnext)) {
 		if (memcmp(daddr, neigh->daddr, INFINIBAND_ALEN) == 0) {
 			/* found, take one ref on behalf of the caller */
-			if (!atomic_inc_not_zero(&neigh->refcnt)) {
+			if (!refcount_inc_not_zero(&neigh->refcnt)) {
 				/* deleted */
 				neigh = NULL;
 				goto out_unlock;
@@ -1382,7 +1382,7 @@ static struct ipoib_neigh *ipoib_neigh_ctor(u8 *daddr,
 	INIT_LIST_HEAD(&neigh->list);
 	ipoib_cm_set(neigh, NULL);
 	/* one ref on behalf of the caller */
-	atomic_set(&neigh->refcnt, 1);
+	refcount_set(&neigh->refcnt, 1);
 
 	return neigh;
 }
@@ -1414,7 +1414,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(u8 *daddr,
 					       lockdep_is_held(&priv->lock))) {
 		if (memcmp(daddr, neigh->daddr, INFINIBAND_ALEN) == 0) {
 			/* found, take one ref on behalf of the caller */
-			if (!atomic_inc_not_zero(&neigh->refcnt)) {
+			if (!refcount_inc_not_zero(&neigh->refcnt)) {
 				/* deleted */
 				neigh = NULL;
 				break;
@@ -1429,7 +1429,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(u8 *daddr,
 		goto out_unlock;
 
 	/* one ref on behalf of the hash table */
-	atomic_inc(&neigh->refcnt);
+	refcount_inc(&neigh->refcnt);
 	neigh->alive = jiffies;
 	/* put in hash */
 	rcu_assign_pointer(neigh->hnext,
-- 
2.7.4

Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[Jason Gunthorpe]

On Fri, May 14, 2021 at 10:11:34AM +0800, Weihang Li wrote:
> The refcount_t API will WARN on underflow and overflow of a reference
> counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
> regarded as there is a risk about use-after-free. So it should be set to 1
> directly during initialization.

What does this comment about 0 to 1 mean?

This all seems like a good idea but I wish you had done one patch per
variable changed

Jason

Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[liweihang]

On 2021/5/14 20:34, Jason Gunthorpe wrote:
> On Fri, May 14, 2021 at 10:11:34AM +0800, Weihang Li wrote:
>> The refcount_t API will WARN on underflow and overflow of a reference
>> counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
>> regarded as there is a risk about use-after-free. So it should be set to 1
>> directly during initialization.
> 
> What does this comment about 0 to 1 mean?
> 

Hi Jason,

I first thought refcount_inc() and atomic_inc() are exactly the same, but I got
a warning about refcount_t on iwpm_init() after the replacement:

[   16.882939] refcount_t: addition on 0; use-after-free.
[   16.888065] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25
refcount_warn_saturate+0xa0/0x144
...
[   17.014698] Call trace:
[   17.017135]  refcount_warn_saturate+0xa0/0x144
[   17.021559]  iwpm_init+0x104/0x12c
[   17.024948]  iw_cm_init+0x24/0xd0
[   17.028248]  do_one_initcall+0x54/0x2d0
[   17.032068]  kernel_init_freeable+0x224/0x294
[   17.036407]  kernel_init+0x20/0x12c
[   17.039880]  ret_from_fork+0x10/0x18

Then I noticed that the comment of refcount_inc() says:

 * Will WARN if the refcount is 0, as this represents a possible use-after-free
 * condition.

so I made changes:

@@ -77,8 +77,12 @@ int iwpm_init(u8 nl_client)
                        ret = -ENOMEM;
                        goto init_exit;
                }
+
+               refcount_set(&iwpm_admin.refcount, 1);
+       } else {
+               refcount_inc(&iwpm_admin.refcount);
        }
-       refcount_inc(&iwpm_admin.refcount);
+

I wrote the comments because I thought someone might be confused by the above
changes :)

> This all seems like a good idea but I wish you had done one patch per
> variable changed
> 
> Jason
> 

Sure, thanks.

Weihang

Re: [PATCH for-next 0/6] RDMA: Use refcount_t for reference counting

[Leon Romanovsky]

On Fri, May 14, 2021 at 10:11:33AM +0800, Weihang Li wrote:
> There are some refcnt in type of atomic_t in RDMA subsystem, almost all of
> them is wrote before the refcount_t is acheived in kernel. refcount_t is
> better than integer for reference counting, it will WARN on
> overflow/underflow and avoid use-after-free risks.
> 
> Weihang Li (6):
>   RDMA/core: Use refcount_t instead of atomic_t for reference counting
>   RDMA/hns: Use refcount_t instead of atomic_t for reference counting
>   RDMA/hns: Use refcount_t APIs for HEM
>   RDMA/cxgb4: Use refcount_t instead of atomic_t for reference counting
>   RDMA/i40iw: Use refcount_t instead of atomic_t for reference counting
>   RDMA/ipoib: Use refcount_t instead of atomic_t for reference counting

This series causes to the following splat when restarting mlx4 or mlx5 drivers.

[  227.529930] ------------[ cut here ]------------
[  227.530966] refcount_t: addition on 0; use-after-free.
[  227.531890] WARNING: CPU: 1 PID: 579 at lib/refcount.c:25 refcount_warn_saturate+0xb4/0x130
[  227.533427] Modules linked in: bonding nf_tables ipip tunnel4 geneve ip6_gre ip6_tunnel tunnel6 ip_gre gre mlx5_ib mlx5_core ptp pps_core rdma_ucm ib_uverbs ib_ipoib ib_umad openvswitch nsh xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 br_netfilter rpcrdma ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_cm ib_core overlay fuse [last unloaded: ib_uverbs]
[  227.539872] CPU: 1 PID: 579 Comm: kworker/u20:7 Not tainted 5.13.0-rc1_2021_05_14_02_56_31_ #1
[  227.541458] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
[  227.543424] Workqueue: ipoib_wq ipoib_mcast_join_task [ib_ipoib]
[  227.544497] RIP: 0010:refcount_warn_saturate+0xb4/0x130
[  227.545515] Code: 5b c3 0f b6 1d 69 19 40 01 80 fb 01 0f 87 60 fd 52 00 83 e3 01 75 91 48 c7 c7 88 9a 57 82 c6 05 4d 19 40 01 01 e8 35 50 4e 00 <0f> 0b 5b c3 0f b6 1d 3f 19 40 01 80 fb 01 0f 87 70 fd 52 00 83 e3
[  227.548723] RSP: 0018:ffff888133d23cb0 EFLAGS: 00010082
[  227.549739] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff8882f58a78b8
[  227.551077] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff8882f58a78b0
[  227.552306] RBP: ffff888133d23d30 R08: ffffffff83b5fd40 R09: 0000000000000001
[  227.553589] R10: 0000000000000001 R11: 746e756f63666572 R12: ffff88813e00c700
[  227.554885] R13: ffff8881182a3720 R14: ffff88810d06f938 R15: ffff8881182a3600
[  227.556104] FS:  0000000000000000(0000) GS:ffff8882f5880000(0000) knlGS:0000000000000000
[  227.557594] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  227.558719] CR2: 000055fb4a2ee768 CR3: 000000010303e006 CR4: 0000000000370ea0
[  227.559940] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  227.561223] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  227.562566] Call Trace:
[  227.563088]  ib_sa_join_multicast+0x4ec/0x580 [ib_core]
[  227.564059]  ipoib_mcast_join+0x16d/0x220 [ib_ipoib]
[  227.564961]  ? ipoib_mcast_join+0x220/0x220 [ib_ipoib]
[  227.565991]  ipoib_mcast_join_task+0x16e/0x320 [ib_ipoib]
[  227.567014]  ? process_one_work+0x1d7/0x5b0
[  227.567809]  process_one_work+0x25a/0x5b0
[  227.568587]  worker_thread+0x4f/0x3e0
[  227.569349]  ? process_one_work+0x5b0/0x5b0
[  227.570210]  kthread+0x129/0x140
[  227.570874]  ? __kthread_bind_mask+0x60/0x60
[  227.571677]  ret_from_fork+0x1f/0x30
[  227.572416] irq event stamp: 150582
[  227.573190] hardirqs last  enabled at (150581): [<ffffffff81c4b337>] _raw_spin_unlock_irqrestore+0x47/0x50
[  227.575046] hardirqs last disabled at (150582): [<ffffffff81c4b130>] _raw_spin_lock_irqsave+0x50/0x60
[  227.576793] softirqs last  enabled at (150576): [<ffffffffa00bcbef>] ipoib_mcast_join_task+0xef/0x320 [ib_ipoib]
[  227.578657] softirqs last disabled at (150574): [<ffffffffa00bcb9f>] ipoib_mcast_join_task+0x9f/0x320 [ib_ipoib]
[  227.580416] ---[ end trace 52a055538498ac03 ]---

Thanks

Re: [PATCH for-next 0/6] RDMA: Use refcount_t for reference counting

[liweihang]

On 2021/5/16 18:18, Leon Romanovsky wrote:
> On Fri, May 14, 2021 at 10:11:33AM +0800, Weihang Li wrote:
>> There are some refcnt in type of atomic_t in RDMA subsystem, almost all of
>> them is wrote before the refcount_t is acheived in kernel. refcount_t is
>> better than integer for reference counting, it will WARN on
>> overflow/underflow and avoid use-after-free risks.
>>
>> Weihang Li (6):
>>   RDMA/core: Use refcount_t instead of atomic_t for reference counting
>>   RDMA/hns: Use refcount_t instead of atomic_t for reference counting
>>   RDMA/hns: Use refcount_t APIs for HEM
>>   RDMA/cxgb4: Use refcount_t instead of atomic_t for reference counting
>>   RDMA/i40iw: Use refcount_t instead of atomic_t for reference counting
>>   RDMA/ipoib: Use refcount_t instead of atomic_t for reference counting
> 
> This series causes to the following splat when restarting mlx4 or mlx5 drivers.
> 
> [  227.529930] ------------[ cut here ]------------
> [  227.530966] refcount_t: addition on 0; use-after-free.
> [  227.531890] WARNING: CPU: 1 PID: 579 at lib/refcount.c:25 refcount_warn_saturate+0xb4/0x130
> [  227.533427] Modules linked in: bonding nf_tables ipip tunnel4 geneve ip6_gre ip6_tunnel tunnel6 ip_gre gre mlx5_ib mlx5_core ptp pps_core rdma_ucm ib_uverbs ib_ipoib ib_umad openvswitch nsh xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 br_netfilter rpcrdma ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_cm ib_core overlay fuse [last unloaded: ib_uverbs]
> [  227.539872] CPU: 1 PID: 579 Comm: kworker/u20:7 Not tainted 5.13.0-rc1_2021_05_14_02_56_31_ #1
> [  227.541458] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> [  227.543424] Workqueue: ipoib_wq ipoib_mcast_join_task [ib_ipoib]
> [  227.544497] RIP: 0010:refcount_warn_saturate+0xb4/0x130
> [  227.545515] Code: 5b c3 0f b6 1d 69 19 40 01 80 fb 01 0f 87 60 fd 52 00 83 e3 01 75 91 48 c7 c7 88 9a 57 82 c6 05 4d 19 40 01 01 e8 35 50 4e 00 <0f> 0b 5b c3 0f b6 1d 3f 19 40 01 80 fb 01 0f 87 70 fd 52 00 83 e3
> [  227.548723] RSP: 0018:ffff888133d23cb0 EFLAGS: 00010082
> [  227.549739] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff8882f58a78b8
> [  227.551077] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff8882f58a78b0
> [  227.552306] RBP: ffff888133d23d30 R08: ffffffff83b5fd40 R09: 0000000000000001
> [  227.553589] R10: 0000000000000001 R11: 746e756f63666572 R12: ffff88813e00c700
> [  227.554885] R13: ffff8881182a3720 R14: ffff88810d06f938 R15: ffff8881182a3600
> [  227.556104] FS:  0000000000000000(0000) GS:ffff8882f5880000(0000) knlGS:0000000000000000
> [  227.557594] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  227.558719] CR2: 000055fb4a2ee768 CR3: 000000010303e006 CR4: 0000000000370ea0
> [  227.559940] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  227.561223] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  227.562566] Call Trace:
> [  227.563088]  ib_sa_join_multicast+0x4ec/0x580 [ib_core]
> [  227.564059]  ipoib_mcast_join+0x16d/0x220 [ib_ipoib]
> [  227.564961]  ? ipoib_mcast_join+0x220/0x220 [ib_ipoib]
> [  227.565991]  ipoib_mcast_join_task+0x16e/0x320 [ib_ipoib]
> [  227.567014]  ? process_one_work+0x1d7/0x5b0
> [  227.567809]  process_one_work+0x25a/0x5b0
> [  227.568587]  worker_thread+0x4f/0x3e0
> [  227.569349]  ? process_one_work+0x5b0/0x5b0
> [  227.570210]  kthread+0x129/0x140
> [  227.570874]  ? __kthread_bind_mask+0x60/0x60
> [  227.571677]  ret_from_fork+0x1f/0x30
> [  227.572416] irq event stamp: 150582
> [  227.573190] hardirqs last  enabled at (150581): [<ffffffff81c4b337>] _raw_spin_unlock_irqrestore+0x47/0x50
> [  227.575046] hardirqs last disabled at (150582): [<ffffffff81c4b130>] _raw_spin_lock_irqsave+0x50/0x60
> [  227.576793] softirqs last  enabled at (150576): [<ffffffffa00bcbef>] ipoib_mcast_join_task+0xef/0x320 [ib_ipoib]
> [  227.578657] softirqs last disabled at (150574): [<ffffffffa00bcb9f>] ipoib_mcast_join_task+0x9f/0x320 [ib_ipoib]
> [  227.580416] ---[ end trace 52a055538498ac03 ]---
> 
> Thanks
> 

Hi Leon,

Thanks for the test, it seems that the problem is caused by the refcount of
mcast_group. I will reply to this email once I have made progress.

Weihang

Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[Jason Gunthorpe]

On Sat, May 15, 2021 at 03:07:58AM +0000, liweihang wrote:
> On 2021/5/14 20:34, Jason Gunthorpe wrote:
> > On Fri, May 14, 2021 at 10:11:34AM +0800, Weihang Li wrote:
> >> The refcount_t API will WARN on underflow and overflow of a reference
> >> counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
> >> regarded as there is a risk about use-after-free. So it should be set to 1
> >> directly during initialization.
> > 
> > What does this comment about 0 to 1 mean?
> > 
> 
> Hi Jason,
> 
> I first thought refcount_inc() and atomic_inc() are exactly the same, but I got
> a warning about refcount_t on iwpm_init() after the replacement:
> 
> [   16.882939] refcount_t: addition on 0; use-after-free.
> [   16.888065] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25
> refcount_warn_saturate+0xa0/0x144
> ...
> [   17.014698] Call trace:
> [   17.017135]  refcount_warn_saturate+0xa0/0x144
> [   17.021559]  iwpm_init+0x104/0x12c
> [   17.024948]  iw_cm_init+0x24/0xd0
> [   17.028248]  do_one_initcall+0x54/0x2d0
> [   17.032068]  kernel_init_freeable+0x224/0x294
> [   17.036407]  kernel_init+0x20/0x12c
> [   17.039880]  ret_from_fork+0x10/0x18
> 
> Then I noticed that the comment of refcount_inc() says:
> 
>  * Will WARN if the refcount is 0, as this represents a possible use-after-free
>  * condition.
> 
> so I made changes:
> 
> @@ -77,8 +77,12 @@ int iwpm_init(u8 nl_client)
>                         ret = -ENOMEM;
>                         goto init_exit;
>                 }
> +
> +               refcount_set(&iwpm_admin.refcount, 1);
> +       } else {
> +               refcount_inc(&iwpm_admin.refcount);
>         }
> -       refcount_inc(&iwpm_admin.refcount);
> +
> 
> I wrote the comments because I thought someone might be confused by the above
> changes :)

Stuff like this needs to be split into a single patch for iwpm_admin

Jason

RE: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[Saleem, Shiraz]

> Subject: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for
> reference counting
> 
> The refcount_t API will WARN on underflow and overflow of a reference counter,
> and avoid use-after-free risks. Increase refcount_t from 0 to 1 is regarded as there
> is a risk about use-after-free. So it should be set to 1 directly during initialization.
> 
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/core/iwcm.c        |  9 ++++-----
>  drivers/infiniband/core/iwcm.h        |  2 +-
>  drivers/infiniband/core/iwpm_util.c   | 12 ++++++++----
>  drivers/infiniband/core/iwpm_util.h   |  2 +-
>  drivers/infiniband/core/mad_priv.h    |  2 +-
>  drivers/infiniband/core/multicast.c   | 30 +++++++++++++++---------------
>  drivers/infiniband/core/uverbs.h      |  2 +-
>  drivers/infiniband/core/uverbs_main.c | 12 ++++++------
>  8 files changed, 37 insertions(+), 34 deletions(-)
> 

[...]

> @@ -589,9 +589,9 @@ static struct mcast_group *acquire_group(struct
> mcast_port *port,
>  		kfree(group);
>  		group = cur_group;
>  	} else
> -		atomic_inc(&port->refcount);
> +		refcount_inc(&port->refcount);
>  found:
> -	atomic_inc(&group->refcount);
> +	refcount_inc(&group->refcount);

Seems like there is refcount_inc with refcount = 0 when the group is first created?

>  	spin_unlock_irqrestore(&port->lock, flags);
>  	return group;
>  }

Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[liweihang]

On 2021/5/18 0:56, Jason Gunthorpe wrote:
> On Sat, May 15, 2021 at 03:07:58AM +0000, liweihang wrote:
>> On 2021/5/14 20:34, Jason Gunthorpe wrote:
>>> On Fri, May 14, 2021 at 10:11:34AM +0800, Weihang Li wrote:
>>>> The refcount_t API will WARN on underflow and overflow of a reference
>>>> counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
>>>> regarded as there is a risk about use-after-free. So it should be set to 1
>>>> directly during initialization.
>>>
>>> What does this comment about 0 to 1 mean?
>>>
>>
>> Hi Jason,
>>
>> I first thought refcount_inc() and atomic_inc() are exactly the same, but I got
>> a warning about refcount_t on iwpm_init() after the replacement:
>>
>> [   16.882939] refcount_t: addition on 0; use-after-free.
>> [   16.888065] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25
>> refcount_warn_saturate+0xa0/0x144
>> ...
>> [   17.014698] Call trace:
>> [   17.017135]  refcount_warn_saturate+0xa0/0x144
>> [   17.021559]  iwpm_init+0x104/0x12c
>> [   17.024948]  iw_cm_init+0x24/0xd0
>> [   17.028248]  do_one_initcall+0x54/0x2d0
>> [   17.032068]  kernel_init_freeable+0x224/0x294
>> [   17.036407]  kernel_init+0x20/0x12c
>> [   17.039880]  ret_from_fork+0x10/0x18
>>
>> Then I noticed that the comment of refcount_inc() says:
>>
>>  * Will WARN if the refcount is 0, as this represents a possible use-after-free
>>  * condition.
>>
>> so I made changes:
>>
>> @@ -77,8 +77,12 @@ int iwpm_init(u8 nl_client)
>>                         ret = -ENOMEM;
>>                         goto init_exit;
>>                 }
>> +
>> +               refcount_set(&iwpm_admin.refcount, 1);
>> +       } else {
>> +               refcount_inc(&iwpm_admin.refcount);
>>         }
>> -       refcount_inc(&iwpm_admin.refcount);
>> +
>>
>> I wrote the comments because I thought someone might be confused by the above
>> changes :)
> 
> Stuff like this needs to be split into a single patch for iwpm_admin
> 
> Jason
> 

Sure, I will split all of the changes into separate patches.

Weihang

Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[liweihang]

On 2021/5/18 7:03, Saleem, Shiraz wrote:
>> Subject: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for
>> reference counting
>>
>> The refcount_t API will WARN on underflow and overflow of a reference counter,
>> and avoid use-after-free risks. Increase refcount_t from 0 to 1 is regarded as there
>> is a risk about use-after-free. So it should be set to 1 directly during initialization.
>>
>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>> ---
>>  drivers/infiniband/core/iwcm.c        |  9 ++++-----
>>  drivers/infiniband/core/iwcm.h        |  2 +-
>>  drivers/infiniband/core/iwpm_util.c   | 12 ++++++++----
>>  drivers/infiniband/core/iwpm_util.h   |  2 +-
>>  drivers/infiniband/core/mad_priv.h    |  2 +-
>>  drivers/infiniband/core/multicast.c   | 30 +++++++++++++++---------------
>>  drivers/infiniband/core/uverbs.h      |  2 +-
>>  drivers/infiniband/core/uverbs_main.c | 12 ++++++------
>>  8 files changed, 37 insertions(+), 34 deletions(-)
>>
> 
> [...]
> 
>> @@ -589,9 +589,9 @@ static struct mcast_group *acquire_group(struct
>> mcast_port *port,
>>  		kfree(group);
>>  		group = cur_group;
>>  	} else
>> -		atomic_inc(&port->refcount);
>> +		refcount_inc(&port->refcount);
>>  found:
>> -	atomic_inc(&group->refcount);
>> +	refcount_inc(&group->refcount);
> 
> Seems like there is refcount_inc with refcount = 0 when the group is first created?

Yes, one of "refcount_inc(&group->refcount)" led to the issue that Leon had
reported. I will fix it, thank you.

Weihang

> 
>>  	spin_unlock_irqrestore(&port->lock, flags);
>>  	return group;
>>  }
> 
>