Re: [for-next v1 2/5] RDMA/ionic: support firmware-assigned CQ IDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] In `ionic_create_cq_cmd()`, the return value of `dma_mapping_error()` is directly assigned to `rc` and returned on failure. `dma_mapping_error()` returns a positive integer (typically 1) on failure, not a standard negative error code (like `-ENOMEM`). This positive return value propagates up to `ionic_create_cq()`, which returns it to the RDMA core. The RDMA core (e.g., `ib_create_cq`) translates this to an error pointer (`ERR_PTR(1)`). Since `IS_ERR()` only checks for negative error values (`[-MAX_ERRNO, -1]`), `IS_ERR(ERR_PTR(1))` evaluates to false. The caller incorrectly assumes the CQ creation succeeded and dereferences the invalid pointer `0x1`, causing a kernel panic.
- [High] In `ionic_create_cq_cmd()`, `resp_buf` is mapped with `DMA_FROM_DEVICE` and the device writes the CQ ID response to it. After the device completes the command, the CPU directly reads `resp_buf->id` and `resp_buf->udma_idx` BEFORE calling `dma_unmap_single()`. On architectures without hardware cache coherency (like some ARM or MIPS CPUs), the CPU may read stale zeroed cache lines instead of the actual data written by the device. The kernel DMA API mandates that memory mapped with `DMA_FROM_DEVICE` must be unmapped or explicitly synchronized (e.g., via `dma_sync_single_for_cpu()`) before the CPU is allowed to read it.
- [High] In `ionic_destroy_cq()`, the function calls `ionic_pre_destroy_cq_cmd()` before issuing the hardware destroy command (`ionic_destroy_cq_cmd()`). `ionic_pre_destroy_cq_cmd()` removes the CQ from the tracking XArray and drops its `kref` to 0, which triggers `complete(&cq->cq_rel_comp)`. If the subsequent hardware command fails (e.g., due to timeout), the error handling path skips `ionic_destroy_cq_common()` (which would normally set `cq->vcq = NULL`) and returns the error to the RDMA core. The RDMA core permits `ib_destroy_cq_user` to fail, leaving the CQ object alive and allowing the user to retry the destruction. Upon retry, `ionic_pre_destroy_cq_cmd()` is invoked again (since `cq->vcq` is not NULL), which calls `kref_put()` on a refcount that is already 0. This triggers a `refcount_t` underflow warning and returns false, bypassing the completion signal. The subsequent `wait_for_completion(&cq->cq_rel_comp)` will sleep forever because the completion was consumed in the first attempt, resulting in an unkillable task deadlock.
--

commit 498fccc09f79cccc72f1e999db1c6895d77eb47e
Author: Abhijit Gangurde <abhijit.gangurde@amd.com>

RDMA/ionic: support firmware-assigned CQ IDs

When the LIF advertises qid allocation for CQ, let firmware return
the completion queue ID instead of allocating it in the driver bitmap.
The CQ create/destroy process is split into distinct phases to avoid
a race condition with the CQ XArray during concurrent creates.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
[ ... ]
> @@ -1243,13 +1254,55 @@ static int ionic_create_cq_cmd(struct ionic_ibdev *dev,
>  			}
>  		}
>  	};
> +	dma_addr_t resp_buf_dma;
> +	int rc;
>  
>  	if (dev->lif_cfg.admin_opcodes <= IONIC_V1_ADMIN_CREATE_CQ)
>  		return -EBADRQC;
>  
> +	if (!ionic_fw_has_qid_alloc(dev, IONIC_LIF_RDMA_ALLOC_QID_CQ)) {
> +		ionic_admin_post(dev, &wr);
> +		return ionic_admin_wait(dev, &wr, 0);
> +	}
> +
> +	resp_buf = kzalloc_obj(*resp_buf);
> +	if (!resp_buf)
> +		return -ENOMEM;
> +
> +	resp_buf_dma = dma_map_single(dev->lif_cfg.hwdev, resp_buf,
> +				      sizeof(*resp_buf),
> +				      DMA_FROM_DEVICE);
> +
> +	rc = dma_mapping_error(dev->lif_cfg.hwdev, resp_buf_dma);
> +	if (rc)
> +		goto err_dma;

