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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] rdma/ionic: Invalid pointer dereference in ionic_create_qp when recv_cq is NULL
- [Medium] rdma/ionic: Missing assignment of negotiated max_sge in ionic_create_srq()
- [Medium] rdma/ionic: Missing ABI validation for reserved bytes in ionic_srq_req
- [Medium] rdma/ionic: Incorrect TEARDOWN flag in ionic_modify_srq_cmd masks hardware errors
--

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

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

Implement device supported verb APIs for shared receive queue.
SRQ is supported for userspace only. ionic_create_srq() returns
-EOPNOTSUPP when there is no ucontext, so kernel ULPs cannot create an
SRQ.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> index 7f0e8b63ed76c..ec9e1188f556f 100644
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
[ ... ]
> @@ -2360,22 +2401,25 @@ int ionic_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attr,
[ ... ]
> +	} else {
> +		if (udata && (req.rq_cmb & IONIC_CMB_ENABLE))
> +			qp->rq.cmb = req.rq_cmb;
> +
> +		/* for non-srq qps, rq qid is same as qpid */
> +		qp->rq.qid = qp->qpid;
> +		rc = ionic_rq_init(dev, ctx, &qp->rq, &req.rq, &rq_buf,
> +				   attr->cap.max_recv_wr, attr->cap.max_recv_sge,
> +				   req.rq_spec, udata);
>  	}
> +	if (rc)
> +		goto err_rq;
>  
>  	rc = ionic_create_qp_cmd(dev, pd,
>  				 to_ionic_vcq_cq(attr->send_cq, qp->udma_idx),
>  				 to_ionic_vcq_cq(attr->recv_cq, qp->udma_idx),
>  				 qp, &sq_buf, &rq_buf, attr, req.ionic_flags);

[Severity: High]
Since this patch adds SRQ support, it permits QPs to be created without a
receive CQ, meaning attr->recv_cq can be NULL.

However, to_ionic_vcq_cq() calculates an offset from its argument.
If attr->recv_cq is NULL, won't to_ionic_vcq_cq() return a non-NULL offset
pointer?

That invalid pointer is passed into ionic_create_qp_cmd() where it gets
unconditionally dereferenced to access recv_cq->cqid:

	} else {
		wr.wqe.cmd.create_qp.rq_tbl_index_srq_id = cpu_to_le32(qp->srq->rq.qid);
		wr.wqe.cmd.create_qp.rq_cq_id = cpu_to_le32(recv_cq->cqid);
	}

Would this cause a kernel panic upon dereference?

[ ... ]
> +static int ionic_modify_srq_cmd(struct ionic_ibdev *dev, u32 srqid,
> +				u16 srq_limit)
> +{
> +	struct ionic_admin_wr wr = {
> +		.work = COMPLETION_INITIALIZER_ONSTACK(wr.work),
> +		.wqe = {
> +			.op = IONIC_V1_ADMIN_MODIFY_SRQ,
> +			.len = cpu_to_le16(IONIC_ADMIN_MODIFY_SRQ_IN_V1_LEN),
> +			.cmd.modify_srq = {
> +				.qid = cpu_to_le32(srqid),
> +				.low_wqes_limit = cpu_to_le16(srq_limit),
> +			},
> +		}
> +	};
> +
> +	if (dev->lif_cfg.admin_opcodes <= IONIC_V1_ADMIN_MODIFY_SRQ)
> +		return -EOPNOTSUPP;
> +
> +	ionic_admin_post(dev, &wr);
> +
> +	return ionic_admin_wait(dev, &wr, IONIC_ADMIN_F_TEARDOWN);

[Severity: Medium]
Does using IONIC_ADMIN_F_TEARDOWN here mask hardware errors?

If the device is resetting, ionic_admin_wait() will return 0 instead of
-ENODEV. This would cause ionic_modify_srq() to incorrectly assume success
and update the software srq->srq_limit, permanently desynchronizing it from
the actual hardware limit.

> +}
> +
> +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;
> +	}

[Severity: Medium]
The ib_copy_validate_udata_in() macro checks that any userspace bytes beyond
the req struct are zeroed, but it doesn't validate the contents of the rsvd
array within the struct itself.

Is there a missing check to ensure req.rsvd is strictly zeroed? Without it,
buggy userspace applications could pass garbage in these reserved bytes,
preventing them from being safely repurposed for future ABI extensions.

[ ... ]
> +		if (srq->rq.cmb & IONIC_CMB_ENABLE) {
> +			ionic_rq_mmap_cmb(dev, ctx, &srq->rq, &resp.rq_cmb_offset);
> +			if (!srq->rq.mmap_cmb) {
> +				rc = -ENOMEM;
> +				goto err_srq_cmd;
> +			}
> +
> +			resp.rq_cmb = srq->rq.cmb;
> +		}
> +
> +		rc = ib_respond_udata(udata, resp);
> +		if (rc)
> +			goto err_srq_cmd;
> +	}
> +
> +	ionic_pgtbl_unbuf(dev, &buf);
> +
> +	attr->attr.max_wr = srq->rq.q.mask;
> +
> +	return 0;

[Severity: Medium]
This updates the negotiated max_wr, but is it missing an assignment to
attr->attr.max_sge before returning? Without it, userspace wouldn't receive
the actual negotiated max_sge limits.

> +
> +err_srq_cmd:
> +	ionic_destroy_srq_cmd(dev, srq->rq.qid);
> +err_cmd:
> +	ionic_pgtbl_unbuf(dev, &buf);
> +	ionic_rq_destroy(dev, ctx, &srq->rq);
> +
> +	return rc;
> +}

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