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 <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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
>