[PATCH for-rc 0/6] HFI fixups around expected recv

[PATCH for-rc 0/6] HFI fixups around expected recv

[Dennis Dalessandro]

While working on a FIXME for next cycle Dean found some other issues that we
should probably try to get in for RC. Mostly in the error/teardown case but this
also has a change to reserve expected TIDs which is the more correc behavior and
enables the follow on patches for the FIXME.

---

Dean Luick (6):
      IB/hfi1: Restore allocated resources on failed copyout
      IB/hfi1: Reject a zero-length user expected buffer
      IB/hfi1: Reserve user expected TIDs
      IB/hfi1: Fix expected receive setup error exit issues
      IB/hfi1: Immediately remove invalid memory from hardware
      IB/hfi1: Remove user expected buffer invalidate race


 drivers/infiniband/hw/hfi1/file_ops.c     |   5 +-
 drivers/infiniband/hw/hfi1/user_exp_rcv.c | 200 +++++++++++++++-------
 drivers/infiniband/hw/hfi1/user_exp_rcv.h |   3 +
 3 files changed, 147 insertions(+), 61 deletions(-)

--
-Denny

[PATCH for-rc 1/6] IB/hfi1: Restore allocated resources on failed copyout

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

Fix a resource leak if an error occurs.

Fixes: f404ca4c7ea8 ("IB/hfi1: Refactor hfi_user_exp_rcv_setup() IOCTL")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/file_ops.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index f5f9269fdc16..c9fc913db00c 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -1318,12 +1318,15 @@ static int user_exp_rcv_setup(struct hfi1_filedata *fd, unsigned long arg,
 		addr = arg + offsetof(struct hfi1_tid_info, tidcnt);
 		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
 				 sizeof(tinfo.tidcnt)))
-			return -EFAULT;
+			ret = -EFAULT;
 
 		addr = arg + offsetof(struct hfi1_tid_info, length);
 		if (copy_to_user((void __user *)addr, &tinfo.length,
 				 sizeof(tinfo.length)))
 			ret = -EFAULT;
+
+		if (ret)
+			hfi1_user_exp_rcv_invalid(fd, &tinfo);
 	}
 
 	return ret;

Re: [PATCH for-rc 1/6] IB/hfi1: Restore allocated resources on failed copyout

[Leon Romanovsky]

On Mon, Jan 09, 2023 at 12:31:06PM -0500, Dennis Dalessandro wrote:
> From: Dean Luick <dean.luick@cornelisnetworks.com>
> 
> Fix a resource leak if an error occurs.
> 
> Fixes: f404ca4c7ea8 ("IB/hfi1: Refactor hfi_user_exp_rcv_setup() IOCTL")
> Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> ---
>  drivers/infiniband/hw/hfi1/file_ops.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
> index f5f9269fdc16..c9fc913db00c 100644
> --- a/drivers/infiniband/hw/hfi1/file_ops.c
> +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> @@ -1318,12 +1318,15 @@ static int user_exp_rcv_setup(struct hfi1_filedata *fd, unsigned long arg,
>  		addr = arg + offsetof(struct hfi1_tid_info, tidcnt);
>  		if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
>  				 sizeof(tinfo.tidcnt)))
> -			return -EFAULT;
> +			ret = -EFAULT;

I don't think that it is right to continue to next copy_to_user() if
first one failed.

Thanks

>  
>  		addr = arg + offsetof(struct hfi1_tid_info, length);
>  		if (copy_to_user((void __user *)addr, &tinfo.length,
>  				 sizeof(tinfo.length)))
>  			ret = -EFAULT;
> +
> +		if (ret)
> +			hfi1_user_exp_rcv_invalid(fd, &tinfo);
>  	}
>  
>  	return ret;
> 
> 

[PATCH for-rc 2/6] IB/hfi1: Reject a zero-length user expected buffer

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

A zero length user buffer makes no sense and the code
does not handle it correctly.  Instead, reject a
zero length as invalid.

Fixes: 97736f36dbeb ("IB/hfi1: Validate page aligned for a given virtual addres")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 186d30291260..3c609b11e71c 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -256,6 +256,8 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 
 	if (!PAGE_ALIGNED(tinfo->vaddr))
 		return -EINVAL;
