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

[Qemu-devel] [PATCH v1 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 v1 of the earlier patch set to fix issues
in the rdma/pvrdma backend.

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

Please note, this patch is created after merging another patch-set from

  -> https://patchwork.kernel.org/patch/10705439/
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02344.html

And local update to create qapi/qapi-events-rdma.h, to fix compiler errors.

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
  pvrdma: check return value from pvrdma_idx_ring_has_ routines
  rdma: remove unused VENDOR_ERR_NO_SGE macro

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

-- 
2.19.2

[Qemu-devel] [PATCH v1 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 v1: use new error code VENDOR_ERR_INV_NUM_SGE
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02220.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 v1 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(+)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 23dc9926e3..8a03ab4669 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 0;
+}
+
 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 v1 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 v1: move check before page dir/tbl map
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02257.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 v1 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 v1: define new function to free PvrdmaRing object
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02328.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 v1 5/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 | 37 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

Update v1: receive error code in idx variable, remove comments
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02342.html

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index 01247fc041..2dccac8442 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -73,23 +73,22 @@ out:
 
 void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
 {
-    unsigned int idx = 0, offset;
+    int idx;
+    unsigned int offset, head;
 
-    /*
-    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)) {
+    idx = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &head);
+    if (idx <= 0) {
         pr_dbg("No more data in ring\n");
         return NULL;
     }
 
+    idx = pvrdma_idx(&ring->ring_state->cons_head, ring->max_elems);
+    if (idx < 0 || head != idx) {
+        pr_dbg("invalid idx\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 +104,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

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

[P J P]

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

Replace VENDOR_ERR_NO_SGE macro with VENDOR_ERR_INV_NUM_SGE
to indicate invalid number of scatter/gather elements.

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

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

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 05:17:21PM +0530, P J P wrote:
> 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 v1: use new error code VENDOR_ERR_INV_NUM_SGE
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02220.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;
>      }

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

>  
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 05:17:23PM +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 v1: move check before page dir/tbl map
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02257.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) {

Thanks.

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

> -- 
> 2.19.2
> 

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

[Marcel Apfelbaum]

Hi Prasad,


On 12/12/18 1:47 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(+)
>
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 23dc9926e3..8a03ab4669 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 0;

The PCI read operation should fail if the read op is  not implemented.
Please use  "return 0xffffffff" to emulate the PCI read failure.
(0 is a successfully read)

Thanks,
Marcel

> +}
> +
>   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 = {

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 05:17:24PM +0530, P J P wrote:
> 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 v1: define new function to free PvrdmaRing object
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02328.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);
>  

Thanks.

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

>      return 0;
>  }
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 05:17:26PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Replace VENDOR_ERR_NO_SGE macro with VENDOR_ERR_INV_NUM_SGE
> to indicate invalid number of scatter/gather elements.

Suggesting a revised commit message such as:
With commit xxxx ("pvrdma: check number of pages when creating rings")
VENDOR_ERR_NO_SGE is no longer in use - delete it.

(kinda ambiguity if subject says "remove" while commit message says
"replace")

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

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 07:23:05PM +0200, Yuval Shaia wrote:
> On Wed, Dec 12, 2018 at 05:17:26PM +0530, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> > 
> > Replace VENDOR_ERR_NO_SGE macro with VENDOR_ERR_INV_NUM_SGE
> > to indicate invalid number of scatter/gather elements.
> 
> Suggesting a revised commit message such as:
> With commit xxxx ("pvrdma: check number of pages when creating rings")
> VENDOR_ERR_NO_SGE is no longer in use - delete it.

Correction, commit is ("rdma: check num_sge does not exceed MAX_SGE")

> 
> (kinda ambiguity if subject says "remove" while commit message says
> "replace")
> 
> > 
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/rdma/rdma_backend.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > 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
> > 

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 05:17:25PM +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 | 37 +++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> Update v1: receive error code in idx variable, remove comments
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02342.html
> 
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
> index 01247fc041..2dccac8442 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -73,23 +73,22 @@ out:
>  
>  void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
>  {
> -    unsigned int idx = 0, offset;
> +    int idx;
> +    unsigned int offset, head;
>  
> -    /*
> -    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)) {
> +    idx = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &head);
> +    if (idx <= 0) {
>          pr_dbg("No more data in ring\n");
>          return NULL;
>      }
>  
> +    idx = pvrdma_idx(&ring->ring_state->cons_head, ring->max_elems);
> +    if (idx < 0 || head != idx) {
> +        pr_dbg("invalid idx\n");
> +        return NULL;
> +    }
> +

Sorry, i mislead you here, idx was already populated by
pvrdma_idx_ring_has_data so call to pvrdma_idx just to retrieve the index
is waste. Please revert back to using another variable to check the return
value from pvrdma_idx_ring_has_data and drop this call to pvrdma_idx.

>      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 +104,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
>