| Message ID | 20180515121333.8949-1-Michal.Kalderon@cavium.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
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>
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 --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,
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(-)