[Qemu-devel] [PATCH v2 0/6] rdma: various issues in rdma/pvrdma backend

[Qemu-devel] [PATCH v2 0/6] rdma: various issues in rdma/pvrdma backend

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

This is a revised version v2 of the earlier patch set to fix issues
in the rdma/pvrdma backend.

Update to include review comments from
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02616.html

Thank you.
---
Prasad J Pandit (6):
  rdma: check num_sge does not exceed MAX_SGE
  pvrdma: add uar_read routine
  pvrdma: check number of pages when creating rings
  pvrdma: release ring object in case of an error
  rdma: remove unused VENDOR_ERR_NO_SGE macro
  pvrdma: check return value from pvrdma_idx_ring_has_ routines

 hw/rdma/rdma_backend.c        | 15 ++++++-----
 hw/rdma/vmw/pvrdma_cmd.c      | 47 +++++++++++++++++++++++++++--------
 hw/rdma/vmw/pvrdma_dev_ring.c | 29 ++++++++-------------
 hw/rdma/vmw/pvrdma_main.c     |  6 +++++
 4 files changed, 60 insertions(+), 37 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v2 1/6] rdma: check num_sge does not exceed MAX_SGE

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

rdma back-end has scatter/gather array ibv_sge[MAX_SGE=4] set
to have 4 elements. A guest could send a 'PvrdmaSqWqe' ring element
with 'num_sge' set to > MAX_SGE, which may lead to OOB access issue.
Add check to avoid it.

Reported-by: Saar Amar <saaramar5@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/rdma_backend.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Update: No change, ack'd v1
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02783.html

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index ae1e4dcb29..bd4710d16f 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -476,9 +476,9 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
     }
 
     pr_dbg("num_sge=%d\n", num_sge);
-    if (!num_sge) {
-        pr_dbg("num_sge=0\n");
-        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);
+    if (!num_sge || num_sge > MAX_SGE) {
+        pr_dbg("invalid num_sge=%d\n", num_sge);
+        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_INV_NUM_SGE, ctx);
         return;
     }
 
@@ -603,9 +603,9 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
     }
 
     pr_dbg("num_sge=%d\n", num_sge);
-    if (!num_sge) {
-        pr_dbg("num_sge=0\n");
-        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);
+    if (!num_sge || num_sge > MAX_SGE) {
+        pr_dbg("invalid num_sge=%d\n", num_sge);
+        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_INV_NUM_SGE, ctx);
         return;
     }
 
-- 
2.19.2

[Qemu-devel] [PATCH v2 2/6] pvrdma: add uar_read routine

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Define skeleton 'uar_read' routine. Avoid NULL dereference.

Reported-by: Li Qiang <liq3ea@163.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/vmw/pvrdma_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

Update: change return value from uar_read()
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 23dc9926e3..997d7f395a 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -448,6 +448,11 @@ static const MemoryRegionOps regs_ops = {
     },
 };
 
+static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0xffffffff;
+}
+
 static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
     PVRDMADev *dev = opaque;
@@ -489,6 +494,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 }
 
 static const MemoryRegionOps uar_ops = {
+    .read = uar_read,
     .write = uar_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-- 
2.19.2

[Qemu-devel] [PATCH v2 3/6] pvrdma: check number of pages when creating rings

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

When creating CQ/QP rings, an object can have up to
PVRDMA_MAX_FAST_REG_PAGES=128 pages. Check 'npages' parameter
to avoid excessive memory allocation or a null dereference.

Reported-by: Li Qiang <liq3ea@163.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/vmw/pvrdma_cmd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Update: No change, ack'd v1
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02786.html

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index 4f616d4177..e37fb18280 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -259,6 +259,11 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
     int rc = -EINVAL;
     char ring_name[MAX_RING_NAME_SZ];
 
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid nchunks: %d\n", nchunks);
+        return rc;
+    }
+
     pr_dbg("pdir_dma=0x%llx\n", (long long unsigned int)pdir_dma);
     dir = rdma_pci_dma_map(pci_dev, pdir_dma, TARGET_PAGE_SIZE);
     if (!dir) {
@@ -371,6 +376,12 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t pdir_dma,
     char ring_name[MAX_RING_NAME_SZ];
     uint32_t wqe_sz;
 
+    if (!spages || spages > PVRDMA_MAX_FAST_REG_PAGES
+        || !rpages || rpages > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid pages: %d, %d\n", spages, rpages);
+        return rc;
+    }
+
     pr_dbg("pdir_dma=0x%llx\n", (long long unsigned int)pdir_dma);
     dir = rdma_pci_dma_map(pci_dev, pdir_dma, TARGET_PAGE_SIZE);
     if (!dir) {
-- 
2.19.2

[Qemu-devel] [PATCH v2 4/6] pvrdma: release ring object in case of an error

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

create_cq and create_qp routines allocate ring object, but it's
not released in case of an error, leading to memory leakage.

Reported-by: Li Qiang <liq3ea@163.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/vmw/pvrdma_cmd.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Update: No change, ack'd v1
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02789.html

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index e37fb18280..7e29607d2f 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -313,6 +313,14 @@ out:
     return rc;
 }
 
+static void destroy_cq_ring(PvrdmaRing *ring)
+{
+    pvrdma_ring_free(ring);
+    /* ring_state was in slot 1, not 0 so need to jump back */
+    rdma_pci_dma_unmap(ring->dev, --ring->ring_state, TARGET_PAGE_SIZE);
+    g_free(ring);
+}
+
 static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
                      union pvrdma_cmd_resp *rsp)
 {
@@ -335,6 +343,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
 
     rc = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev, cmd->cqe,
                           &resp->cq_handle, ring);
