diff mbox

[for-rc,v2] RDMA/qedr: Fix doorbell bar mapping for dpi > 1

Message ID 20180515121333.8949-1-Michal.Kalderon@cavium.com (mailing list archive)
State Accepted
Delegated to: Jason Gunthorpe
Headers show

Commit Message

Kalderon, Michal May 15, 2018, 12:13 p.m. UTC
Each user_context receives a separate dpi value and thus a different
address on the doorbell bar. The qedr_mmap function needs to validate
the address and map the doorbell bar accordingly.
The current implementation always checked against dpi=0 doorbell range
leading to a wrong mapping for doorbell bar. (It entered an else case
that mapped the address differently). qedr_mmap should only be used
for doorbells, so the else was actually wrong in the first place.
This only has an affect on arm architecture and not an issue on a
x86 based architecture.
This lead to doorbells not occurring on arm based systems and left
applications that use more than one dpi (or several applications
run simultaneously ) to hang.

Fixes: ac1b36e55a51 ("qedr: Add support for user context verbs")
Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
---
V2
	Use %pK to avoid leaking kernel pointers. 
---
 drivers/infiniband/hw/qedr/verbs.c | 60 ++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

Comments

Leon Romanovsky May 15, 2018, 12:50 p.m. UTC | #1
On Tue, May 15, 2018 at 03:13:33PM +0300, Michal Kalderon wrote:
> Each user_context receives a separate dpi value and thus a different
> address on the doorbell bar. The qedr_mmap function needs to validate
> the address and map the doorbell bar accordingly.
> The current implementation always checked against dpi=0 doorbell range
> leading to a wrong mapping for doorbell bar. (It entered an else case
> that mapped the address differently). qedr_mmap should only be used
> for doorbells, so the else was actually wrong in the first place.
> This only has an affect on arm architecture and not an issue on a
> x86 based architecture.
> This lead to doorbells not occurring on arm based systems and left
> applications that use more than one dpi (or several applications
> run simultaneously ) to hang.
>
> Fixes: ac1b36e55a51 ("qedr: Add support for user context verbs")
> Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
> ---
> V2
> 	Use %pK to avoid leaking kernel pointers.
> ---
>  drivers/infiniband/hw/qedr/verbs.c | 60 ++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
Jason Gunthorpe May 23, 2018, 9:49 p.m. UTC | #2
On Tue, May 15, 2018 at 03:13:33PM +0300, Kalderon, Michal wrote:
> Each user_context receives a separate dpi value and thus a different
> address on the doorbell bar. The qedr_mmap function needs to validate
> the address and map the doorbell bar accordingly.
> The current implementation always checked against dpi=0 doorbell range
> leading to a wrong mapping for doorbell bar. (It entered an else case
> that mapped the address differently). qedr_mmap should only be used
> for doorbells, so the else was actually wrong in the first place.
> This only has an affect on arm architecture and not an issue on a
> x86 based architecture.
> This lead to doorbells not occurring on arm based systems and left
> applications that use more than one dpi (or several applications
> run simultaneously ) to hang.
> 
> Fixes: ac1b36e55a51 ("qedr: Add support for user context verbs")
> Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> V2
> 	Use %pK to avoid leaking kernel pointers. 
> ---
>  drivers/infiniband/hw/qedr/verbs.c | 60 ++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)

Applied to for-rc

> diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
> index 7d3763b..3f9afc0 100644
> --- a/drivers/infiniband/hw/qedr/verbs.c
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -401,49 +401,47 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
>  {
>  	struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
>  	struct qedr_dev *dev = get_qedr_dev(context->device);
> -	unsigned long vm_page = vma->vm_pgoff << PAGE_SHIFT;
> -	u64 unmapped_db = dev->db_phys_addr;
> +	unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
>  	unsigned long len = (vma->vm_end - vma->vm_start);
> -	int rc = 0;
> -	bool found;
> +	unsigned long dpi_start;
> +
> +	dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
>  
>  	DP_DEBUG(dev, QEDR_MSG_INIT,
> -		 "qedr_mmap called vm_page=0x%lx vm_pgoff=0x%lx unmapped_db=0x%llx db_size=%x, len=%lx\n",
> -		 vm_page, vma->vm_pgoff, unmapped_db, dev->db_size, len);
> -	if (vma->vm_start & (PAGE_SIZE - 1)) {
> -		DP_ERR(dev, "Vma_start not page aligned = %ld\n",
> -		       vma->vm_start);
> +		 "mmap invoked with vm_start=0x%pK, vm_end=0x%pK,vm_pgoff=0x%pK; dpi_start=0x%pK dpi_size=0x%x\n",
> +		 (void *)vma->vm_start, (void *)vma->vm_end,
> +		 (void *)vma->vm_pgoff, (void *)dpi_start, ucontext->dpi_size);
> +
> +	if ((vma->vm_start & (PAGE_SIZE - 1)) || (len & (PAGE_SIZE - 1))) {
> +		DP_ERR(dev,
> +		       "failed mmap, adrresses must be page aligned: start=0x%pK, end=0x%pK\n",
> +		       (void *)vma->vm_start, (void *)vma->vm_end);
>  		return -EINVAL;
>  	}
>  
> -	found = qedr_search_mmap(ucontext, vm_page, len);
> -	if (!found) {
> -		DP_ERR(dev, "Vma_pgoff not found in mapped array = %ld\n",
> +	if (!qedr_search_mmap(ucontext, phys_addr, len)) {
> +		DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not authorized\n",
>  		       vma->vm_pgoff);
>  		return -EINVAL;
>  	}
>  
> -	DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping doorbell bar\n");
> -
> -	if ((vm_page >= unmapped_db) && (vm_page <= (unmapped_db +
> -						     dev->db_size))) {
> -		DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping doorbell bar\n");
> -		if (vma->vm_flags & VM_READ) {
> -			DP_ERR(dev, "Trying to map doorbell bar for read\n");
> -			return -EPERM;
> -		}
> -
> -		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +	if (phys_addr < dpi_start ||
> +	    ((phys_addr + len) > (dpi_start + ucontext->dpi_size))) {
> +		DP_ERR(dev,
> +		       "failed mmap, pages are outside of dpi; page address=0x%pK, dpi_start=0x%pK, dpi_size=0x%x\n",
> +		       (void *)phys_addr, (void *)dpi_start,
> +		       ucontext->dpi_size);
> +		return -EINVAL;
> +	}
>  
> -		rc = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> -					PAGE_SIZE, vma->vm_page_prot);
> -	} else {
> -		DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping chains\n");
> -		rc = remap_pfn_range(vma, vma->vm_start,
> -				     vma->vm_pgoff, len, vma->vm_page_prot);
> +	if (vma->vm_flags & VM_READ) {
> +		DP_ERR(dev, "failed mmap, cannot map doorbell bar for read\n");
> +		return -EINVAL;
>  	}
> -	DP_DEBUG(dev, QEDR_MSG_INIT, "qedr_mmap return code: %d\n", rc);
> -	return rc;
> +
> +	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +	return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, len,
> +				  vma->vm_page_prot);
>  }
>  
>  struct ib_pd *qedr_alloc_pd(struct ib_device *ibdev,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 7d3763b..3f9afc0 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -401,49 +401,47 @@  int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
 {
 	struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
 	struct qedr_dev *dev = get_qedr_dev(context->device);
-	unsigned long vm_page = vma->vm_pgoff << PAGE_SHIFT;
-	u64 unmapped_db = dev->db_phys_addr;
+	unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long len = (vma->vm_end - vma->vm_start);
-	int rc = 0;
-	bool found;
+	unsigned long dpi_start;
+
+	dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
 
 	DP_DEBUG(dev, QEDR_MSG_INIT,
-		 "qedr_mmap called vm_page=0x%lx vm_pgoff=0x%lx unmapped_db=0x%llx db_size=%x, len=%lx\n",
-		 vm_page, vma->vm_pgoff, unmapped_db, dev->db_size, len);
-	if (vma->vm_start & (PAGE_SIZE - 1)) {
-		DP_ERR(dev, "Vma_start not page aligned = %ld\n",
-		       vma->vm_start);
+		 "mmap invoked with vm_start=0x%pK, vm_end=0x%pK,vm_pgoff=0x%pK; dpi_start=0x%pK dpi_size=0x%x\n",
+		 (void *)vma->vm_start, (void *)vma->vm_end,
+		 (void *)vma->vm_pgoff, (void *)dpi_start, ucontext->dpi_size);
+
+	if ((vma->vm_start & (PAGE_SIZE - 1)) || (len & (PAGE_SIZE - 1))) {
+		DP_ERR(dev,
+		       "failed mmap, adrresses must be page aligned: start=0x%pK, end=0x%pK\n",
+		       (void *)vma->vm_start, (void *)vma->vm_end);
 		return -EINVAL;
 	}
 
-	found = qedr_search_mmap(ucontext, vm_page, len);
-	if (!found) {
-		DP_ERR(dev, "Vma_pgoff not found in mapped array = %ld\n",
+	if (!qedr_search_mmap(ucontext, phys_addr, len)) {
+		DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not authorized\n",
 		       vma->vm_pgoff);
 		return -EINVAL;
 	}
 
-	DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping doorbell bar\n");
-
-	if ((vm_page >= unmapped_db) && (vm_page <= (unmapped_db +
-						     dev->db_size))) {
-		DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping doorbell bar\n");
-		if (vma->vm_flags & VM_READ) {
-			DP_ERR(dev, "Trying to map doorbell bar for read\n");
-			return -EPERM;
-		}
-
-		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+	if (phys_addr < dpi_start ||
+	    ((phys_addr + len) > (dpi_start + ucontext->dpi_size))) {
+		DP_ERR(dev,
+		       "failed mmap, pages are outside of dpi; page address=0x%pK, dpi_start=0x%pK, dpi_size=0x%x\n",
+		       (void *)phys_addr, (void *)dpi_start,
+		       ucontext->dpi_size);
+		return -EINVAL;
+	}
 
-		rc = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-					PAGE_SIZE, vma->vm_page_prot);
-	} else {
-		DP_DEBUG(dev, QEDR_MSG_INIT, "Mapping chains\n");
-		rc = remap_pfn_range(vma, vma->vm_start,
-				     vma->vm_pgoff, len, vma->vm_page_prot);
+	if (vma->vm_flags & VM_READ) {
+		DP_ERR(dev, "failed mmap, cannot map doorbell bar for read\n");
+		return -EINVAL;
 	}
-	DP_DEBUG(dev, QEDR_MSG_INIT, "qedr_mmap return code: %d\n", rc);
-	return rc;
+
+	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+	return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, len,
+				  vma->vm_page_prot);
 }
 
 struct ib_pd *qedr_alloc_pd(struct ib_device *ibdev,