[PATCH v2 for-next 0/3] RDMA: Provide an API for drivers to disassociate mmap pages

[PATCH v2 for-next 0/3] RDMA: Provide an API for drivers to disassociate mmap pages

[Junxian Huang]

Provide an API rdma_user_mmap_disassociate() for drivers to disassociate
mmap pages. Use this API in hns to prevent userspace from ringing doorbell
when HW is reset.

v1 -> v2:
* Keep uverbs_user_mmap_disassociate() in uverbs_main.c. The new api
  rdma_user_mmap_disassociate() is also moved to this file.
* Add "#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)" to hns's
  rdma_user_mmap_disassociate() call.

Chengchang Tang (3):
  RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap
    pages
  RDMA/hns: Link all uctx to uctx_list on a device
  RDMA/hns: Disassociate mmap pages for all uctx when HW is being reset

 drivers/infiniband/core/uverbs_main.c       | 21 +++++++++++++++++++++
 drivers/infiniband/hw/hns/hns_roce_device.h |  4 ++++
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 17 +++++++++++++++++
 drivers/infiniband/hw/hns/hns_roce_main.c   | 13 +++++++++++++
 include/rdma/ib_verbs.h                     |  1 +
 5 files changed, 56 insertions(+)

--
2.33.0

[PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Junxian Huang]

From: Chengchang Tang <tangchengchang@huawei.com>

Provide a new api rdma_user_mmap_disassociate() for drivers to
disassociate mmap pages for ucontext.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/infiniband/core/uverbs_main.c | 21 +++++++++++++++++++++
 include/rdma/ib_verbs.h               |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index bc099287de9a..00dab5bfb78c 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -880,6 +880,27 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
 	}
 }
 
+/**
+ * rdma_user_mmap_disassociate() - disassociate the mmap from the ucontext.
+ *
+ * @ucontext: associated user context.
+ *
+ * This function should be called by drivers that need to disable mmap for
+ * some ucontexts.
+ */
+void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext)
+{
+	struct ib_uverbs_file *ufile = ucontext->ufile;
+
+	/* Racing with uverbs_destroy_ufile_hw */
+	if (!down_read_trylock(&ufile->hw_destroy_rwsem))
+		return;
+
+	uverbs_user_mmap_disassociate(ufile);
+	up_read(&ufile->hw_destroy_rwsem);
+}
+EXPORT_SYMBOL(rdma_user_mmap_disassociate);
+
 /*
  * ib_uverbs_open() does not need the BKL:
  *
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index a1dcf812d787..d6e34ca5c727 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2947,6 +2947,7 @@ int rdma_user_mmap_entry_insert_range(struct ib_ucontext *ucontext,
 				      struct rdma_user_mmap_entry *entry,
 				      size_t length, u32 min_pgoff,
 				      u32 max_pgoff);
+void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext);
 
 static inline int
 rdma_user_mmap_entry_insert_exact(struct ib_ucontext *ucontext,
-- 
2.33.0

[PATCH v2 for-next 2/3] RDMA/hns: Link all uctx to uctx_list on a device

[Junxian Huang]

From: Chengchang Tang <tangchengchang@huawei.com>

Link all uctx to uctx_list on a device.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/infiniband/hw/hns/hns_roce_device.h |  4 ++++
 drivers/infiniband/hw/hns/hns_roce_main.c   | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 0b1e21cb6d2d..93768660569d 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -217,6 +217,7 @@ struct hns_roce_ucontext {
 	struct mutex		page_mutex;
 	struct hns_user_mmap_entry *db_mmap_entry;
 	u32			config;
+	struct list_head list; /* link all uctx to uctx_list on hr_dev */
 };
 
 struct hns_roce_pd {
@@ -1030,6 +1031,9 @@ struct hns_roce_dev {
 	u64 dwqe_page;
 	struct hns_roce_dev_debugfs dbgfs;
 	atomic64_t *dfx_cnt;
+
+	struct list_head	uctx_list; /* list of all uctx on this dev */
+	struct mutex		uctx_list_mutex; /* protect @uctx_list */
 };
 
 static inline struct hns_roce_dev *to_hr_dev(struct ib_device *ib_dev)
diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
index 4cb0af733587..e19871ebc315 100644
--- a/drivers/infiniband/hw/hns/hns_roce_main.c
+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
@@ -426,6 +426,10 @@ static int hns_roce_alloc_ucontext(struct ib_ucontext *uctx,
 	if (ret)
 		goto error_fail_copy_to_udata;
 
+	mutex_lock(&hr_dev->uctx_list_mutex);
+	list_add(&context->list, &hr_dev->uctx_list);
+	mutex_unlock(&hr_dev->uctx_list_mutex);
+
 	return 0;
 
 error_fail_copy_to_udata:
@@ -448,6 +452,10 @@ static void hns_roce_dealloc_ucontext(struct ib_ucontext *ibcontext)
 	struct hns_roce_ucontext *context = to_hr_ucontext(ibcontext);
 	struct hns_roce_dev *hr_dev = to_hr_dev(ibcontext->device);
 
+	mutex_lock(&hr_dev->uctx_list_mutex);
+	list_del(&context->list);
+	mutex_unlock(&hr_dev->uctx_list_mutex);
+
 	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_CQ_RECORD_DB ||
 	    hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_RECORD_DB)
 		mutex_destroy(&context->page_mutex);
@@ -943,6 +951,7 @@ static int hns_roce_init_hem(struct hns_roce_dev *hr_dev)
 static void hns_roce_teardown_hca(struct hns_roce_dev *hr_dev)
 {
 	hns_roce_cleanup_bitmap(hr_dev);
+	mutex_destroy(&hr_dev->uctx_list_mutex);
 
 	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_CQ_RECORD_DB ||
 	    hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_RECORD_DB)
@@ -961,6 +970,9 @@ static int hns_roce_setup_hca(struct hns_roce_dev *hr_dev)
 
 	spin_lock_init(&hr_dev->sm_lock);
 
+	INIT_LIST_HEAD(&hr_dev->uctx_list);
+	mutex_init(&hr_dev->uctx_list_mutex);
+
 	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_CQ_RECORD_DB ||
 	    hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_RECORD_DB) {
 		INIT_LIST_HEAD(&hr_dev->pgdir_list);
@@ -1000,6 +1012,7 @@ static int hns_roce_setup_hca(struct hns_roce_dev *hr_dev)
 	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_CQ_RECORD_DB ||
 	    hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_RECORD_DB)
 		mutex_destroy(&hr_dev->pgdir_mutex);