[Severity: Critical]
Does this code return a positive integer on DMA mapping failure?

dma_mapping_error() returns a positive value (like 1) on failure rather
than a standard negative error code. This positive value will propagate
up to ionic_create_cq() and back to the RDMA core.

Because IS_ERR() only checks for negative error codes, IS_ERR(ERR_PTR(1))
evaluates to false. Could this cause the caller to incorrectly assume
the CQ creation succeeded and dereference the invalid pointer 0x1,
leading to a kernel panic?

> +
> +	wr.wqe.len = cpu_to_le16(IONIC_ADMIN_CREATE_CQ_IN_V2_LEN);
> +	wr.wqe.cmd.create_cq.udma_mask = udma_mask;
> +	wr.wqe.cmd.create_cq.resp_dma_addr = cpu_to_le64(resp_buf_dma);
> +	wr.wqe.cmd.create_cq.resp_buf_len = cpu_to_le32(IONIC_ADMIN_CREATE_CQ_OUT_V1_LEN);
>  
>  	ionic_admin_post(dev, &wr);
> +	rc = ionic_admin_wait(dev, &wr, 0);
> +	if (rc)
> +		goto err_admin;
>  
> -	return ionic_admin_wait(dev, &wr, 0);
> +	if (be32_to_cpu(wr.cqe.status_length) < IONIC_ADMIN_CREATE_CQ_OUT_V1_LEN) {
> +		rc = -EOPNOTSUPP;
> +		goto err_admin;
> +	}
> +
> +	cq->cqid = le32_to_cpu(resp_buf->id);
> +	if (out_udma_idx)
> +		*out_udma_idx = resp_buf->udma_idx;

[Severity: High]
Are we reading the DMA buffer before unmapping it?

The CPU directly reads resp_buf->id and resp_buf->udma_idx here, but
dma_unmap_single() hasn't been called yet. On architectures without
hardware cache coherency, could the CPU read stale zeroed cache lines
instead of the actual data written by the device?

The DMA API requires memory mapped with DMA_FROM_DEVICE to be unmapped
or explicitly synchronized (e.g., via dma_sync_single_for_cpu()) before
the CPU can safely read it.

> +
> +err_admin:
> +	dma_unmap_single(dev->lif_cfg.hwdev, resp_buf_dma, sizeof(*resp_buf),
> +			 DMA_FROM_DEVICE);
> +err_dma:
> +	kfree(resp_buf);
> +
> +	return rc;
>  }
[ ... ]
> @@ -1364,6 +1424,13 @@ int ionic_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
>  		if (!(vcq->udma_mask & BIT(udma_idx)))
>  			continue;
>  
> +		/*
> +		 * Untrack the CQ before releasing its hardware ID below, so a
> +		 * concurrent create that gets the same ID reused by firmware
> +		 * cannot have its fresh XArray entry erased by this destroy.
> +		 */
> +		ionic_pre_destroy_cq_cmd(dev, &vcq->cq[udma_idx]);
> +
>  		rc_tmp = ionic_destroy_cq_cmd(dev, vcq->cq[udma_idx].cqid);
>  		if (rc_tmp) {
>  			if (!rc)
>  				rc = rc_tmp;
>  
>  			continue;
>  		}

[Severity: High]
If ionic_destroy_cq_cmd() fails, does this error path leave the CQ
in an unrecoverable state?

The call to ionic_pre_destroy_cq_cmd() drops the CQ's kref to 0 and
triggers complete(&cq->cq_rel_comp). If the subsequent hardware command
fails, the function returns the error to the RDMA core without clearing
the CQ object state.

The RDMA core permits destruction to be retried. If the user retries:

ionic_destroy_cq()
   ionic_pre_destroy_cq_cmd()
       kref_put(&cq->cq_kref, ionic_cq_complete);

This second attempt will hit a refcount underflow (since it is already 0),
meaning complete() is bypassed. The subsequent wait_for_completion()
would then block forever because the completion was consumed on the
first attempt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090802.152142-1-abhijit.gangurde@amd.com?part=2