+	if (tinfo->length == 0)
+		return -EINVAL;
 
 	tidbuf = kzalloc(sizeof(*tidbuf), GFP_KERNEL);
 	if (!tidbuf)

[PATCH for-rc 3/6] IB/hfi1: Reserve user expected TIDs

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

To avoid a race, reserve the number of user expected
TIDs before setup.

Fixes: 7e7a436ecb6e ("staging/hfi1: Add TID entry program function body")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 3c609b11e71c..d7487555d109 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -282,16 +282,13 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	/* Find sets of physically contiguous pages */
 	tidbuf->n_psets = find_phys_blocks(tidbuf, pinned);
 
-	/*
-	 * We don't need to access this under a lock since tid_used is per
-	 * process and the same process cannot be in hfi1_user_exp_rcv_clear()
-	 * and hfi1_user_exp_rcv_setup() at the same time.
-	 */
+	/* Reserve the number of expected tids to be used. */
 	spin_lock(&fd->tid_lock);
 	if (fd->tid_used + tidbuf->n_psets > fd->tid_limit)
 		pageset_count = fd->tid_limit - fd->tid_used;
 	else
 		pageset_count = tidbuf->n_psets;
+	fd->tid_used += pageset_count;
 	spin_unlock(&fd->tid_lock);
 
 	if (!pageset_count)
@@ -400,10 +397,11 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 nomem:
 	hfi1_cdbg(TID, "total mapped: tidpairs:%u pages:%u (%d)", tididx,
 		  mapped_pages, ret);
+	/* adjust reserved tid_used to actual count */
+	spin_lock(&fd->tid_lock);
+	fd->tid_used -= pageset_count - tididx;
+	spin_unlock(&fd->tid_lock);
 	if (tididx) {
-		spin_lock(&fd->tid_lock);
-		fd->tid_used += tididx;
-		spin_unlock(&fd->tid_lock);
 		tinfo->tidcnt = tididx;
 		tinfo->length = mapped_pages * PAGE_SIZE;
 

[PATCH for-rc 4/6] IB/hfi1: Fix expected receive setup error exit issues

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

Fix three error exit issues in expected receive setup.
Re-arrange error exits to increase readability.

Issues and fixes:
1. Possible missed page unpin if tidlist copyout fails and
   not all pinned pages where made part of a TID.
   Fix: Unpin the unused pages.

2. Return success with unset return values tidcnt and length
   when no pages were pinned.
   Fix: Return -ENOSPC if no pages were pinned.

3. Return success with unset return values tidcnt and length when
   no rcvarray entries available.
   Fix: Return -ENOSPC if no rcvarray entries are available.

Fixes: 7e7a436ecb6e ("staging/hfi1: Add TID entry program function body")
Fixes: 97736f36dbeb ("IB/hfi1: Validate page aligned for a given virtual addres")
Fixes: f404ca4c7ea8 ("IB/hfi1: Refactor hfi_user_exp_rcv_setup() IOCTL")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   83 +++++++++++++++++------------
 1 file changed, 50 insertions(+), 33 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index d7487555d109..88df8ca4bb57 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -268,15 +268,14 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	tidbuf->psets = kcalloc(uctxt->expected_count, sizeof(*tidbuf->psets),
 				GFP_KERNEL);
 	if (!tidbuf->psets) {
-		kfree(tidbuf);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto fail_release_mem;
 	}
 
 	pinned = pin_rcv_pages(fd, tidbuf);
 	if (pinned <= 0) {
-		kfree(tidbuf->psets);
-		kfree(tidbuf);
-		return pinned;
+		ret = (pinned < 0) ? pinned : -ENOSPC;
+		goto fail_unpin;
 	}
 
 	/* Find sets of physically contiguous pages */
@@ -291,14 +290,16 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	fd->tid_used += pageset_count;
 	spin_unlock(&fd->tid_lock);
 
-	if (!pageset_count)
-		goto bail;
+	if (!pageset_count) {
+		ret = -ENOSPC;
+		goto fail_unreserve;
+	}
 
 	ngroups = pageset_count / dd->rcv_entries.group_size;
 	tidlist = kcalloc(pageset_count, sizeof(*tidlist), GFP_KERNEL);
 	if (!tidlist) {
 		ret = -ENOMEM;
-		goto nomem;
+		goto fail_unreserve;
 	}
 
 	tididx = 0;
@@ -394,44 +395,60 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	}
 unlock:
 	mutex_unlock(&uctxt->exp_mutex);