+    if (rc) {
+        destroy_cq_ring(ring);
+    }
 
     return rc;
 }
@@ -355,10 +366,7 @@ static int destroy_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
     }
 
     ring = (PvrdmaRing *)cq->opaque;
-    pvrdma_ring_free(ring);
-    /* ring_state was in slot 1, not 0 so need to jump back */
-    rdma_pci_dma_unmap(PCI_DEVICE(dev), --ring->ring_state, TARGET_PAGE_SIZE);
-    g_free(ring);
+    destroy_cq_ring(ring);
 
     rdma_rm_dealloc_cq(&dev->rdma_dev_res, cmd->cq_handle);
 
@@ -456,6 +464,17 @@ out:
     return rc;
 }
 
+static void destroy_qp_rings(PvrdmaRing *ring)
+{
+    pr_dbg("sring=%p\n", &ring[0]);
+    pvrdma_ring_free(&ring[0]);
+    pr_dbg("rring=%p\n", &ring[1]);
+    pvrdma_ring_free(&ring[1]);
+
+    rdma_pci_dma_unmap(ring->dev, ring->ring_state, TARGET_PAGE_SIZE);
+    g_free(ring);
+}
+
 static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                      union pvrdma_cmd_resp *rsp)
 {
@@ -485,6 +504,7 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                           cmd->max_recv_sge, cmd->recv_cq_handle, rings,
                           &resp->qpn);
     if (rc) {
+        destroy_qp_rings(rings);
         return rc;
     }
 
@@ -557,13 +577,7 @@ static int destroy_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
     rdma_rm_dealloc_qp(&dev->rdma_dev_res, cmd->qp_handle);
 
     ring = (PvrdmaRing *)qp->opaque;
-    pr_dbg("sring=%p\n", &ring[0]);
-    pvrdma_ring_free(&ring[0]);
-    pr_dbg("rring=%p\n", &ring[1]);
-    pvrdma_ring_free(&ring[1]);
-
-    rdma_pci_dma_unmap(PCI_DEVICE(dev), ring->ring_state, TARGET_PAGE_SIZE);
-    g_free(ring);
+    destroy_qp_rings(ring);
 
     return 0;
 }
-- 
2.19.2

[Qemu-devel] [PATCH v2 5/6] rdma: remove unused VENDOR_ERR_NO_SGE macro

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

With commit 4481985c (rdma: check num_sge does not exceed MAX_SGE)
macro VENDOR_ERR_NO_SGE is no longer in use - delete it.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/rdma_backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Update: change commit log message
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02793.html

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index bd4710d16f..c28bfbd44d 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -37,12 +37,11 @@
 #define VENDOR_ERR_TOO_MANY_SGES    0x202
 #define VENDOR_ERR_NOMEM            0x203
 #define VENDOR_ERR_QP0              0x204
-#define VENDOR_ERR_NO_SGE           0x205
+#define VENDOR_ERR_INV_NUM_SGE      0x205
 #define VENDOR_ERR_MAD_SEND         0x206
 #define VENDOR_ERR_INVLKEY          0x207
 #define VENDOR_ERR_MR_SMALL         0x208
 #define VENDOR_ERR_INV_MAD_BUFF     0x209