+	mutex_destroy(&hr_dev->uctx_list_mutex);
 
 	return ret;
 }
-- 
2.33.0

[PATCH v2 for-next 3/3] RDMA/hns: Disassociate mmap pages for all uctx when HW is being reset

[Junxian Huang]

From: Chengchang Tang <tangchengchang@huawei.com>

When HW is being reset, userspace should not ring doorbell otherwise
it may lead to abnormal consequence such as RAS.

Disassociate mmap pages for all uctx to prevent userspace from ringing
doorbell to HW.

Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver")
Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 621b057fb9da..e30610a426b3 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -7012,6 +7012,20 @@ static void hns_roce_hw_v2_uninit_instance(struct hnae3_handle *handle,
 
 	handle->rinfo.instance_state = HNS_ROCE_STATE_NON_INIT;
 }
+
+static void hns_roce_v2_reset_notify_user(struct hns_roce_dev *hr_dev)
+{
+	struct hns_roce_ucontext *uctx, *tmp;
+
+	mutex_lock(&hr_dev->uctx_list_mutex);
+	list_for_each_entry_safe(uctx, tmp, &hr_dev->uctx_list, list) {
+#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
+		rdma_user_mmap_disassociate(&uctx->ibucontext);
+#endif
+	}
+	mutex_unlock(&hr_dev->uctx_list_mutex);
+}
+
 static int hns_roce_hw_v2_reset_notify_down(struct hnae3_handle *handle)
 {
 	struct hns_roce_dev *hr_dev;
@@ -7030,6 +7044,9 @@ static int hns_roce_hw_v2_reset_notify_down(struct hnae3_handle *handle)
 
 	hr_dev->active = false;
 	hr_dev->dis_db = true;
+
+	hns_roce_v2_reset_notify_user(hr_dev);
+
 	hr_dev->state = HNS_ROCE_DEVICE_STATE_RST_DOWN;
 
 	return 0;
-- 
2.33.0

Re: [PATCH v2 for-next 3/3] RDMA/hns: Disassociate mmap pages for all uctx when HW is being reset

[Jason Gunthorpe]

On Mon, Aug 12, 2024 at 08:56:40PM +0800, Junxian Huang wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> When HW is being reset, userspace should not ring doorbell otherwise
> it may lead to abnormal consequence such as RAS.
> 
> Disassociate mmap pages for all uctx to prevent userspace from ringing
> doorbell to HW.
> 
> Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver")
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index 621b057fb9da..e30610a426b3 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -7012,6 +7012,20 @@ static void hns_roce_hw_v2_uninit_instance(struct hnae3_handle *handle,
>  
>  	handle->rinfo.instance_state = HNS_ROCE_STATE_NON_INIT;
>  }
> +
> +static void hns_roce_v2_reset_notify_user(struct hns_roce_dev *hr_dev)
> +{
> +	struct hns_roce_ucontext *uctx, *tmp;
> +
> +	mutex_lock(&hr_dev->uctx_list_mutex);
> +	list_for_each_entry_safe(uctx, tmp, &hr_dev->uctx_list, list) {
> +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
> +		rdma_user_mmap_disassociate(&uctx->ibucontext);
> +#endif
> +	}
> +	mutex_unlock(&hr_dev->uctx_list_mutex);
> +}

I'm not keen on the drivers having to maintain a list of ucontexts
when the caller already does it.

Try something like this?

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 00dab5bfb78cc8..6b4d5a660a2f9d 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -76,6 +76,7 @@ static dev_t dynamic_uverbs_dev;
 static DEFINE_IDA(uverbs_ida);
 static int ib_uverbs_add_one(struct ib_device *device);
 static void ib_uverbs_remove_one(struct ib_device *device, void *client_data);
+static struct ib_client uverbs_client;
 
 static char *uverbs_devnode(const struct device *dev, umode_t *mode)
 {
@@ -881,23 +882,25 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
 }
 
 /**
- * rdma_user_mmap_disassociate() - disassociate the mmap from the ucontext.
+ * rdma_user_mmap_disassociate() - Revoke mmaps for a device
+ * @device: device to revoke
  *
- * @ucontext: associated user context.
- *
- * This function should be called by drivers that need to disable mmap for
- * some ucontexts.
+ * This function should be called by drivers that need to disable mmaps for the
+ * device, for instance because it is going to be reset.
  */
-void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext)
+void rdma_user_mmap_disassociate(struct ib_device *device)
 {
-	struct ib_uverbs_file *ufile = ucontext->ufile;
+	struct ib_uverbs_device *uverbs_dev =
+		ib_get_client_data(device, &uverbs_client);
+	struct ib_uverbs_file *ufile;
 
-	/* Racing with uverbs_destroy_ufile_hw */
-	if (!down_read_trylock(&ufile->hw_destroy_rwsem))
-		return;
-
-	uverbs_user_mmap_disassociate(ufile);
-	up_read(&ufile->hw_destroy_rwsem);
+	mutex_lock(&uverbs_dev->lists_mutex);
+	list_for_each_entry(ufile, &uverbs_dev->uverbs_file_list, list) {
+		down_read(&ufile->hw_destroy_rwsem);
+		uverbs_user_mmap_disassociate(ufile);
+		up_read(&ufile->hw_destroy_rwsem);
+	}
+	mutex_unlock(&uverbs_dev->lists_mutex);
 }
 EXPORT_SYMBOL(rdma_user_mmap_disassociate);
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index e30610a426b387..ecf4f1c9f51d32 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -7015,15 +7015,7 @@ static void hns_roce_hw_v2_uninit_instance(struct hnae3_handle *handle,
 
 static void hns_roce_v2_reset_notify_user(struct hns_roce_dev *hr_dev)
 {
-	struct hns_roce_ucontext *uctx, *tmp;
-
-	mutex_lock(&hr_dev->uctx_list_mutex);
-	list_for_each_entry_safe(uctx, tmp, &hr_dev->uctx_list, list) {
-#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
-		rdma_user_mmap_disassociate(&uctx->ibucontext);
-#endif
-	}
-	mutex_unlock(&hr_dev->uctx_list_mutex);
+	rdma_user_mmap_disassociate(&hr_dev->ib_dev);
 }
 
 static int hns_roce_hw_v2_reset_notify_down(struct hnae3_handle *handle)
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index d6e34ca5c7276c..de9db15243c66f 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2947,7 +2947,13 @@ int rdma_user_mmap_entry_insert_range(struct ib_ucontext *ucontext,
 				      struct rdma_user_mmap_entry *entry,
 				      size_t length, u32 min_pgoff,
 				      u32 max_pgoff);
-void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext);
+#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
+void rdma_user_mmap_disassociate(struct ib_device *device);
+#else
+static inline void rdma_user_mmap_disassociate(struct ib_device *device)
+{
+}
+#endif
 
 static inline int
 rdma_user_mmap_entry_insert_exact(struct ib_ucontext *ucontext,

Re: [PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Jason Gunthorpe]

On Mon, Aug 12, 2024 at 08:56:38PM +0800, Junxian Huang wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Provide a new api rdma_user_mmap_disassociate() for drivers to
> disassociate mmap pages for ucontext.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> ---
>  drivers/infiniband/core/uverbs_main.c | 21 +++++++++++++++++++++
>  include/rdma/ib_verbs.h               |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index bc099287de9a..00dab5bfb78c 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -880,6 +880,27 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
>  	}
>  }
>  
> +/**
> + * rdma_user_mmap_disassociate() - disassociate the mmap from the ucontext.
> + *
> + * @ucontext: associated user context.
> + *
> + * This function should be called by drivers that need to disable mmap for
> + * some ucontexts.
> + */
> +void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext)
> +{
> +	struct ib_uverbs_file *ufile = ucontext->ufile;
> +
> +	/* Racing with uverbs_destroy_ufile_hw */
> +	if (!down_read_trylock(&ufile->hw_destroy_rwsem))
> +		return;

This is not quite right here, in the other cases lock failure is
aborting an operation that is about to start, in this case we are must
ensure the zap completed otherwise we break the contract of no mmaps
upon return.

So at least this needs to be a naked down_read()

But..

That lock lockdep assertion in uverbs_user_mmap_disassociate() I think
was supposed to say the write side is held, which we can't actually
get here.

This is because the nasty algorithm works by pulling things off the
list, if we don't have a lock then one thread could be working on an
item while another thread is unaware which will also break the
contract.

You may need to add a dedicated mutex inside
uverbs_user_mmap_disassociate() and not try to re-use
hw_destroy_rwsem.

Jason

Re: [PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Junxian Huang]

On 2024/8/23 23:25, Jason Gunthorpe wrote:
> On Mon, Aug 12, 2024 at 08:56:38PM +0800, Junxian Huang wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> Provide a new api rdma_user_mmap_disassociate() for drivers to
>> disassociate mmap pages for ucontext.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>> ---
>>  drivers/infiniband/core/uverbs_main.c | 21 +++++++++++++++++++++
>>  include/rdma/ib_verbs.h               |  1 +
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
>> index bc099287de9a..00dab5bfb78c 100644
>> --- a/drivers/infiniband/core/uverbs_main.c
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -880,6 +880,27 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
>>  	}
>>  }
>>  
>> +/**
>> + * rdma_user_mmap_disassociate() - disassociate the mmap from the ucontext.
>> + *
>> + * @ucontext: associated user context.
>> + *
>> + * This function should be called by drivers that need to disable mmap for
>> + * some ucontexts.
>> + */
>> +void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext)
>> +{
>> +	struct ib_uverbs_file *ufile = ucontext->ufile;
>> +
>> +	/* Racing with uverbs_destroy_ufile_hw */
>> +	if (!down_read_trylock(&ufile->hw_destroy_rwsem))
>> +		return;
> 
> This is not quite right here, in the other cases lock failure is
> aborting an operation that is about to start, in this case we are must
> ensure the zap completed otherwise we break the contract of no mmaps
> upon return.
> 
> So at least this needs to be a naked down_read()
> 
> But..
> 
> That lock lockdep assertion in uverbs_user_mmap_disassociate() I think
> was supposed to say the write side is held, which we can't actually
> get here.
> 
> This is because the nasty algorithm works by pulling things off the
> list, if we don't have a lock then one thread could be working on an
> item while another thread is unaware which will also break the
> contract.
> 
> You may need to add a dedicated mutex inside
> uverbs_user_mmap_disassociate() and not try to re-use
> hw_destroy_rwsem.
> 

Thanks for the reply, Jason. Based on your modification in patch #3,
we plan to change the codes like this (some init and destroy are omitted
here). We'll re-send as soon as we finish the testings.

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 821d93c8f712..05b589aad5ef 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -160,6 +160,9 @@ struct ib_uverbs_file {
        struct page *disassociate_page;

        struct xarray           idr;
+
+       struct mutex disassociation_lock;
+       bool disassociated;
 };

 struct ib_uverbs_event {
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 6b4d5a660a2f..6503f9a23211 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -722,12 +722,12 @@ static void rdma_umap_open(struct vm_area_struct *vma)
                return;

        /* We are racing with disassociation */
-       if (!down_read_trylock(&ufile->hw_destroy_rwsem))
+       if (!mutex_trylock(&ufile->disassociation_lock))
                goto out_zap;
        /*
         * Disassociation already completed, the VMA should already be zapped.
         */
-       if (!ufile->ucontext)
+       if (!ufile->ucontext || ufile->disassociated)
                goto out_unlock;

        priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -735,11 +735,11 @@ static void rdma_umap_open(struct vm_area_struct *vma)
                goto out_unlock;
        rdma_umap_priv_init(priv, vma, opriv->entry);

-       up_read(&ufile->hw_destroy_rwsem);
+       mutex_unlock(&ufile->disassociation_lock);
        return;

 out_unlock:
-       up_read(&ufile->hw_destroy_rwsem);
+       mutex_unlock(&ufile->disassociation_lock);
 out_zap:
        /*
         * We can't allow the VMA to be created with the actual IO pages, that
@@ -823,6 +823,8 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
        struct rdma_umap_priv *priv, *next_priv;

        lockdep_assert_held(&ufile->hw_destroy_rwsem);
+       mutex_lock(&ufile->disassociation_lock);
+       ufile->disassociated = true;

        while (1) {
                struct mm_struct *mm = NULL;
@@ -848,8 +850,10 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
                        break;
                }
                mutex_unlock(&ufile->umap_lock);
-               if (!mm)
+               if (!mm) {
+                       mutex_unlock(&ufile->disassociation_lock);
                        return;
+               }

                /*
                 * The umap_lock is nested under mmap_lock since it used within
@@ -879,6 +883,8 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
                mmap_read_unlock(mm);
                mmput(mm);
        }
+
+       mutex_unlock(&ufile->disassociation_lock);
 }

 /**
@@ -897,7 +903,8 @@ void rdma_user_mmap_disassociate(struct ib_device *device)
        mutex_lock(&uverbs_dev->lists_mutex);
        list_for_each_entry(ufile, &uverbs_dev->uverbs_file_list, list) {
                down_read(&ufile->hw_destroy_rwsem);
-               uverbs_user_mmap_disassociate(ufile);
+               if (ufile->ucontext && !ufile->disassociated)
+                       uverbs_user_mmap_disassociate(ufile);
                up_read(&ufile->hw_destroy_rwsem);
        }
        mutex_unlock(&uverbs_dev->lists_mutex);


> Jason

Re: [PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Jason Gunthorpe]

On Mon, Aug 26, 2024 at 09:12:33PM +0800, Junxian Huang wrote:
 
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 821d93c8f712..05b589aad5ef 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -160,6 +160,9 @@ struct ib_uverbs_file {
>         struct page *disassociate_page;
> 
>         struct xarray           idr;
> +
> +       struct mutex disassociation_lock;
> +       bool disassociated;
>  };
> 
>  struct ib_uverbs_event {
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 6b4d5a660a2f..6503f9a23211 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -722,12 +722,12 @@ static void rdma_umap_open(struct vm_area_struct *vma)
>                 return;
> 
>         /* We are racing with disassociation */
> -       if (!down_read_trylock(&ufile->hw_destroy_rwsem))
> +       if (!mutex_trylock(&ufile->disassociation_lock))
>                 goto out_zap;

Nonon, don't touch this stuff! It is fine as is

The extra lock should be in the mmap zap functions only

Jason

Re: [PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Junxian Huang]

On 2024/8/26 21:16, Jason Gunthorpe wrote:
> On Mon, Aug 26, 2024 at 09:12:33PM +0800, Junxian Huang wrote:
>  
>> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
>> index 821d93c8f712..05b589aad5ef 100644
>> --- a/drivers/infiniband/core/uverbs.h
>> +++ b/drivers/infiniband/core/uverbs.h
>> @@ -160,6 +160,9 @@ struct ib_uverbs_file {
>>         struct page *disassociate_page;
>>
>>         struct xarray           idr;
>> +
>> +       struct mutex disassociation_lock;
>> +       bool disassociated;
>>  };
>>
>>  struct ib_uverbs_event {
>> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
>> index 6b4d5a660a2f..6503f9a23211 100644
>> --- a/drivers/infiniband/core/uverbs_main.c
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -722,12 +722,12 @@ static void rdma_umap_open(struct vm_area_struct *vma)
>>                 return;
>>
>>         /* We are racing with disassociation */
>> -       if (!down_read_trylock(&ufile->hw_destroy_rwsem))
>> +       if (!mutex_trylock(&ufile->disassociation_lock))
>>                 goto out_zap;
> 
> Nonon, don't touch this stuff! It is fine as is
> 
> The extra lock should be in the mmap zap functions only
> 
> Jason

Okay, I got it. Thanks

Junxian