Re: [for-next v1 4/5] RDMA/ionic: add Shared receive queue (SRQ) support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Kernel CQ polling is broken for SRQ-attached QPs, leading to a permanent CQ stall and dmesg log flood.
- [High] DMA API Violation: Stale CPU cache reads of `resp_buf` on non-cache-coherent architectures.
- [Medium] Unprivileged userspace can trigger unrestricted kernel log spam via conflicting CMB flags.
- [Medium] SRQ Controller Memory Buffer (CMB) initialization is dead code because `srq->rq.cmb` is uninitialized and firmware flags are omitted.
- [Medium] API Contract Violation: `ionic_create_srq` fails to update `attr->attr.max_sge`.

Pre-existing issues:
- [High] Double free / Use-After-Free of `mmap_cmb` entries on error handling paths.
--

commit d21e90481932bd4c2be24a16e0cc7eedd7d8fe39
Author: Amit Kumar <a.k@amd.com>

RDMA/ionic: add Shared receive queue (SRQ) support

This commit implements device-supported verb APIs for shared receive
queues (SRQ) in the ionic RDMA driver.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> index 884f5665e7a08..6979e0c5a25c2 100644
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
[ ... ]
> @@ -2069,6 +2074,42 @@ static void ionic_qp_sq_destroy(struct ionic_ibdev *dev,
>  		ionic_queue_destroy(&qp->sq, dev->lif_cfg.hwdev);
>  }
>  
> +static void ionic_rq_mmap_cmb(struct ionic_ibdev *dev,
> +			      struct ionic_ctx *ctx,
> +			      struct ionic_rq *rq,
> +			      u64 *cmb_offset)
> +{
> +	bool wc;
> +
> +	/* set mapping by default to uncached for
> +	 * expdb (to guarantee writes order) otherwise
> +	 * writecombine, unless this default is
> +	 * overridden by userspace
> +	 */
> +	if ((rq->cmb & (IONIC_CMB_WC | IONIC_CMB_UC)) ==
> +		(IONIC_CMB_WC | IONIC_CMB_UC)) {
> +		ibdev_warn(&dev->ibdev,
> +			   "Both rq_cmb flags IONIC_CMB_WC and IONIC_CMB_UC set, using default driver mapping\n");

[Severity: Medium]
Can an unprivileged user trigger unrestricted kernel log spam here?

Since ionic_rq_mmap_cmb() is reachable from userspace QP or SRQ creation
without rate limiting, passing both IONIC_CMB_WC and IONIC_CMB_UC in the
init attributes can flood the dmesg log.

> +		rq->cmb &= ~(IONIC_CMB_WC | IONIC_CMB_UC);
> +	}
[ ... ]
> +static int ionic_create_srq_cmd(struct ionic_ibdev *dev,
> +				struct ionic_ctx *ctx,
> +				struct ionic_srq *srq,
> +				struct ionic_pd *pd,
> +				struct ionic_tbl_buf *buf,
> +				u8 udma_mask)
> +{
> +	struct ionic_admin_create_srq_resp *resp_buf;
> +	const u16 dbid = ionic_ctx_dbid(dev, ctx);
> +	struct ionic_admin_wr wr = {
> +		.work = COMPLETION_INITIALIZER_ONSTACK(wr.work),
> +		.wqe = {
> +			.op = IONIC_V1_ADMIN_CREATE_SRQ,
> +			.len = cpu_to_le16(IONIC_ADMIN_CREATE_SRQ_IN_V1_LEN),
> +			.cmd.create_srq = {
> +				.pd_id = cpu_to_le32(pd->pdid),
> +				.depth_log2 = srq->rq.q.depth_log2,
> +				.stride_log2 = srq->rq.q.stride_log2,
> +				.page_size_log2 = buf->page_size_log2,
> +				.map_count = cpu_to_le32(buf->tbl_pages),
> +				.dma_addr = ionic_pgtbl_dma(buf, 0),
> +				.dbid = cpu_to_le16(dbid),
> +				.qid = cpu_to_le32(srq->rq.qid),
> +				.low_wqes_limit = cpu_to_le16(srq->srq_limit),
> +				.udma_mask = udma_mask,
> +			}

[Severity: Medium]
Is the IONIC_SRQF_CMB flag missing from this firmware command?

The flags field is left uninitialized (0), so IONIC_SRQF_CMB is never sent
to the firmware even if the user requested a Controller Memory Buffer.

> +		}
> +	};
[ ... ]
> +	ionic_admin_post(dev, &wr);
> +
> +	rc = ionic_admin_wait(dev, &wr, 0);
> +	if (rc)
> +		goto err_admin;
> +
> +	if (be32_to_cpu(wr.cqe.status_length) < IONIC_ADMIN_CREATE_SRQ_OUT_V1_LEN) {
> +		rc = -EOPNOTSUPP;
> +		goto err_admin;
> +	}
> +
> +	srq->rq.qid = le32_to_cpu(resp_buf->id);
> +	srq->udma_idx = resp_buf->udma_idx;
> +
> +err_admin:
> +	dma_unmap_single(dev->lif_cfg.hwdev, resp_buf_dma, sizeof(*resp_buf),
> +			 DMA_FROM_DEVICE);

[Severity: High]
Will this result in stale cache reads on non-cache-coherent architectures?

The CPU accesses resp_buf->id and resp_buf->udma_idx before calling
dma_unmap_single(). On platforms without hardware cache coherency, this
can read stale zeroes from the CPU cache instead of the DMA-written values,
breaking SRQ usage.

> +err_dma:
> +	kfree(resp_buf);
> +
> +	return rc;
> +}
[ ... ]
> +int ionic_create_srq(struct ib_srq *ibsrq, struct ib_srq_init_attr *attr,
> +		     struct ib_udata *udata)
> +{
[ ... ]
> +	udma_mask = BIT(dev->lif_cfg.udma_count) - 1;
> +	if (udata) {
> +		rc = ib_copy_validate_udata_in(udata, req, rsvd);
> +		if (rc)
> +			return rc;
> +
> +		udma_mask &= req.udma_mask;
> +	}
> +
> +	if (!udma_mask)
> +		return -EINVAL;
> +
> +	rc = ionic_rq_init(dev, ctx, &srq->rq, &req.rq, &buf,
> +			   attr->attr.max_wr, attr->attr.max_sge,
> +			   req.rq_spec, udata);

[Severity: Medium]
Does this ignore the user's req.rq_cmb request?

Unlike QP creation which assigns qp->rq.cmb = req.rq_cmb, SRQ creation fails
to copy the field. Because srq is kzalloc'd, srq->rq.cmb remains 0, bypassing
CMB allocation entirely.

> +	if (rc)
> +		return rc;
> +
> +	srq->srq_limit = attr->attr.srq_limit;
[ ... ]
> +	ionic_pgtbl_unbuf(dev, &buf);
> +	attr->attr.max_wr = srq->rq.q.mask;
> +
> +	return 0;

[Severity: Medium]
Is an assignment to attr->attr.max_sge missing here?

Userspace applications rely on the returned capability boundaries. Without
informing the ULP or application of the actual allocated scatter-gather
element limit, they might post too many SGEs.

> +err_resp:
> +	if (udata && (srq->rq.cmb & IONIC_CMB_ENABLE))
> +		rdma_user_mmap_entry_remove(srq->rq.mmap_cmb);
> +err_mmap_rq:
> +	ionic_destroy_srq_cmd(dev, srq->rq.qid);
> +err_cmd:
> +	ionic_pgtbl_unbuf(dev, &buf);
> +	ionic_rq_destroy(dev, ctx, &srq->rq);

[Severity: High]
This is a pre-existing issue, but does this error path result in a
double-free or use-after-free of the mmap entry?

If ib_respond_udata() fails, rdma_user_mmap_entry_remove(srq->rq.mmap_cmb)
is explicitly called, but the pointer is not zeroed. The cleanup then falls
through to ionic_rq_destroy(), which calls ionic_rq_destroy_cmb() and frees
the exact same pointer again.

> +
> +	return rc;
> +}
[ ... ]
> diff --git a/drivers/infiniband/hw/ionic/ionic_datapath.c b/drivers/infiniband/hw/ionic/ionic_datapath.c
> index 9c40b54123d5f..e2d4d441052e6 100644
> --- a/drivers/infiniband/hw/ionic/ionic_datapath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_datapath.c
> @@ -1311,7 +1311,7 @@ static int ionic_post_recv_common(struct ionic_ibdev *dev,
>  	if (!bad)
>  		return -EINVAL;
>  
> -	if (!qp->has_rq) {
> +	if (qp->srq) {
>  		*bad = wr;
>  		return -EINVAL;
>  	}

[Severity: High]
Does kernel CQ polling break for SRQ-attached QPs, leading to a permanent
CQ stall and dmesg log flood?

When a userspace application polls the CQ, the kernel's ionic_poll_vcq_cq()
sees IONIC_V1_CQE_TYPE_RECV and calls ionic_poll_recv(). Because SRQ QPs
do not use qp->rq, qp->rq.q is zero-initialized and ionic_queue_empty()
returns true:

ionic_poll_recv() {
    if (ionic_queue_empty(&qp->rq.q)) {
        ibdev_warn(&dev->ibdev, "qp %u is empty\n", qp->qpid);
        return -EIO;
    }
    ...
}

When -EIO is returned, ionic_poll_vcq_cq() skips consuming the CQE:

ionic_poll_vcq_cq() {
    ...
    case IONIC_V1_CQE_TYPE_RECV:
        spin_lock(&qp->rq.lock);
        rc = ionic_poll_recv(dev, cq, qp, cqe, wc + npolled);
        spin_unlock(&qp->rq.lock);

        if (rc < 0)
            goto out;
    ...
}

This can permanently wedge the hardware CQ and allow an unprivileged
user to spam the kernel log.

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