Re: [Qemu-devel] [PATCH] hw/pvrdma: Post CQE when receive invalid gid index

[Marcel Apfelbaum]

On 1/18/19 6:23 PM, Yuval Shaia wrote:

On Fri, Jan 18, 2019 at 03:55:36PM +0200, Marcel Apfelbaum wrote:

Hi Yuval,

On 1/9/19 10:15 PM, Yuval Shaia wrote:

This error should propagate back to guest.

Signed-off-by: Yuval Shaia <yuval.sh...@oracle.com>
---
   hw/rdma/rdma_backend.h      | 1 +
   hw/rdma/vmw/pvrdma_qp_ops.c | 6 ++++--
   2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index a9ba40ae48..5114c90e67 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -32,6 +32,7 @@
   #define VENDOR_ERR_INVLKEY          0x207
   #define VENDOR_ERR_MR_SMALL         0x208
   #define VENDOR_ERR_INV_MAD_BUFF     0x209
+#define VENDOR_ERR_INV_GID_IDX      0x210
   /* Add definition for QP0 and QP1 as there is no userspace enums for them */
   enum ibv_special_qp_type {
diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
index 465bee8641..0565eba981 100644
--- a/hw/rdma/vmw/pvrdma_qp_ops.c
+++ b/hw/rdma/vmw/pvrdma_qp_ops.c
@@ -178,7 +178,8 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
           sgid = rdma_rm_get_gid(&dev->rdma_dev_res, 
wqe->hdr.wr.ud.av.gid_index);
           if (!sgid) {
               pr_dbg("Fail to get gid for idx %d\n", 
wqe->hdr.wr.ud.av.gid_index);
-            return -EIO;
+            complete_with_error(VENDOR_ERR_INV_GID_IDX, comp_ctx);

There may be a problem here, comp_ctx may be uninitialized at this point.
I see that comp_ctx gets initalized after this call:

          /* Prepare CQE */
         comp_ctx = g_malloc(sizeof(CompHandlerCtx));
         comp_ctx->dev = dev;


What do you think?

Thanks,
Marcel

Applying this patch on top of upstream make sense.
Problem start because you probably applied Li Qiang's patch "[PATCH v2] hw:
pvrdma: fix memory leak in error path" which moves the initialization of
comp_ctx to be right after all checks are done.

More than that, accepting this patch makes Li Qiang's patch redundant since
the cleanup of comp_ctx will be taken care by the usual process of
'completion' (see the callback function pvrdma_qp_ops_comp_handler).
Li Qiang, can you confirm?

Sounds right to me. I'll add Reported-by tag and keep the coverity info.

Thanks,
Marcel

Yuval

+            continue;
           }
           pr_dbg("sgid_id=%d, sgid=0x%llx\n", wqe->hdr.wr.ud.av.gid_index,
                  sgid->global.interface_id);
@@ -189,7 +190,8 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
           if (sgid_idx <= 0) {
               pr_dbg("Fail to get bk sgid_idx for sgid_idx %d\n",
                      wqe->hdr.wr.ud.av.gid_index);
-            return -EIO;
+            complete_with_error(VENDOR_ERR_INV_GID_IDX, comp_ctx);
+            continue;
           }
           if (wqe->hdr.num_sge > dev->dev_attr.max_sge) {