-#define VENDOR_ERR_INV_NUM_SGE      0x210
 
 #define THR_NAME_LEN 16
 #define THR_POLL_TO  5000
-- 
2.19.2

[Qemu-devel] [PATCH v2 6/6] pvrdma: check return value from pvrdma_idx_ring_has_ routines

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

pvrdma_idx_ring_has_[data/space] routines also return invalid
index PVRDMA_INVALID_IDX[=-1], if ring has no data/space. Check
return value from these routines to avoid plausible infinite loops.

Reported-by: Li Qiang <liq3ea@163.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/rdma/vmw/pvrdma_dev_ring.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

Update: revert use of idx variable in pvrdma_ring_next_elem_read()
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02814.html

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index 01247fc041..e8e5b502f6 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -73,23 +73,16 @@ out:
 
 void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
 {
+    int e;
     unsigned int idx = 0, offset;
 
-    /*
-    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
-           ring->ring_state->cons_head);
-    */
-
-    if (!pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx)) {
+    e = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx);
+    if (e <= 0) {
         pr_dbg("No more data in ring\n");
         return NULL;
     }
 
     offset = idx * ring->elem_sz;
-    /*
-    pr_dbg("idx=%d\n", idx);
-    pr_dbg("offset=%d\n", offset);
-    */
     return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);
 }
 
@@ -105,20 +98,20 @@ void pvrdma_ring_read_inc(PvrdmaRing *ring)
 
 void *pvrdma_ring_next_elem_write(PvrdmaRing *ring)
 {
-    unsigned int idx, offset, tail;
+    int idx;
+    unsigned int offset, tail;
 
-    /*
-    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
-           ring->ring_state->cons_head);
-    */
-
-    if (!pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail)) {
+    idx = pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail);
+    if (idx <= 0) {
         pr_dbg("CQ is full\n");
         return NULL;
     }
 
     idx = pvrdma_idx(&ring->ring_state->prod_tail, ring->max_elems);
-    /* TODO: tail == idx */
+    if (idx < 0 || tail != idx) {
+        pr_dbg("invalid idx\n");
+        return NULL;
+    }
 
     offset = idx * ring->elem_sz;
     return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);
-- 
2.19.2

Re: [Qemu-devel] [PATCH v2 5/6] rdma: remove unused VENDOR_ERR_NO_SGE macro

[Yuval Shaia]

On Thu, Dec 13, 2018 at 01:00:38AM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> With commit 4481985c (rdma: check num_sge does not exceed MAX_SGE)
> macro VENDOR_ERR_NO_SGE is no longer in use - delete it.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/rdma/rdma_backend.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Update: change commit log message
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02793.html
> 
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index bd4710d16f..c28bfbd44d 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -37,12 +37,11 @@
>  #define VENDOR_ERR_TOO_MANY_SGES    0x202
>  #define VENDOR_ERR_NOMEM            0x203
>  #define VENDOR_ERR_QP0              0x204
> -#define VENDOR_ERR_NO_SGE           0x205
> +#define VENDOR_ERR_INV_NUM_SGE      0x205
>  #define VENDOR_ERR_MAD_SEND         0x206
>  #define VENDOR_ERR_INVLKEY          0x207
>  #define VENDOR_ERR_MR_SMALL         0x208
>  #define VENDOR_ERR_INV_MAD_BUFF     0x209
> -#define VENDOR_ERR_INV_NUM_SGE      0x210
>  
>  #define THR_NAME_LEN 16
>  #define THR_POLL_TO  5000

Thanks.

Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

> -- 
> 2.19.2
> 

Re: [Qemu-devel] [PATCH v2 6/6] pvrdma: check return value from pvrdma_idx_ring_has_ routines

[Yuval Shaia]

