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

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

[P J P]

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

Hello,

Various issues OOB access, null dereference and possible infinite loop were
reported in the rdma/pvrdma backends. This patch set attempts to fix these.

Thank you.
---
Prasad J Pandit (5):
  rdma: check that 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

 hw/rdma/rdma_backend.c        |  8 ++++----
 hw/rdma/vmw/pvrdma_cmd.c      | 17 ++++++++++++++++-
 hw/rdma/vmw/pvrdma_dev_ring.c | 16 +++++++++++-----
 hw/rdma/vmw/pvrdma_main.c     |  6 ++++++
 4 files changed, 37 insertions(+), 10 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH 1/5] rdma: check that 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index d7a4bbd91f..0b3b98a94c 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -311,8 +311,8 @@ 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");
+    if (!num_sge || num_sge > MAX_SGE) {
+        pr_dbg("invalid num_sge=%d\n", num_sge);
         comp_handler(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);
         return;
     }
@@ -390,8 +390,8 @@ 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");
+    if (!num_sge || num_sge > MAX_SGE) {
+        pr_dbg("invalid num_sge=%d\n", num_sge);
         comp_handler(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);
         return;
     }
-- 
2.19.2

[Qemu-devel] [PATCH 2/5] 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 ca5fa8d981..a6211d416d 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -455,6 +455,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;
@@ -496,6 +501,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 3/5] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index 4faeb21631..ee2888259c 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -273,6 +273,10 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
         pr_dbg("Failed to map to CQ page table\n");
         goto out;
     }
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid nchunks: %d\n", nchunks);
+        goto out;
+    }
 
     r = g_malloc(sizeof(*r));
     *ring = r;
@@ -389,6 +393,11 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t pdir_dma,
         pr_dbg("Failed to map to CQ page table\n");
         goto out;
     }
+    if (!spages || spages > PVRDMA_MAX_FAST_REG_PAGES
+        || !rpages || rpages > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid pages: %d, %d\n", spages, rpages);
+        goto out;
+    }
 
     sr = g_malloc(2 * sizeof(*rr));
     rr = &sr[1];
-- 
2.19.2

[Qemu-devel] [PATCH 4/5] 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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index ee2888259c..e8d99f29fa 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
 
     resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev,
                                      cmd->cqe, &resp->cq_handle, ring);
-    resp->cqe = cmd->cqe;
+    if (resp->hdr.err) {
+        g_free(ring);
+    }
 
 out:
     pr_dbg("ret=%d\n", resp->hdr.err);
@@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                                      cmd->max_send_sge, cmd->send_cq_handle,
                                      cmd->max_recv_wr, cmd->max_recv_sge,
                                      cmd->recv_cq_handle, rings, &resp->qpn);
+    if (resp->hdr.err) {
+        g_free(rings);
+        goto out;
+    }
 
     resp->max_send_wr = cmd->max_send_wr;
     resp->max_recv_wr = cmd->max_recv_wr;
-- 
2.19.2

