| Message ID | 20180513180707.22003-1-Michal.Kalderon@cavium.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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@cavium.com> > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > --- > 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 > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Leon Romanovsky [mailto:leon@kernel.org] 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@cavium.com> > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > --- > > 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, > > > > > 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@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- 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
On Mon, May 14, 2018 at 07:31:22AM +0000, Kalderon, Michal wrote: > From: Leon Romanovsky [mailto:leon@kernel.org] > 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@cavium.com> > > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > > --- > > > 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@vger.kernel.org More > > majordomo > > > info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 07:31:22AM +0000, Kalderon, Michal wrote: > From: Leon Romanovsky [mailto:leon@kernel.org] > 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@cavium.com> > > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > > 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 I think PCI physical addresses are expected to be protected as well. Jason -- 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
From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Leon Romanovsky <leon@kernel.org> Sent: Monday, May 14, 2018 11:06 AM > > > > > > 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. Modified the code as follows for comparison: DP_DEBUG(dev, QEDR_MSG_INIT, "mmap (pk) invoked with vm_start=0x%pk,x 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); DP_DEBUG(dev, QEDR_MSG_INIT, "mmap (px) invoked with vm_start=0x%px,x vm_end=0x%px,vm_pgoff=0x%px; dpi_start=0x%px dpi_size=0x%x\n", (void *)vma->vm_start, (void *)vma->vm_end, (void *)vma->vm_pgoff, (void *)dpi_start, ucontext->dpi_size); DP_DEBUG(dev, QEDR_MSG_INIT, "mmap invoked with vm_start=0x%lx,x 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); and got the following output: [32937.851202] (qedr1) INIT: mmap (pk) invoked with vm_start=0x000000009dacc20a,x vm_end=0x0000000040480bf1,vm_pgoff=0x00000000d9849dd6; dpi_start=0x00000000c74fd5ea dpi_size=0x2000 [32937.851203] (qedr1) INIT: mmap (px) invoked with vm_start=0x00007f20575ad000,x vm_end=0x00007f20575af000,vm_pgoff=0x0000000000091b03; dpi_start=0x0000000091b03000 dpi_size=0x2000 [32937.851205] (qedr1) INIT: mmap invoked with vm_start=0x7f20575ad000,x vm_end=0x7f20575af000,vm_pgoff=0x91b03; dpi_start=0x91b03000 dpi_size=0x2000 looks like the %pk gives hashed results similar to the %p even if the kptr_restrict is zero. sysctl kernel/kptr_restrict kernel.kptr_restrict = 0 > Thanks -- 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
From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe <jgg@mellanox.com> > > > > 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 > I think PCI physical addresses are expected to be protected as well. Not sure what can be done though with this physical address, as it is an address into the devices doorbell -> any attempt to write data that is not in the doorbell format will be dropped by device. It's important for debugging to see the actual doorbell address. (and it seems %pK gives a hashed value and not actual value like %lx) I can send a v2 to change the vma addresses to be printed with %pk although they are printed in other driver (mlx) with %lx as well. Since i didn't add new prints in this patch - only modified existing ones, could you please consider accepting this bug fix and i'll provide a separate patch for modifying the vma prints? thanks, Michal Jason -- 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 -- 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
On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe <jgg@mellanox.com> > > > > > > 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 > > > I think PCI physical addresses are expected to be protected as well. > > Not sure what can be done though with this physical address, as it is > an address into the devices doorbell -> any attempt to write data that is > not in the doorbell format will be dropped by device. It obscures information about the kernel memory layout which is the point of all of this. If you want to report the doorbell address for debugging then report it as an offset from the start of the device's bar. > It's important for debugging to see the actual doorbell > address. (and it seems %pK gives a hashed value and not actual value > like %lx) I can send a v2 to change the vma addresses to be printed > with %pk although they are printed in other driver (mlx) with %lx as > well. I'm sure Leon will fix mlx drivers if he is made aware of where they are doing this.. Jason -- 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
On Mon, May 14, 2018 at 03:14:11PM -0600, Jason Gunthorpe wrote: > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe <jgg@mellanox.com> > > > > > > > > 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 > > > It's important for debugging to see the actual doorbell > > address. (and it seems %pK gives a hashed value and not actual value > > like %lx) I can send a v2 to change the vma addresses to be printed > > with %pk although they are printed in other driver (mlx) with %lx as > > well. > > I'm sure Leon will fix mlx drivers if he is made aware of where they > are doing this.. Or patch which removes them. Thanks > > Jason > -- > 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
> From: Leon Romanovsky [mailto:leon@kernel.org] > Sent: Tuesday, May 15, 2018 12:09 PM > To: Jason Gunthorpe <jgg@mellanox.com> > Cc: Kalderon, Michal <Michal.Kalderon@cavium.com>; > dledford@redhat.com; linux-rdma@vger.kernel.org; Bason, Yuval > <Yuval.Bason@cavium.com>; Elior, Ariel <Ariel.Elior@cavium.com> > Subject: Re: [PATCH for-rc] RDMA/qedr: Fix doorbell bar mapping for dpi > 1 > > On Mon, May 14, 2018 at 03:14:11PM -0600, Jason Gunthorpe wrote: > > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > > From: linux-rdma-owner@vger.kernel.org > > > <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe > > > <jgg@mellanox.com> > > > > > > > > > > 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 > > > > > It's important for debugging to see the actual doorbell address. > > > (and it seems %pK gives a hashed value and not actual value like > > > %lx) I can send a v2 to change the vma addresses to be printed with > > > %pk although they are printed in other driver (mlx) with %lx as > > > well. > > > > I'm sure Leon will fix mlx drivers if he is made aware of where they > > are doing this.. > > Or patch which removes them. https://elixir.bootlin.com/linux/v4.17-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2061 https://elixir.bootlin.com/linux/v4.17-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2166 > > Thanks > > > > > Jason > > -- > > 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 -- 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
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > From: linux-rdma-owner@vger.kernel.org > > <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe > > <jgg@mellanox.com> > > > > > > > > 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 > > > > > I think PCI physical addresses are expected to be protected as well. > > > > Not sure what can be done though with this physical address, as it is > > an address into the devices doorbell -> any attempt to write data that > > is not in the doorbell format will be dropped by device. > > It obscures information about the kernel memory layout which is the point of > all of this. > > If you want to report the doorbell address for debugging then report it as an > offset from the start of the device's bar. The doorbell bar address (all bars ) is visible with a simple lspci -vv ( also when run with non-priviledged user and kptr_restrict=2) Why is reporting it in printk a leak in this case? > > > It's important for debugging to see the actual doorbell address. (and > > it seems %pK gives a hashed value and not actual value like %lx) I can > > send a v2 to change the vma addresses to be printed with %pk although > > they are printed in other driver (mlx) with %lx as well. > > I'm sure Leon will fix mlx drivers if he is made aware of where they are doing > this.. > > Jason > -- > 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 -- 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
On Mon, May 14, 2018 at 07:46:55PM +0000, Kalderon, Michal wrote: > From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Leon Romanovsky <leon@kernel.org> > Sent: Monday, May 14, 2018 11:06 AM > > > > > > > > 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. > > Modified the code as follows for comparison: > DP_DEBUG(dev, QEDR_MSG_INIT, > "mmap (pk) invoked with vm_start=0x%pk,x 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); > > DP_DEBUG(dev, QEDR_MSG_INIT, > "mmap (px) invoked with vm_start=0x%px,x vm_end=0x%px,vm_pgoff=0x%px; dpi_start=0x%px dpi_size=0x%x\n", > (void *)vma->vm_start, (void *)vma->vm_end, (void *)vma->vm_pgoff, (void *)dpi_start, > ucontext->dpi_size); > > > DP_DEBUG(dev, QEDR_MSG_INIT, > "mmap invoked with vm_start=0x%lx,x 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); > > and got the following output: > > [32937.851202] (qedr1) INIT: mmap (pk) invoked with vm_start=0x000000009dacc20a,x vm_end=0x0000000040480bf1,vm_pgoff=0x00000000d9849dd6; dpi_start=0x00000000c74fd5ea dpi_size=0x2000 > [32937.851203] (qedr1) INIT: mmap (px) invoked with vm_start=0x00007f20575ad000,x vm_end=0x00007f20575af000,vm_pgoff=0x0000000000091b03; dpi_start=0x0000000091b03000 dpi_size=0x2000 > [32937.851205] (qedr1) INIT: mmap invoked with vm_start=0x7f20575ad000,x vm_end=0x7f20575af000,vm_pgoff=0x91b03; dpi_start=0x91b03000 dpi_size=0x2000 > > looks like the %pk gives hashed results similar to the %p even if the kptr_restrict is zero. > sysctl kernel/kptr_restrict > kernel.kptr_restrict = 0 I found the reason to such behavior: commit ef0010a30935de4e0211cbc7bdffc30446cdee9b Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed Nov 29 11:28:09 2017 -0800 vsprintf: don't use 'restricted_pointer()' when not restricting Instead, just fall back on the new '%p' behavior which hashes the pointer. Otherwise, '%pK' - that was intended to mark a pointer as restricted - just ends up leaking pointers that a normal '%p' wouldn't leak. Which just make the whole thing pointless. I suspect we should actually get rid of '%pK' entirely, and make it just work as '%p' regardless, but this is the minimal obvious fix. People who actually use 'kptr_restrict' should weigh in on which behavior they want. Cc: Tobin Harding <me@tobin.cc> Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> This commit fixes the 7b1924a1d930 ("vsprintf: add printk specifier %px") Thanks > > > > Thanks > -- > 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
> From: Leon Romanovsky [mailto:leon@kernel.org] > Sent: Tuesday, May 15, 2018 2:46 PM > To: Kalderon, Michal <Michal.Kalderon@cavium.com> > Cc: jgg@mellanox.com; dledford@redhat.com; linux-rdma@vger.kernel.org; > Bason, Yuval <Yuval.Bason@cavium.com>; Elior, Ariel > <Ariel.Elior@cavium.com> > Subject: Re: [PATCH for-rc] RDMA/qedr: Fix doorbell bar mapping for dpi > 1 > > On Mon, May 14, 2018 at 07:46:55PM +0000, Kalderon, Michal wrote: > > From: linux-rdma-owner@vger.kernel.org > > <linux-rdma-owner@vger.kernel.org> on behalf of Leon Romanovsky > > <leon@kernel.org> > > Sent: Monday, May 14, 2018 11:06 AM > > > > > > > > > > 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. > > > > Modified the code as follows for comparison: > > DP_DEBUG(dev, QEDR_MSG_INIT, > > "mmap (pk) invoked with vm_start=0x%pk,x > 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); > > > > DP_DEBUG(dev, QEDR_MSG_INIT, > > "mmap (px) invoked with vm_start=0x%px,x > vm_end=0x%px,vm_pgoff=0x%px; dpi_start=0x%px dpi_size=0x%x\n", > > (void *)vma->vm_start, (void *)vma->vm_end, (void *)vma- > >vm_pgoff, (void *)dpi_start, > > ucontext->dpi_size); > > > > > > DP_DEBUG(dev, QEDR_MSG_INIT, > > "mmap invoked with vm_start=0x%lx,x > 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); > > > > and got the following output: > > > > [32937.851202] (qedr1) INIT: mmap (pk) invoked with > > vm_start=0x000000009dacc20a,x > > vm_end=0x0000000040480bf1,vm_pgoff=0x00000000d9849dd6; > > dpi_start=0x00000000c74fd5ea dpi_size=0x2000 [32937.851203] (qedr1) > > INIT: mmap (px) invoked with vm_start=0x00007f20575ad000,x > > vm_end=0x00007f20575af000,vm_pgoff=0x0000000000091b03; > > dpi_start=0x0000000091b03000 dpi_size=0x2000 [32937.851205] (qedr1) > > INIT: mmap invoked with vm_start=0x7f20575ad000,x > > vm_end=0x7f20575af000,vm_pgoff=0x91b03; dpi_start=0x91b03000 > > dpi_size=0x2000 > > > > looks like the %pk gives hashed results similar to the %p even if the > kptr_restrict is zero. > > sysctl kernel/kptr_restrict > > kernel.kptr_restrict = 0 > > I found the reason to such behavior: > commit ef0010a30935de4e0211cbc7bdffc30446cdee9b > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Wed Nov 29 11:28:09 2017 -0800 > > vsprintf: don't use 'restricted_pointer()' when not restricting > > Instead, just fall back on the new '%p' behavior which hashes the > pointer. > > Otherwise, '%pK' - that was intended to mark a pointer as restricted - > just ends up leaking pointers that a normal '%p' wouldn't leak. Which > just make the whole thing pointless. > > I suspect we should actually get rid of '%pK' entirely, and make it just > work as '%p' regardless, but this is the minimal obvious fix. People > who actually use 'kptr_restrict' should weigh in on which behavior they > want. > > Cc: Tobin Harding <me@tobin.cc> > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > > This commit fixes the 7b1924a1d930 ("vsprintf: add printk specifier %px") Thanks Leon, was just looking at this fix and realized I had to set kptr_restrict to 1 to get actual address. V2 on it's way. > > Thanks > > > > > > > > Thanks > > -- > > 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 -- 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
On Tue, May 15, 2018 at 10:46:30AM +0000, Kalderon, Michal wrote: > > From: Leon Romanovsky [mailto:leon@kernel.org] > > Sent: Tuesday, May 15, 2018 12:09 PM > > To: Jason Gunthorpe <jgg@mellanox.com> > > Cc: Kalderon, Michal <Michal.Kalderon@cavium.com>; > > dledford@redhat.com; linux-rdma@vger.kernel.org; Bason, Yuval > > <Yuval.Bason@cavium.com>; Elior, Ariel <Ariel.Elior@cavium.com> > > Subject: Re: [PATCH for-rc] RDMA/qedr: Fix doorbell bar mapping for dpi > 1 > > > > On Mon, May 14, 2018 at 03:14:11PM -0600, Jason Gunthorpe wrote: > > > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > > > From: linux-rdma-owner@vger.kernel.org > > > > <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe > > > > <jgg@mellanox.com> > > > > > > > > > > > > 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 > > > > > > > It's important for debugging to see the actual doorbell address. > > > > (and it seems %pK gives a hashed value and not actual value like > > > > %lx) I can send a v2 to change the vma addresses to be printed with > > > > %pk although they are printed in other driver (mlx) with %lx as > > > > well. > > > > > > I'm sure Leon will fix mlx drivers if he is made aware of where they > > > are doing this.. > > > > Or patch which removes them. > https://elixir.bootlin.com/linux/v4.17-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2061 > https://elixir.bootlin.com/linux/v4.17-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2166 > https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/commit/?h=rdma-next&id=f8636e5ef120b01c26aab893cd8bf387cd4ef997 > > > > > Thanks > > > > > > > > Jason > > > -- > > > 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 > -- > 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
On Tue, May 15, 2018 at 11:06:21AM +0000, Kalderon, Michal wrote: > > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > > From: linux-rdma-owner@vger.kernel.org > > > <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe > > > <jgg@mellanox.com> > > > > > > > > > > 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 > > > > > > > I think PCI physical addresses are expected to be protected as well. > > > > > > Not sure what can be done though with this physical address, as it is > > > an address into the devices doorbell -> any attempt to write data that > > > is not in the doorbell format will be dropped by device. > > > > It obscures information about the kernel memory layout which is the point of > > all of this. > > > > If you want to report the doorbell address for debugging then report it as an > > offset from the start of the device's bar. > > The doorbell bar address (all bars ) is visible with a simple lspci -vv ( also when run > with non-priviledged user and kptr_restrict=2) Why is reporting it in printk > a leak in this case? I didn't make feature.. But if I recall, lspci -vv only shows the bar physical address because your system is not in a high enough security mode? Jason -- 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
From: Jason Gunthorpe <jgg@mellanox.com> Sent: Tuesday, May 15, 2018 5:22 PM > On Tue, May 15, 2018 at 11:06:21AM +0000, Kalderon, Michal wrote: > > > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > > > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > > > On Mon, May 14, 2018 at 08:01:32PM +0000, Kalderon, Michal wrote: > > > > From: linux-rdma-owner@vger.kernel.org > > > > <linux-rdma-owner@vger.kernel.org> on behalf of Jason Gunthorpe > > > > <jgg@mellanox.com> > > > > > > > > > > > > 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 > > > > > > > > > I think PCI physical addresses are expected to be protected as well. > > > > > > > > Not sure what can be done though with this physical address, as it is > > > > an address into the devices doorbell -> any attempt to write data that > > > > is not in the doorbell format will be dropped by device. > > > > > > It obscures information about the kernel memory layout which is the point of > > > all of this. > > > > > > If you want to report the doorbell address for debugging then report it as an > > > offset from the start of the device's bar. > > > > The doorbell bar address (all bars ) is visible with a simple lspci -vv ( also when run > > with non-priviledged user and kptr_restrict=2) Why is reporting it in printk > > a leak in this case? > > I didn't make feature.. > > But if I recall, lspci -vv only shows the bar physical address because > your system is not in a high enough security mode? > I'll check again, thanks. In any case, following discussion with Leon I saw that using pK does give the same information needed, sent a v2 with the fix. thanks, Michal > > Jason -- 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..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,
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> --- drivers/infiniband/hw/qedr/verbs.c | 59 ++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 31 deletions(-)