On Thu, Feb 14, 2019 at 9:49 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Thu, Feb 14, 2019 at 09:16:50PM +0530, Devesh Sharma wrote:
>
> > > All the cases that touch pg_arr seem to want to touch the
> > > dma_alloc_coherent memory, not the user pages, so setting it to NULL
> > > for the usage pages seems like the right thing to do?
> >
> > I agree that in user-space object case setting pg_arr to NULL should be okay.
> > However the driver is broken in current condition. It immediately
> > requires additional patch to restrict unconditional access
> > of pg_arr data structure when the object belong to user-space. The fix
> > would be to put check if any object belongs to user space then
> > skip calling the functions which require pg_arr. i.e. __clean_cq() etc.
>
> This is completly wrong, you can't just call sg_virt on user memory
> then use the pointer and hope for the best. That breaks so many valid
> flows in the kernel.
>
> Look you guys need to review patches on your driver, Shiraz had posted
> this stuff multiple times over months.
>
> Send a patch to fix this mess and don't mis-use the SG & DMA API.
Yes, that will be done, we will supply an immediate fix so that the
broken driver comes to working condition without crashing
and then put in clean-up stuff.
>
> This should probably fix things in the short term. Please test it:
>
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> index d08b9d9948fd3c..42838c4dab4c92 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> @@ -118,9 +118,13 @@ static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
> is_umem = true;
> for_each_sg_dma_page (sghead, &sg_iter, pages, 0) {
> pbl->pg_map_arr[i] = sg_page_iter_dma_address(&sg_iter);
> - pbl->pg_arr[i] = NULL;
> - if (!pbl->pg_arr[i])
> - goto fail;
> +
> + /*
> + * FIXME: The driver should not be trying to get
> + * kernel access to user pages in this way.
> + */
> + pbl->pg_arr[i] =
> + page_address(sg_page_iter_page(&sg_iter.base));
>
> i++;
> pbl->pg_count++;