[Qemu-devel] [PATCH 5/5] 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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index 01247fc041..61b6a0a869 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -73,6 +73,7 @@ out:
 
 void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
 {
+    int e;
     unsigned int idx = 0, offset;
 
     /*
@@ -80,7 +81,8 @@ void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
            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;
     }
@@ -105,20 +107,24 @@ 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 1/5] rdma: check that num_sge does not exceed MAX_SGE

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:56:38PM +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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index d7a4bbd91f..0b3b98a94c 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -311,8 +311,8 @@ 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");
> +    if (!num_sge || num_sge > MAX_SGE) {
> +        pr_dbg("invalid num_sge=%d\n", num_sge);
>          comp_handler(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);

Please use VENDOR_ERR_INV_NUM_SGE

>          return;
>      }
> @@ -390,8 +390,8 @@ 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");
> +    if (!num_sge || num_sge > MAX_SGE) {
> +        pr_dbg("invalid num_sge=%d\n", num_sge);
>          comp_handler(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);

Ditto.

And since VENDOR_ERR_NO_SGE is no loger used it can be delete.

>          return;
>      }
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:56:39PM +0530, 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 ca5fa8d981..a6211d416d 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -455,6 +455,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;
> @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>  }
>  
>  static const MemoryRegionOps uar_ops = {
> +    .read = uar_read,

Are you sure it is needed?
Looking at memory_region_dispatch_read1 i can see that there is a check but
not sure this is the right place. Anyways, if it is not, i believe this
should be framework responsibility.

>      .write = uar_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .impl = {
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:56:40PM +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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index 4faeb21631..ee2888259c 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -273,6 +273,10 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
>          pr_dbg("Failed to map to CQ page table\n");
>          goto out;
>      }
> +    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
> +        pr_dbg("invalid nchunks: %d\n", nchunks);
> +        goto out;
> +    }
>  
>      r = g_malloc(sizeof(*r));
>      *ring = r;
> @@ -389,6 +393,11 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t pdir_dma,
>          pr_dbg("Failed to map to CQ page table\n");
>          goto out;
>      }
> +    if (!spages || spages > PVRDMA_MAX_FAST_REG_PAGES
> +        || !rpages || rpages > PVRDMA_MAX_FAST_REG_PAGES) {
> +        pr_dbg("invalid pages: %d, %d\n", spages, rpages);
> +        goto out;
> +    }
>  

This check (along with the one in create_cq_ring) better be placed before
mapping to page table.

With or without accepting the suggestion fix LGTM.

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

>      sr = g_malloc(2 * sizeof(*rr));
>      rr = &sr[1];
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:56:41PM +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 | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index ee2888259c..e8d99f29fa 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
>  
>      resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev,
>                                       cmd->cqe, &resp->cq_handle, ring);
> -    resp->cqe = cmd->cqe;
> +    if (resp->hdr.err) {
> +        g_free(ring);

This is not enough since all ring's resources (ring state and ring's pages)
left mapped.

The steps needed are the steps detailed in destroy_cq.

> +    }
>  
>  out:
>      pr_dbg("ret=%d\n", resp->hdr.err);
> @@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
>                                       cmd->max_send_sge, cmd->send_cq_handle,
>                                       cmd->max_recv_wr, cmd->max_recv_sge,
>                                       cmd->recv_cq_handle, rings, &resp->qpn);
> +    if (resp->hdr.err) {
> +        g_free(rings);

Ditto, here send rind and recv rings stays mapped.
Look at how QP's ring is destroyed in destroy_qp.

For both case suggesting to define a new static function that destroy rings
and call it from both error flow of create_* and from destroy_*

> +        goto out;
> +    }
>  
>      resp->max_send_wr = cmd->max_send_wr;
>      resp->max_recv_wr = cmd->max_recv_wr;
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:56:42PM +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 | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
> index 01247fc041..61b6a0a869 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -73,6 +73,7 @@ out:
>  
>  void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
>  {
> +    int e;
>      unsigned int idx = 0, offset;
>  
>      /*
> @@ -80,7 +81,8 @@ void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
>             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;
>      }
> @@ -105,20 +107,24 @@ 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;
> +    }

Thanks Prasad,
Two cosmetic suggestions though:
(1) In pvrdma_ring_next_elem_read, can you stick to your convention and
replace 'e' with 'idx'?
(2) Can you delete all leftover comments?

In any case:

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

>  
>      offset = idx * ring->elem_sz;
>      return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);
> -- 
> 2.19.2
> 

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

[Yuval Shaia]

On Tue, Dec 11, 2018 at 06:47:43PM +0200, Yuval Shaia wrote:
> On Tue, Dec 11, 2018 at 06:56:41PM +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 | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> > index ee2888259c..e8d99f29fa 100644
> > --- a/hw/rdma/vmw/pvrdma_cmd.c
> > +++ b/hw/rdma/vmw/pvrdma_cmd.c
> > @@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
> >  
> >      resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev,
> >                                       cmd->cqe, &resp->cq_handle, ring);
> > -    resp->cqe = cmd->cqe;
> > +    if (resp->hdr.err) {
> > +        g_free(ring);
> 
> This is not enough since all ring's resources (ring state and ring's pages)
> left mapped.
> 
> The steps needed are the steps detailed in destroy_cq.
> 
> > +    }
> >  
> >  out:
> >      pr_dbg("ret=%d\n", resp->hdr.err);
> > @@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
> >                                       cmd->max_send_sge, cmd->send_cq_handle,
> >                                       cmd->max_recv_wr, cmd->max_recv_sge,
> >                                       cmd->recv_cq_handle, rings, &resp->qpn);
> > +    if (resp->hdr.err) {
> > +        g_free(rings);
> 
> Ditto, here send rind and recv rings stays mapped.
> Look at how QP's ring is destroyed in destroy_qp.
> 
> For both case suggesting to define a new static function that destroy rings
> and call it from both error flow of create_* and from destroy_*
> 
> > +        goto out;
> > +    }
> >  
> >      resp->max_send_wr = cmd->max_send_wr;
> >      resp->max_recv_wr = cmd->max_recv_wr;

Also, can you rebase this patch on top of the patchset i posted last week:
https://patchwork.kernel.org/patch/10705439/

Thanks,

> > -- 
> > 2.19.2
> > 

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

[P J P]

  Hello Yuval,

+-- On Tue, 11 Dec 2018, Yuval Shaia wrote --+
| > Ditto, here send rind and recv rings stays mapped.
| > Look at how QP's ring is destroyed in destroy_qp.
| > 
| > For both case suggesting to define a new static function that destroy rings
| > and call it from both error flow of create_* and from destroy_*
| > 

I see, okay.

Similar resource clean-up required in pvrdma_realize(). In case of an error it 
does: 'goto out;', but nothing is free'd there.

Is another destroy_ routine required there?
 
| Also, can you rebase this patch on top of the patchset i posted last week:
| https://patchwork.kernel.org/patch/10705439/

Okay, I'll send revised patch set. Thanks so much for the prompt review.

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

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

[李强]

At 2018-12-11 23:22:32, "Yuval Shaia" <yuval.shaia@oracle.com> wrote:
>On Tue, Dec 11, 2018 at 06:56:39PM +0530, 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 ca5fa8d981..a6211d416d 100644
>> --- a/hw/rdma/vmw/pvrdma_main.c
>> +++ b/hw/rdma/vmw/pvrdma_main.c
>> @@ -455,6 +455,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;
>> @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>  }
>>  
>>  static const MemoryRegionOps uar_ops = {
>> +    .read = uar_read,
>

>Are you sure it is needed?


I'm quite sure this.
The issue here is that in memory_region_dispatch_read1
if there is no mr's read callback, the 'memory_region_read_with_attrs_accessor' 
will be called, but in that the 'mr->ops->raed_with_attrs' has no check.


In fact, I have send out a patch for the framework:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02265.html


But no more response.


>Looking at memory_region_dispatch_read1 i can see that there is a check but >not sure this is the right place. Anyways, if it is not, i believe this
>should be framework responsibility.


Reference Peter's answer here:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01404.html


"Currently our semantics are "you must provide both read and write, even
if one of them just always returns 0 / does nothing / returns an error".
We could probably reasonably assert this at the point when the
MemoryRegionOps is registered."




Thanks,
Li Qiang


> >> .write = uar_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> .impl = { >> -- >> 2.19.2 >>

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

[P J P]

+-- On Wed, 12 Dec 2018, P J P wrote --+
| | Also, can you rebase this patch on top of the patchset i posted last week:
| | https://patchwork.kernel.org/patch/10705439/
| 
| Okay, I'll send revised patch set. Thanks so much for the prompt review.

I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But 
it does not seem to apply cleanly.

===
$ git branch rdma v3.1.0-rc0
$ git checkout rdma 
Switched to branch 'rdma'
$ git apply --check /tmp/add-support-for-RDMA-MAD.patch
error: patch failed: Makefile.objs:1
error: Makefile.objs: patch does not apply
error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
===

I skipped one of the makefile.objs/.json patch, applied others and then 
hand-fixed one of the patch pvrdma_cmd.c.

After that when I try to build QEMU, it fails with

===
 $ ./configure --prefix=/tmp/ --enable-debug
      --target-list='x86_64-softmmu x86_64-linux-user'
      --enable-rdma --with-sdlabi=2.0
 $ make
../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function 
‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration]  qapi_event_send_rdma_gid_status_changed(ifname, true,
...
../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No 
such file or directory
 #include "qapi/qapi-events-rdma.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
===

Not sure if other patches are required. Any idea?

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

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 03:09:19PM +0530, P J P wrote:
> +-- On Wed, 12 Dec 2018, P J P wrote --+
> | | Also, can you rebase this patch on top of the patchset i posted last week:
> | | https://patchwork.kernel.org/patch/10705439/
> | 
> | Okay, I'll send revised patch set. Thanks so much for the prompt review.
> 
> I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But 
> it does not seem to apply cleanly.
> 
> ===
> $ git branch rdma v3.1.0-rc0
> $ git checkout rdma 
> Switched to branch 'rdma'
> $ git apply --check /tmp/add-support-for-RDMA-MAD.patch
> error: patch failed: Makefile.objs:1
> error: Makefile.objs: patch does not apply
> error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
> error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
> ===
> 
> I skipped one of the makefile.objs/.json patch, applied others and then 
> hand-fixed one of the patch pvrdma_cmd.c.
> 
> After that when I try to build QEMU, it fails with
> 
> ===
>  $ ./configure --prefix=/tmp/ --enable-debug
>       --target-list='x86_64-softmmu x86_64-linux-user'
>       --enable-rdma --with-sdlabi=2.0
>  $ make
> ../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function 
> ‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration]  qapi_event_send_rdma_gid_status_changed(ifname, true,
> ...
> ../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No 
> such file or directory
>  #include "qapi/qapi-events-rdma.h"
>           ^~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> ===
> 
> Not sure if other patches are required. Any idea?

Ok, you rebased your patches on latest upstream, lets deal with merge
conflicts later.

Marcel, any idea?
Can Prasad take from your tree?

Thanks,

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

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

[Yuval Shaia]

On Wed, Dec 12, 2018 at 03:09:19PM +0530, P J P wrote:
> +-- On Wed, 12 Dec 2018, P J P wrote --+
> | | Also, can you rebase this patch on top of the patchset i posted last week:
> | | https://patchwork.kernel.org/patch/10705439/
> | 
> | Okay, I'll send revised patch set. Thanks so much for the prompt review.
> 
> I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But 
> it does not seem to apply cleanly.
> 
> ===
> $ git branch rdma v3.1.0-rc0

Can you try master?
I just did a rebase on top of master and had no conflicts.

> $ git checkout rdma 
> Switched to branch 'rdma'
> $ git apply --check /tmp/add-support-for-RDMA-MAD.patch
> error: patch failed: Makefile.objs:1
> error: Makefile.objs: patch does not apply
> error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
> error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
> ===
> 
> I skipped one of the makefile.objs/.json patch, applied others and then 
> hand-fixed one of the patch pvrdma_cmd.c.
> 
> After that when I try to build QEMU, it fails with
> 
> ===
>  $ ./configure --prefix=/tmp/ --enable-debug
>       --target-list='x86_64-softmmu x86_64-linux-user'
>       --enable-rdma --with-sdlabi=2.0
>  $ make
> ../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function 
> ‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration]  qapi_event_send_rdma_gid_status_changed(ifname, true,
> ...
> ../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No 
> such file or directory
>  #include "qapi/qapi-events-rdma.h"
>           ^~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> ===
> 
> Not sure if other patches are required. Any idea?
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[P J P]

+-- On Wed, 12 Dec 2018, Yuval Shaia wrote --+
| Can you try master?
| I just did a rebase on top of master and had no conflicts.

  Yes, I tried with master first, got the same errors, that's when I tried 
earlier versions.

Preparing patch-set v2 with the suggested updates.

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