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.
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++;
