Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Yishai Hadas <yishaih@mellanox.com>
To: linux-rdma@vger.kernel.org, jgg@mellanox.com, dledford@redhat.com
Cc: yishaih@mellanox.com, maorg@mellanox.com, michaelgur@mellanox.com
Subject: [PATCH rdma-next 14/14] RDMA/core: Use READ_ONCE for ib_ufile.async_file
Date: Wed,  8 Jan 2020 19:22:06 +0200
Message-ID: <1578504126-9400-15-git-send-email-yishaih@mellanox.com> (raw)
In-Reply-To: <1578504126-9400-1-git-send-email-yishaih@mellanox.com>

From: Jason Gunthorpe <jgg@mellanox.com>

The writer for async_file holds the ucontext_lock, while the readers are
left unlocked. Most readers rely on an implicit locking, either by having
a uobject (which cannot be created before a context) or by holding the
ib_ufile kref.

However ib_uverbs_free_hw_resources() has no implicit lock and has a
possible race. Make this all clear and sane by using READ_ONCE
consistently.

Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/infiniband/core/uverbs.h              |  6 ++----
 drivers/infiniband/core/uverbs_cmd.c          |  2 +-
 drivers/infiniband/core/uverbs_main.c         | 29 +++++++++++----------------
 drivers/infiniband/core/uverbs_std_types.c    |  6 +++---
 drivers/infiniband/core/uverbs_std_types_cq.c |  1 -
 5 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index ccde5d2..aaa5c75 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -220,11 +220,9 @@ struct ib_ucq_object {
 void ib_uverbs_free_event_queue(struct ib_uverbs_event_queue *event_queue);
 void ib_uverbs_flow_resources_free(struct ib_uflow_resources *uflow_res);
 
-void ib_uverbs_release_ucq(struct ib_uverbs_file *file,
-			   struct ib_uverbs_completion_event_file *ev_file,
+void ib_uverbs_release_ucq(struct ib_uverbs_completion_event_file *ev_file,
 			   struct ib_ucq_object *uobj);
-void ib_uverbs_release_uevent(struct ib_uverbs_file *file,
-			      struct ib_uevent_object *uobj);
+void ib_uverbs_release_uevent(struct ib_uevent_object *uobj);
 void ib_uverbs_release_file(struct kref *ref);
 
 void ib_uverbs_comp_handler(struct ib_cq *cq, void *cq_context);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index ced1384..29b1b5a 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1056,7 +1056,7 @@ static struct ib_ucq_object *create_cq(struct uverbs_attr_bundle *attrs,
 	kfree(cq);
 err_file:
 	if (ev_file)
-		ib_uverbs_release_ucq(attrs->ufile, ev_file, obj);
+		ib_uverbs_release_ucq(ev_file, obj);
 
 err:
 	uobj_alloc_abort(&obj->uevent.uobject, attrs);
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 121e65f..1f279b0a 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -125,9 +125,8 @@ static void ib_uverbs_release_dev(struct device *device)
 	kfree(dev);
 }
 
-void ib_uverbs_release_ucq(struct ib_uverbs_file *file,
-			  struct ib_uverbs_completion_event_file *ev_file,
-			  struct ib_ucq_object *uobj)
+void ib_uverbs_release_ucq(struct ib_uverbs_completion_event_file *ev_file,
+			   struct ib_ucq_object *uobj)
 {
 	struct ib_uverbs_event *evt, *tmp;
 
@@ -142,20 +141,21 @@ void ib_uverbs_release_ucq(struct ib_uverbs_file *file,
 		uverbs_uobject_put(&ev_file->uobj);
 	}
 
-	ib_uverbs_release_uevent(file, &uobj->uevent);
+	ib_uverbs_release_uevent(&uobj->uevent);
 }
 
-void ib_uverbs_release_uevent(struct ib_uverbs_file *file,
-			      struct ib_uevent_object *uobj)
+void ib_uverbs_release_uevent(struct ib_uevent_object *uobj)
 {
+	struct ib_uverbs_async_event_file *async_file =
+		READ_ONCE(uobj->uobject.ufile->async_file);
 	struct ib_uverbs_event *evt, *tmp;
 
-	spin_lock_irq(&file->async_file->ev_queue.lock);
+	spin_lock_irq(&async_file->ev_queue.lock);
 	list_for_each_entry_safe(evt, tmp, &uobj->event_list, obj_list) {
 		list_del(&evt->list);
 		kfree(evt);
 	}
-	spin_unlock_irq(&file->async_file->ev_queue.lock);
+	spin_unlock_irq(&async_file->ev_queue.lock);
 }
 
 void ib_uverbs_detach_umcast(struct ib_qp *qp,
@@ -420,7 +420,7 @@ void ib_uverbs_comp_handler(struct ib_cq *cq, void *cq_context)
 static void uverbs_uobj_event(struct ib_uevent_object *eobj,
 			      struct ib_event *event)
 {
-	ib_uverbs_async_handler(eobj->uobject.ufile->async_file,
+	ib_uverbs_async_handler(READ_ONCE(eobj->uobject.ufile->async_file),
 				eobj->uobject.user_handle, event->event,
 				&eobj->event_list, &eobj->events_reported);
 }
@@ -476,9 +476,9 @@ void ib_uverbs_init_async_event_file(
 	ib_uverbs_init_event_queue(&async_file->ev_queue);
 
 	if (!WARN_ON(uverbs_file->async_file)) {
-		uverbs_file->async_file = async_file;
 		/* Pairs with the put in ib_uverbs_release_file */
 		uverbs_uobject_get(&async_file->uobj);
+		smp_store_release(&uverbs_file->async_file, async_file);
 	}
 
 	INIT_IB_EVENT_HANDLER(&async_file->event_handler, ib_dev,
@@ -1156,13 +1156,9 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
 					struct ib_device *ib_dev)
 {
 	struct ib_uverbs_file *file;
-	struct ib_event event;
 
 	/* Pending running commands to terminate */
 	uverbs_disassociate_api_pre(uverbs_dev);
-	event.event = IB_EVENT_DEVICE_FATAL;
-	event.element.port_num = 0;
-	event.device = ib_dev;
 
 	mutex_lock(&uverbs_dev->lists_mutex);
 	while (!list_empty(&uverbs_dev->uverbs_file_list)) {
@@ -1178,9 +1174,8 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
 		 */
 		mutex_unlock(&uverbs_dev->lists_mutex);
 
-		if (file->async_file)
-			ib_uverbs_event_handler(
-				&file->async_file->event_handler, &event);
+		ib_uverbs_async_handler(READ_ONCE(file->async_file), 0,
+					IB_EVENT_DEVICE_FATAL, NULL, NULL);
 
 		uverbs_destroy_ufile_hw(file, RDMA_REMOVE_DRIVER_REMOVE);
 		kref_put(&file->ref, ib_uverbs_release_file);
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index efe70bc..994d874 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -105,7 +105,7 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
 	if (uqp->uxrcd)
 		atomic_dec(&uqp->uxrcd->refcnt);
 
-	ib_uverbs_release_uevent(attrs->ufile, &uqp->uevent);
+	ib_uverbs_release_uevent(&uqp->uevent);
 	return ret;
 }
 
@@ -138,7 +138,7 @@ static int uverbs_free_wq(struct ib_uobject *uobject,
 	if (ib_is_destroy_retryable(ret, why, uobject))
 		return ret;
 
-	ib_uverbs_release_uevent(attrs->ufile, &uwq->uevent);
+	ib_uverbs_release_uevent(&uwq->uevent);
 	return ret;
 }
 
@@ -163,7 +163,7 @@ static int uverbs_free_srq(struct ib_uobject *uobject,
 		atomic_dec(&us->uxrcd->refcnt);
 	}
 
-	ib_uverbs_release_uevent(attrs->ufile, uevent);
+	ib_uverbs_release_uevent(uevent);
 	return ret;
 }
 
diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
index a41c758..da4110a 100644
--- a/drivers/infiniband/core/uverbs_std_types_cq.c
+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
@@ -49,7 +49,6 @@ static int uverbs_free_cq(struct ib_uobject *uobject,
 		return ret;
 
 	ib_uverbs_release_ucq(
-		attrs->ufile,
 		ev_queue ? container_of(ev_queue,
 					struct ib_uverbs_completion_event_file,
 					ev_queue) :
-- 
1.8.3.1


  parent reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-08 17:21 [PATCH rdma-next 00/14] Refactoring FD usage Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 01/14] RDMA/mlx5: Use RCU and direct refcounts to keep memory alive Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 02/14] RDMA/core: Simplify destruction of FD uobjects Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 03/14] RDMA/mlx5: Simplify devx async commands Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 04/14] RDMA/core: Do not allow alloc_commit to fail Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 05/14] RDMA/core: Make ib_ucq_object use ib_uevent_object Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 06/14] RDMA/core: Do not erase the type of ib_cq.uobject Yishai Hadas
2020-01-08 17:21 ` [PATCH rdma-next 07/14] RDMA/core: Do not erase the type of ib_qp.uobject Yishai Hadas
2020-01-08 17:22 ` [PATCH rdma-next 08/14] RDMA/core: Do not erase the type of ib_srq.uobject Yishai Hadas
2020-01-08 17:22 ` [PATCH rdma-next 09/14] RDMA/core: Do not erase the type of ib_wq.uobject Yishai Hadas
2020-01-08 17:22 ` [PATCH rdma-next 10/14] RDMA/core: Simplify type usage for ib_uverbs_async_handler() Yishai Hadas
2020-01-08 17:22 ` [PATCH rdma-next 11/14] RDMA/core: Fix locking in ib_uverbs_event_read Yishai Hadas
2020-01-10 14:54   ` Jason Gunthorpe
2020-01-08 17:22 ` [PATCH rdma-next 12/14] RDMA/core: Remove the ufile arg from rdma_alloc_begin_uobject Yishai Hadas
2020-01-08 17:22 ` [PATCH rdma-next 13/14] RDMA/core: Make ib_uverbs_async_event_file into a uobject Yishai Hadas
2020-01-08 17:22 ` Yishai Hadas [this message]
2020-01-13 20:37 ` [PATCH rdma-next 00/14] Refactoring FD usage Jason Gunthorpe

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1578504126-9400-15-git-send-email-yishaih@mellanox.com \
    --to=yishaih@mellanox.com \
    --cc=dledford@redhat.com \
    --cc=jgg@mellanox.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=maorg@mellanox.com \
    --cc=michaelgur@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git