On Thu, Dec 13, 2018 at 01:00:39AM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> pvrdma_idx_ring_has_[data/space] routines also return invalid
> index PVRDMA_INVALID_IDX[=-1], if ring has no data/space. Check
> return value from these routines to avoid plausible infinite loops.
> 
> Reported-by: Li Qiang <liq3ea@163.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/rdma/vmw/pvrdma_dev_ring.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> Update: revert use of idx variable in pvrdma_ring_next_elem_read()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02814.html
> 
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
> index 01247fc041..e8e5b502f6 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -73,23 +73,16 @@ out:
>  
>  void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
>  {
> +    int e;
>      unsigned int idx = 0, offset;
>  
> -    /*
> -    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
> -           ring->ring_state->cons_head);
> -    */
> -
> -    if (!pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx)) {
> +    e = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx);
> +    if (e <= 0) {
>          pr_dbg("No more data in ring\n");
>          return NULL;
>      }
>  
>      offset = idx * ring->elem_sz;
> -    /*
> -    pr_dbg("idx=%d\n", idx);
> -    pr_dbg("offset=%d\n", offset);
> -    */
>      return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);
>  }
>  
> @@ -105,20 +98,20 @@ void pvrdma_ring_read_inc(PvrdmaRing *ring)
>  
>  void *pvrdma_ring_next_elem_write(PvrdmaRing *ring)
>  {
> -    unsigned int idx, offset, tail;
> +    int idx;
> +    unsigned int offset, tail;
>  
> -    /*
> -    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
> -           ring->ring_state->cons_head);
> -    */
> -
> -    if (!pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail)) {
> +    idx = pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail);
> +    if (idx <= 0) {
>          pr_dbg("CQ is full\n");
>          return NULL;
>      }
>  
>      idx = pvrdma_idx(&ring->ring_state->prod_tail, ring->max_elems);
> -    /* TODO: tail == idx */
> +    if (idx < 0 || tail != idx) {
> +        pr_dbg("invalid idx\n");
> +        return NULL;
> +    }
>  
>      offset = idx * ring->elem_sz;
>      return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);

Thanks.

Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

> -- 
> 2.19.2
> 

Re: [Qemu-devel] [PATCH v2 2/6] pvrdma: add uar_read routine

[Marcel Apfelbaum]

