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

[Abhijit Gangurde <abhijit.gangurde@amd.com>]

On 9/23/26 15:29, sashiko-bot@kernel.org wrote:
> 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?

This is not reachable. ionic_create_qp() rejects qp_type > IB_QPT_UD, so 
only GSI/RC/UC/UD reach the driver, and uverbs always supplies a recv_cq 
for those types.

>
> [ ... ]
>> +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.
Ack
>
>> +}
>> +
>> +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.
Ack
>
> [ ... ]
>> +		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.

Since driver does not modify it, ib_uverbs_create_srq() just echoes back 
whatever userspace requested.

Thanks,
Abhijit

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