On Mon, May 14, 2018 at 07:31:22AM +0000, Kalderon, Michal wrote: > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] > Sent: Monday, May 14, 2018 9:37 AM > > > > On Sun, May 13, 2018 at 09:07:07PM +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@xxxxxxxxxx> > > > Signed-off-by: Michal Kalderon <Michal.Kalderon@xxxxxxxxxx> > > > --- > > > drivers/infiniband/hw/qedr/verbs.c | 59 > > > ++++++++++++++++++-------------------- > > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > Please don't leak kernel pointers [1], and use %pK instead of %x [2]. > > > > [1] https://lwn.net/Articles/735589/ > > [2] Documentation/core-api/printk-formats.rst > > > > Thanks > Thanks for pointing these out. I was not familiar with this convention. However, the addresses > Which are printed below are the physical doorbell addresses which don't > Seem to fall into the category of the kernel pointers described in the article as the ones that should > Be protected. Using the %pk doesn't print the value correctly. > Thanks, Can you share the example code for such case? AFAIK, the %pK is actually censored equivalent of %px and that is equal to your %lx. Also prints of vma->vm_start and vma->vm_end are for sure not physical doorbells. Thanks > > > > > > > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > > > b/drivers/infiniband/hw/qedr/verbs.c > > > index 7d3763b..27c0f00 100644 > > > --- a/drivers/infiniband/hw/qedr/verbs.c > > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > > @@ -401,49 +401,46 @@ 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%lx, > > vm_end=0x%lx,vm_pgoff=0x%lx; dpi_start=0x%lx dpi_size=0x%x\n", > > > + vma->vm_start, vma->vm_end, vma->vm_pgoff, 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%lx, end=0x%lx\n", > > > + vma->vm_start, 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%lx, dpi_start=0x%lx, dpi_size=0x%x\n", > > > + phys_addr, 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, > > > -- > > > 2.9.5 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More > > majordomo > > > info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