-nomem:
 	hfi1_cdbg(TID, "total mapped: tidpairs:%u pages:%u (%d)", tididx,
 		  mapped_pages, ret);
+
+	/* fail if nothing was programmed, set error if none provided */
+	if (tididx == 0) {
+		if (ret >= 0)
+			ret = -ENOSPC;
+		goto fail_unreserve;
+	}
+
 	/* adjust reserved tid_used to actual count */
 	spin_lock(&fd->tid_lock);
 	fd->tid_used -= pageset_count - tididx;
 	spin_unlock(&fd->tid_lock);
-	if (tididx) {
-		tinfo->tidcnt = tididx;
-		tinfo->length = mapped_pages * PAGE_SIZE;
 
-		if (copy_to_user(u64_to_user_ptr(tinfo->tidlist),
-				 tidlist, sizeof(tidlist[0]) * tididx)) {
-			/*
-			 * On failure to copy to the user level, we need to undo
-			 * everything done so far so we don't leak resources.
-			 */
-			tinfo->tidlist = (unsigned long)&tidlist;
-			hfi1_user_exp_rcv_clear(fd, tinfo);
-			tinfo->tidlist = 0;
-			ret = -EFAULT;
-			goto bail;
-		}
+	/* unpin all pages not covered by a TID */
+	unpin_rcv_pages(fd, tidbuf, NULL, mapped_pages, pinned - mapped_pages,
+			false);
+
+	tinfo->tidcnt = tididx;
+	tinfo->length = mapped_pages * PAGE_SIZE;
+
+	if (copy_to_user(u64_to_user_ptr(tinfo->tidlist),
+			 tidlist, sizeof(tidlist[0]) * tididx)) {
+		ret = -EFAULT;
+		goto fail_unprogram;
 	}
 
-	/*
-	 * If not everything was mapped (due to insufficient RcvArray entries,
-	 * for example), unpin all unmapped pages so we can pin them nex time.
-	 */
-	if (mapped_pages != pinned)
-		unpin_rcv_pages(fd, tidbuf, NULL, mapped_pages,
-				(pinned - mapped_pages), false);
-bail:
+	kfree(tidbuf->pages);
 	kfree(tidbuf->psets);
+	kfree(tidbuf);
 	kfree(tidlist);
+	return 0;
+
+fail_unprogram:
+	/* unprogram, unmap, and unpin all allocated TIDs */
+	tinfo->tidlist = (unsigned long)tidlist;
+	hfi1_user_exp_rcv_clear(fd, tinfo);
+	tinfo->tidlist = 0;
+	pinned = 0;		/* nothing left to unpin */
+	pageset_count = 0;	/* nothing left reserved */
+fail_unreserve:
+	spin_lock(&fd->tid_lock);
+	fd->tid_used -= pageset_count;
+	spin_unlock(&fd->tid_lock);
+fail_unpin:
+	if (pinned > 0)
+		unpin_rcv_pages(fd, tidbuf, NULL, 0, pinned, false);
+fail_release_mem:
 	kfree(tidbuf->pages);
+	kfree(tidbuf->psets);
 	kfree(tidbuf);
-	return ret > 0 ? 0 : ret;
+	kfree(tidlist);
+	return ret;
 }
 
 int hfi1_user_exp_rcv_clear(struct hfi1_filedata *fd,

[PATCH for-rc 5/6] IB/hfi1: Immediately remove invalid memory from hardware

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

When a user expected receive page is unmapped, it should be
immediately removed from hardware rather than depend on a
reaction from user space.

Fixes: 2677a7680e77 ("IB/hfi1: Fix memory leak during unexpected shutdown")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   43 ++++++++++++++++++++---------
 drivers/infiniband/hw/hfi1/user_exp_rcv.h |    1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 88df8ca4bb57..f402af1e2903 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -28,8 +28,9 @@ static int program_rcvarray(struct hfi1_filedata *fd, struct tid_user_buf *,
 			    unsigned int start, u16 count,
 			    u32 *tidlist, unsigned int *tididx,
 			    unsigned int *pmapped);
-static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo,
-			      struct tid_group **grp);
+static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo);
+static void __clear_tid_node(struct hfi1_filedata *fd,
+			     struct tid_rb_node *node);
 static void clear_tid_node(struct hfi1_filedata *fd, struct tid_rb_node *node);
 
 static const struct mmu_interval_notifier_ops tid_mn_ops = {
@@ -469,7 +470,7 @@ int hfi1_user_exp_rcv_clear(struct hfi1_filedata *fd,
 
 	mutex_lock(&uctxt->exp_mutex);
 	for (tididx = 0; tididx < tinfo->tidcnt; tididx++) {
-		ret = unprogram_rcvarray(fd, tidinfo[tididx], NULL);
+		ret = unprogram_rcvarray(fd, tidinfo[tididx]);
 		if (ret) {
 			hfi1_cdbg(TID, "Failed to unprogram rcv array %d",
 				  ret);
@@ -723,6 +724,7 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
 	}
 
 	node->fdata = fd;
+	mutex_init(&node->invalidate_mutex);
 	node->phys = page_to_phys(pages[0]);
 	node->npages = npages;
 	node->rcventry = rcventry;
@@ -762,8 +764,7 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
 	return -EFAULT;
 }
 
-static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo,
-			      struct tid_group **grp)
+static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo)
 {
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
 	struct hfi1_devdata *dd = uctxt->dd;
@@ -786,9 +787,6 @@ static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo,
 	if (!node || node->rcventry != (uctxt->expected_base + rcventry))
 		return -EBADF;
 
-	if (grp)
-		*grp = node->grp;
-
 	if (fd->use_mn)
 		mmu_interval_notifier_remove(&node->notifier);
 	cacheless_tid_rb_remove(fd, node);
@@ -796,23 +794,34 @@ static int unprogram_rcvarray(struct hfi1_filedata *fd, u32 tidinfo,
 	return 0;
 }
 
-static void clear_tid_node(struct hfi1_filedata *fd, struct tid_rb_node *node)
+static void __clear_tid_node(struct hfi1_filedata *fd, struct tid_rb_node *node)
 {
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
 	struct hfi1_devdata *dd = uctxt->dd;
 
+	mutex_lock(&node->invalidate_mutex);
+	if (node->freed)
+		goto done;
+	node->freed = true;
+
 	trace_hfi1_exp_tid_unreg(uctxt->ctxt, fd->subctxt, node->rcventry,
 				 node->npages,
 				 node->notifier.interval_tree.start, node->phys,
 				 node->dma_addr);
 
-	/*
-	 * Make sure device has seen the write before we unpin the
-	 * pages.
-	 */
+	/* Make sure device has seen the write before pages are unpinned */
 	hfi1_put_tid(dd, node->rcventry, PT_INVALID_FLUSH, 0, 0);
 
 	unpin_rcv_pages(fd, NULL, node, 0, node->npages, true);
+done:
+	mutex_unlock(&node->invalidate_mutex);
+}
+
+static void clear_tid_node(struct hfi1_filedata *fd, struct tid_rb_node *node)
+{
+	struct hfi1_ctxtdata *uctxt = fd->uctxt;
+
+	__clear_tid_node(fd, node);
 
 	node->grp->used--;
 	node->grp->map &= ~(1 << (node->rcventry - node->grp->base));
@@ -871,10 +880,16 @@ static bool tid_rb_invalidate(struct mmu_interval_notifier *mni,
 	if (node->freed)
 		return true;
 
+	/* take action only if unmapping */
+	if (range->event != MMU_NOTIFY_UNMAP)
+		return true;
+
 	trace_hfi1_exp_tid_inval(uctxt->ctxt, fdata->subctxt,
 				 node->notifier.interval_tree.start,
 				 node->rcventry, node->npages, node->dma_addr);
-	node->freed = true;
+
+	/* clear the hardware rcvarray entry */
+	__clear_tid_node(fdata, node);
 
 	spin_lock(&fdata->invalid_lock);
 	if (fdata->invalid_tid_idx < uctxt->expected_count) {
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.h b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
index 8c53e416bf84..2ddb3dac7d91 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
@@ -27,6 +27,7 @@ struct tid_user_buf {
 struct tid_rb_node {
 	struct mmu_interval_notifier notifier;
 	struct hfi1_filedata *fdata;
+	struct mutex invalidate_mutex; /* covers hw removal */
 	unsigned long phys;
 	struct tid_group *grp;
 	u32 rcventry;

[PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Dennis Dalessandro]

From: Dean Luick <dean.luick@cornelisnetworks.com>

During setup, there is a possible race between a page invalidate
and hardware programming.  Add a covering invalidate over the user
target range during setup.  If anything within that range is
invalidated during setup, fail the setup.  Once set up, each
TID will have its own invalidate callback and invalidate.

Fixes: 3889551db212 ("RDMA/hfi1: Use mmu_interval_notifier_insert for user_exp_rcv")
Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   58 +++++++++++++++++++++++++++--
 drivers/infiniband/hw/hfi1/user_exp_rcv.h |    2 +
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index f402af1e2903..b02f2f0809c8 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -23,6 +23,9 @@ static void cacheless_tid_rb_remove(struct hfi1_filedata *fdata,
 static bool tid_rb_invalidate(struct mmu_interval_notifier *mni,
 			      const struct mmu_notifier_range *range,
 			      unsigned long cur_seq);
+static bool tid_cover_invalidate(struct mmu_interval_notifier *mni,
+			         const struct mmu_notifier_range *range,
+			         unsigned long cur_seq);
 static int program_rcvarray(struct hfi1_filedata *fd, struct tid_user_buf *,
 			    struct tid_group *grp,
 			    unsigned int start, u16 count,
@@ -36,6 +39,9 @@ static void clear_tid_node(struct hfi1_filedata *fd, struct tid_rb_node *node);
 static const struct mmu_interval_notifier_ops tid_mn_ops = {
 	.invalidate = tid_rb_invalidate,
 };
+static const struct mmu_interval_notifier_ops tid_cover_ops = {
+	.invalidate = tid_cover_invalidate,
+};
 
 /*
  * Initialize context and file private data needed for Expected
@@ -254,6 +260,7 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 		tididx = 0, mapped, mapped_pages = 0;
 	u32 *tidlist = NULL;
 	struct tid_user_buf *tidbuf;
+	unsigned long mmu_seq = 0;
 
 	if (!PAGE_ALIGNED(tinfo->vaddr))
 		return -EINVAL;
@@ -264,6 +271,7 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	if (!tidbuf)
 		return -ENOMEM;
 
+	mutex_init(&tidbuf->cover_mutex);
 	tidbuf->vaddr = tinfo->vaddr;
 	tidbuf->length = tinfo->length;
 	tidbuf->psets = kcalloc(uctxt->expected_count, sizeof(*tidbuf->psets),
@@ -273,6 +281,16 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 		goto fail_release_mem;
 	}
 
+	if (fd->use_mn) {
+		ret = mmu_interval_notifier_insert(
+			&tidbuf->notifier, current->mm,
+			tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
+			&tid_cover_ops);
+		if (ret)
+			goto fail_release_mem;
+		mmu_seq = mmu_interval_read_begin(&tidbuf->notifier);
+	}
+
 	pinned = pin_rcv_pages(fd, tidbuf);
 	if (pinned <= 0) {
 		ret = (pinned < 0) ? pinned : -ENOSPC;
@@ -415,6 +433,20 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	unpin_rcv_pages(fd, tidbuf, NULL, mapped_pages, pinned - mapped_pages,
 			false);
 
+	if (fd->use_mn) {
+		/* check for an invalidate during setup */
+		bool fail = false;
+
+		mutex_lock(&tidbuf->cover_mutex);
+		fail = mmu_interval_read_retry(&tidbuf->notifier, mmu_seq);
+		mutex_unlock(&tidbuf->cover_mutex);
+
+		if (fail) {
+			ret = -EBUSY;
+			goto fail_unprogram;
+		}
+	}
+
 	tinfo->tidcnt = tididx;
 	tinfo->length = mapped_pages * PAGE_SIZE;
 
@@ -424,6 +456,8 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 		goto fail_unprogram;
 	}
 
+	if (fd->use_mn)
+		mmu_interval_notifier_remove(&tidbuf->notifier);
 	kfree(tidbuf->pages);
 	kfree(tidbuf->psets);
 	kfree(tidbuf);
@@ -442,6 +476,8 @@ int hfi1_user_exp_rcv_setup(struct hfi1_filedata *fd,
 	fd->tid_used -= pageset_count;
 	spin_unlock(&fd->tid_lock);
 fail_unpin:
+	if (fd->use_mn)
+		mmu_interval_notifier_remove(&tidbuf->notifier);
 	if (pinned > 0)
 		unpin_rcv_pages(fd, tidbuf, NULL, 0, pinned, false);
 fail_release_mem:
@@ -740,11 +776,6 @@ static int set_rcvarray_entry(struct hfi1_filedata *fd,
 			&tid_mn_ops);
 		if (ret)
 			goto out_unmap;
-		/*
-		 * FIXME: This is in the wrong order, the notifier should be
-		 * established before the pages are pinned by pin_rcv_pages.
-		 */
-		mmu_interval_read_begin(&node->notifier);
 	}
 	fd->entry_to_rb[node->rcventry - uctxt->expected_base] = node;
 
@@ -919,6 +950,23 @@ static bool tid_rb_invalidate(struct mmu_interval_notifier *mni,
 	return true;
 }
 
+static bool tid_cover_invalidate(struct mmu_interval_notifier *mni,
+			         const struct mmu_notifier_range *range,
+			         unsigned long cur_seq)
+{
+	struct tid_user_buf *tidbuf =
+		container_of(mni, struct tid_user_buf, notifier);
+
+	/* take action only if unmapping */
+	if (range->event == MMU_NOTIFY_UNMAP) {
+		mutex_lock(&tidbuf->cover_mutex);
+		mmu_interval_set_seq(mni, cur_seq);
+		mutex_unlock(&tidbuf->cover_mutex);
+	}
+
+	return true;
+}
+
 static void cacheless_tid_rb_remove(struct hfi1_filedata *fdata,
 				    struct tid_rb_node *tnode)
 {
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.h b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
index 2ddb3dac7d91..f8ee997d0050 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
@@ -16,6 +16,8 @@ struct tid_pageset {
 };
 
 struct tid_user_buf {
+	struct mmu_interval_notifier notifier;
+	struct mutex cover_mutex;
 	unsigned long vaddr;
 	unsigned long length;
 	unsigned int npages;

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Jason Gunthorpe]

On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:

> +	if (fd->use_mn) {
> +		ret = mmu_interval_notifier_insert(
> +			&tidbuf->notifier, current->mm,
> +			tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
> +			&tid_cover_ops);

This is still not the right way to use these notifiers, you should be
removing the calls to mmu_notifier_register()

Jason

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Dean Luick]

On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
> On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
>
>> +    if (fd->use_mn) {
>> +            ret = mmu_interval_notifier_insert(
>> +                    &tidbuf->notifier, current->mm,
>> +                    tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
>> +                    &tid_cover_ops);
>
> This is still not the right way to use these notifiers, you should be
> removing the calls to mmu_notifier_register()

I am confused by your comment.  This is the user expected receive code.  There are no calls to mmu_notifier_register() here.  You removed those calls when you added the FIXME.  The Send DMA side still has calls to mmu_notifier_register().  This series is all about user expected receive.

-Dean

>
> Jason

External recipient

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Jason Gunthorpe]

On Tue, Jan 17, 2023 at 01:19:14PM -0600, Dean Luick wrote:
> On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
> > On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
> >
> >> +    if (fd->use_mn) {
> >> +            ret = mmu_interval_notifier_insert(
> >> +                    &tidbuf->notifier, current->mm,
> >> +                    tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
> >> +                    &tid_cover_ops);
> >
> > This is still not the right way to use these notifiers, you should be
> > removing the calls to mmu_notifier_register()
> 
> I am confused by your comment.  This is the user expected receive
> code.  There are no calls to mmu_notifier_register() here.  You
> removed those calls when you added the FIXME.  The Send DMA side
> still has calls to mmu_notifier_register().  This series is all
> about user expected receive.

Then something else seems wrong because you shouldn't be removing the
notifiers in the same function you add them

Jason

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Dean Luick]

On 1/18/2023 6:42 AM, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2023 at 01:19:14PM -0600, Dean Luick wrote:
>> On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
>>> On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
>>>
>>>> +    if (fd->use_mn) {
>>>> +            ret = mmu_interval_notifier_insert(
>>>> +                    &tidbuf->notifier, current->mm,
>>>> +                    tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
>>>> +                    &tid_cover_ops);
>>>
>>> This is still not the right way to use these notifiers, you should be
>>> removing the calls to mmu_notifier_register()
>>
>> I am confused by your comment.  This is the user expected receive
>> code.  There are no calls to mmu_notifier_register() here.  You
>> removed those calls when you added the FIXME.  The Send DMA side
>> still has calls to mmu_notifier_register().  This series is all
>> about user expected receive.
>
> Then something else seems wrong because you shouldn't be removing the
> notifiers in the same function you add them

The add-then-remove is intentional.  The purpose is to make sure there are no invalidates while we pin pages and set up the "permanent" notifiers that cover exact ranges based on sequential pages and how the DMA hardware is programmed.  Once the programmed hardware range notifiers are in place, the covering range serves no purpose and can be removed.


-Dean

External recipient

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Jason Gunthorpe]

On Thu, Jan 19, 2023 at 10:00:39AM -0600, Dean Luick wrote:
> On 1/18/2023 6:42 AM, Jason Gunthorpe wrote:
> > On Tue, Jan 17, 2023 at 01:19:14PM -0600, Dean Luick wrote:
> >> On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
> >>> On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
> >>>
> >>>> +    if (fd->use_mn) {
> >>>> +            ret = mmu_interval_notifier_insert(
> >>>> +                    &tidbuf->notifier, current->mm,
> >>>> +                    tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
> >>>> +                    &tid_cover_ops);
> >>>
> >>> This is still not the right way to use these notifiers, you should be
> >>> removing the calls to mmu_notifier_register()
> >>
> >> I am confused by your comment.  This is the user expected receive
> >> code.  There are no calls to mmu_notifier_register() here.  You
> >> removed those calls when you added the FIXME.  The Send DMA side
> >> still has calls to mmu_notifier_register().  This series is all
> >> about user expected receive.
> >
> > Then something else seems wrong because you shouldn't be removing the
> > notifiers in the same function you add them
> 
> The add-then-remove is intentional.  The purpose is to make sure
> there are no invalidates while we pin pages and set up the
> "permanent" notifiers that cover exact ranges based on sequential
> pages and how the DMA hardware is programmed.  Once the programmed
> hardware range notifiers are in place, the covering range serves no
> purpose and can be removed.

Uh, that doesn't sound very good, it is very expensive to create these
notifiers, they were not intended to be overly narrowed like this -
you are better to have a wider range and deal with the rare false hit
than to take the cost of register/unregister on every activity??

Also I'm not entirely sure this can be race free, having two notifiers
responsible for the same memory at the same time sounds like Danger
Danger sort of situation..

Jason

Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Dean Luick]

On 1/19/2023 12:05 PM, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2023 at 10:00:39AM -0600, Dean Luick wrote:
>> On 1/18/2023 6:42 AM, Jason Gunthorpe wrote:
>>> On Tue, Jan 17, 2023 at 01:19:14PM -0600, Dean Luick wrote:
>>>> On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
>>>>> On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
>>>>>
>>>>>> +    if (fd->use_mn) {
>>>>>> +            ret = mmu_interval_notifier_insert(
>>>>>> +                    &tidbuf->notifier, current->mm,
>>>>>> +                    tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
>>>>>> +                    &tid_cover_ops);
>>>>>
>>>>> This is still not the right way to use these notifiers, you should be
>>>>> removing the calls to mmu_notifier_register()
>>>>
>>>> I am confused by your comment.  This is the user expected receive
>>>> code.  There are no calls to mmu_notifier_register() here.  You
>>>> removed those calls when you added the FIXME.  The Send DMA side
>>>> still has calls to mmu_notifier_register().  This series is all
>>>> about user expected receive.
>>>
>>> Then something else seems wrong because you shouldn't be removing the
>>> notifiers in the same function you add them
>>
>> The add-then-remove is intentional.  The purpose is to make sure
>> there are no invalidates while we pin pages and set up the
>> "permanent" notifiers that cover exact ranges based on sequential
>> pages and how the DMA hardware is programmed.  Once the programmed
>> hardware range notifiers are in place, the covering range serves no
>> purpose and can be removed.
>
> Uh, that doesn't sound very good, it is very expensive to create these
> notifiers, they were not intended to be overly narrowed like this -
> you are better to have a wider range and deal with the rare false hit
> than to take the cost of register/unregister on every activity??

The temporary cover is a compromise effort to avoid the following situation.  Suppose, as you suggest, we keep a single range on the registration call.  Then:

User registers range A.  The hardware breaks it up into several separate pieces - "TIDs".  Due to pressure, software releases several, but not all the pieces of A.  Because not all of A is gone, the notify range is retained.

Later, software registers range B, which includes parts of the original A.  A new single range is registered.  Since this overlaps with A, we now have multiple range notifiers over the same memory range.  All would still be functionally correct, since there would only be one owner of each TID. But we now have overlapping permanent notify ranges.

Repeat this several times and you can have a mess of overlapping ranges, each with a little piece that is actually live.

With a unique notifier over each TID, the notifier can be dismissed when the TID is released.

We judged it would be simpler and better to keep the smaller, more specific ranges that matched the programmed hardware.  No searching or checking needed.  A 1-1 correspondence.

None of this would be needed if there was a correct, safe way to hold off all memory invalidates in a target range (or just in general) while we set up our per-TID notifiers.


> Also I'm not entirely sure this can be race free, having two notifiers
> responsible for the same memory at the same time sounds like Danger
> Danger sort of situation..

No danger at all.  The temporary "cover" range only registers that something has happened in the whole range.  The "permanent", aka TID, range is responsible for clearing the hardware.  Separate responsibilities.

-Dean

External recipient

Re: [PATCH for-rc 0/6] HFI fixups around expected recv

[Leon Romanovsky]

On Mon, Jan 09, 2023 at 12:31:00PM -0500, Dennis Dalessandro wrote:
> While working on a FIXME for next cycle Dean found some other issues that we
> should probably try to get in for RC. Mostly in the error/teardown case but this
> also has a change to reserve expected TIDs which is the more correc behavior and
> enables the follow on patches for the FIXME.
> 
> ---
> 
> Dean Luick (6):
>       IB/hfi1: Restore allocated resources on failed copyout

This needs to be resent.

>       IB/hfi1: Reject a zero-length user expected buffer
>       IB/hfi1: Reserve user expected TIDs
>       IB/hfi1: Fix expected receive setup error exit issues
>       IB/hfi1: Immediately remove invalid memory from hardware
>       IB/hfi1: Remove user expected buffer invalidate race

These applied to -rc.

Thanks

> 
> 
>  drivers/infiniband/hw/hfi1/file_ops.c     |   5 +-
>  drivers/infiniband/hw/hfi1/user_exp_rcv.c | 200 +++++++++++++++-------
>  drivers/infiniband/hw/hfi1/user_exp_rcv.h |   3 +
>  3 files changed, 147 insertions(+), 61 deletions(-)
> 
> --
> -Denny
>