On 12/12/18 9:30 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Define skeleton 'uar_read' routine. Avoid NULL dereference.
>
> Reported-by: Li Qiang <liq3ea@163.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/rdma/vmw/pvrdma_main.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> Update: change return value from uar_read()
>    -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html
>
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 23dc9926e3..997d7f395a 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -448,6 +448,11 @@ static const MemoryRegionOps regs_ops = {
>       },
>   };
>   
> +static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return 0xffffffff;
> +}
> +
>   static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>   {
>       PVRDMADev *dev = opaque;
> @@ -489,6 +494,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>   }
>   
>   static const MemoryRegionOps uar_ops = {
> +    .read = uar_read,
>       .write = uar_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .impl = {


Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 3/6] pvrdma: check number of pages when creating rings

[Yuval Shaia]

Hi Prasad,
Turned out that this patch cause a regression.

My test plan includes the following steps:
- Start two VMs.
- Run RC and UD traffic between the two.
- Run sanity local test on both which includes:
	- RC traffic on 3 gids with various message size.
	- UD traffic.
	- RDMA-CM connection with MAD.
	- MPI test.
- Power off the two VMs.

With this patch the last step fails, the guest OS hangs, trying to probably
unload pvrdma driver and finally gave up after 3 minutes.

On its face this patch does not seems to be related to the problem above
but fact is a fact, without this patch VM goes down with no issues. The
only thing i can think of is that somehow the guest driver does not capture
the error or does not handles the error correctly.

Anyways with debug turned on i have noticed that there is one case that
devices gets 129 nchunks (i think in MPI) while your patch limits it to
128.
>From pvrdma source code  we can see that first page is dedicated to ring
state, this means that it maybe correct that 128 is the limit but we
should check that nchunks does not exceed 129, not 128.

What do you think?

Ie. to replace this line from create_cq_ring
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
with this
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES + 1) {

Let me know your opinion.
I can make a quick fix to your patch or send a new patch on top of yours
for a review.

Yuval

On Thu, Dec 13, 2018 at 01:00:36AM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> When creating CQ/QP rings, an object can have up to
> PVRDMA_MAX_FAST_REG_PAGES=128 pages. Check 'npages' parameter
> to avoid excessive memory allocation or a null dereference.
> 
> Reported-by: Li Qiang <liq3ea@163.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/rdma/vmw/pvrdma_cmd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Update: No change, ack'd v1
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02786.html
> 
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index 4f616d4177..e37fb18280 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -259,6 +259,11 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
>      int rc = -EINVAL;
>      char ring_name[MAX_RING_NAME_SZ];
>  
> +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
> +        pr_dbg("invalid nchunks: %d\n", nchunks);
> +        return rc;
> +    }
> +
>      pr_dbg("pdir_dma=0x%llx\n", (long long unsigned int)pdir_dma);
>      dir = rdma_pci_dma_map(pci_dev, pdir_dma, TARGET_PAGE_SIZE);
>      if (!dir) {
> @@ -371,6 +376,12 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t pdir_dma,
>      char ring_name[MAX_RING_NAME_SZ];
>      uint32_t wqe_sz;
>  
> +    if (!spages || spages > PVRDMA_MAX_FAST_REG_PAGES
> +        || !rpages || rpages > PVRDMA_MAX_FAST_REG_PAGES) {
> +        pr_dbg("invalid pages: %d, %d\n", spages, rpages);
> +        return rc;
> +    }
> +
>      pr_dbg("pdir_dma=0x%llx\n", (long long unsigned int)pdir_dma);
>      dir = rdma_pci_dma_map(pci_dev, pdir_dma, TARGET_PAGE_SIZE);
>      if (!dir) {
> -- 
> 2.19.2
> 

Re: [Qemu-devel] [PATCH v2 3/6] pvrdma: check number of pages when creating rings

[P J P]

  Hello Yuval,

+-- On Sun, 16 Dec 2018, Yuval Shaia wrote --+
| With this patch the last step fails, the guest OS hangs, trying to probably 
| unload pvrdma driver and finally gave up after 3 minutes.

Strange...
 
| Anyways with debug turned on i have noticed that there is one case that 
| devices gets 129 nchunks (i think in MPI) while your patch limits it to 128.
| >From pvrdma source code  we can see that first page is dedicated to ring
| state, this means that it maybe correct that 128 is the limit but we
| should check that nchunks does not exceed 129, not 128.
| 
| What do you think?

 -> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/vmw_pvrdma/pvrdma_mr.c?id=fdf82a7856b32d905c39afc85e34364491e46346#n201

the vmw_pvrdma kernel driver also seems to set MAX_FAST_REG_PAGE = 128.


| Ie. to replace this line from create_cq_ring
| +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
| with this
| +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES + 1) {
| 
| Let me know your opinion.

While it may help to fix the regression. I'm not sure it's a right fix.
129 seems a little odd number to have as limit.

Is it possible MPI is erring in getting 129 chunks?

IMO it's better to confirm the right value for 'MAX_FAST_REG_PAGES', before 
going with > PVRDMA_MAX_FAS_REG_PAGES(=128) + 1.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 3/6] pvrdma: check number of pages when creating rings

[Yuval Shaia]

On Tue, Dec 18, 2018 at 12:17:59AM +0530, P J P wrote:
>   Hello Yuval,
> 
> +-- On Sun, 16 Dec 2018, Yuval Shaia wrote --+
> | With this patch the last step fails, the guest OS hangs, trying to probably 
> | unload pvrdma driver and finally gave up after 3 minutes.
> 
> Strange...
>  
> | Anyways with debug turned on i have noticed that there is one case that 
> | devices gets 129 nchunks (i think in MPI) while your patch limits it to 128.
> | >From pvrdma source code  we can see that first page is dedicated to ring
> | state, this means that it maybe correct that 128 is the limit but we
> | should check that nchunks does not exceed 129, not 128.
> | 
> | What do you think?
> 
>  -> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/vmw_pvrdma/pvrdma_mr.c?id=fdf82a7856b32d905c39afc85e34364491e46346#n201
> 
> the vmw_pvrdma kernel driver also seems to set MAX_FAST_REG_PAGE = 128.

So does the user-space library.
Maybe the mr_type is IB_MR_TYPE_MEM_REG.

> 
> 
> | Ie. to replace this line from create_cq_ring
> | +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
> | with this
> | +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES + 1) {
> | 
> | Let me know your opinion.
> 
> While it may help to fix the regression. I'm not sure it's a right fix.
> 129 seems a little odd number to have as limit.

Agree, let's stick with this patch.

> 
> Is it possible MPI is erring in getting 129 chunks?

Yeah but still the driver is holding the shutdown, not MPI.

Anyways, I found a wrong setting of respose to driver in "Add support for
RDMA MAD" patchset v6 and fixed that.
Now the regression is fine, i.e. VM goes down smoothly.

> 
> IMO it's better to confirm the right value for 'MAX_FAST_REG_PAGES', before 
> going with > PVRDMA_MAX_FAS_REG_PAGES(=128) + 1.

Agree.

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F