Re: [PATCH v1 13/15] RDMA/xsc: add QP verbs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] RDMA/xsc: heap out-of-bounds write in create_user_qp() when hw_npages is 0
- [High] RDMA/xsc: NULL CQ dereference in create_qp_common() debug formatting
- [High] RDMA/xsc: 32-bit overflow in create_user_qp() sq.offset calculation
- [High] RDMA/xsc: incomplete and racy err_state_qp_list cleanup in xsc_ib_destroy_qp()
- [High] RDMA/xsc: leaked error nodes and unlocked WQ reset in xsc_ib_modify_qp()
- [High] RDMA/xsc: SRQ WR leak and qp_num corruption across QP state transitions
- [High] RDMA/xsc: uninitialized kernel memory leak to user in xsc_ib_query_qp()
- [Medium] RDMA/xsc: insufficient udata->inlen checks in create_user_qp()
- [Medium] RDMA/xsc: uninitialized stack variable offset logged in create_user_qp()
- [Medium] RDMA/xsc: integer truncation in set_rq_size() before max_wqes check
- [Medium] RDMA/xsc: IB_MTU_8192 misreported as IB_MTU_4096 in xsc_ib_query_qp()
- [Medium] RDMA/xsc: premature ctx mutation and wrong page_shift in QP verbs
--

commit f535ae7002b2233def13ec88aba5f48f2448d9b9
Author: Xin Tian <tianx@yunsilicon.com>

RDMA/xsc: add QP verbs

This commit implements queue pair creation, modification, query, and
destruction callbacks for the xsc IB driver, supporting RC, MAD QP0/QP1, and
raw packet queue pairs for both kernel and user-space callers.

> diff --git a/drivers/infiniband/hw/xsc/qp.c b/drivers/infiniband/hw/xsc/qp.c
> index a41d880cf03d9..0dfb102949070 100644
> --- a/drivers/infiniband/hw/xsc/qp.c
> +++ b/drivers/infiniband/hw/xsc/qp.c
[ ... ]
> +static int set_rq_size(struct xsc_ib_dev *dev, struct ib_qp_init_attr *init_attr,
> +		       int has_rq, struct xsc_ib_qp *qp, struct xsc_ib_create_qp *ucmd)
> +{
> +	u32 wqe_cnt = init_attr->cap.max_recv_wr ?
> +		roundup_pow_of_two(init_attr->cap.max_recv_wr) : 0;
> +
> +	if (init_attr->srq)
> +		wqe_cnt = to_xsrq(init_attr->srq)->xsrq.max;
> +
> +	if (wqe_cnt > dev->xdev->caps.max_wqes) {
> +		ibdev_err(&dev->ib_dev, "max_recv_wr:%d exceed max rq depth\n",
> +			  init_attr->cap.max_recv_wr);
> +		return -EINVAL;
> +	}
[ ... ]
> +static int calc_sq_size(struct xsc_ib_dev *dev, struct ib_qp_init_attr *attr,
> +			struct xsc_ib_qp *qp)
> +{
> +	int wqe_size;
> +	size_t wq_size;
> +
> +	if (!attr->cap.max_send_wr) {
> +		ibdev_err(&dev->ib_dev, "invalid max_send_wr:%d\n", attr->cap.max_send_wr);
> +		return -1;
> +	}
> +
> +	wqe_size = 1 << dev->xdev->caps.send_wqe_shift;
> +	qp->max_inline_data = (dev->xdev->caps.send_ds_num - 2) * sizeof(struct xsc_wqe_data_seg);
> +	attr->cap.max_inline_data = qp->max_inline_data;
> +
> +	attr->cap.max_send_wr = min_t(u32, attr->cap.max_send_wr, dev->xdev->caps.max_wqes);
> +	qp->sq.wqe_cnt = roundup_pow_of_two(attr->cap.max_send_wr);

[Severity: Medium]
In set_rq_size(), if init_attr->cap.max_recv_wr is greater than 0x80000000U
and init_attr->srq is NULL, roundup_pow_of_two(init_attr->cap.max_recv_wr)
returns 1UL << 32 on 64-bit platforms (or invokes shift undefined behavior on
32-bit platforms) and truncates to 0 when assigned to u32 wqe_cnt.

When wqe_cnt truncates to 0, the wqe_cnt > dev->xdev->caps.max_wqes check is
bypassed and, for kernel QPs, qp->rq.wqe_cnt and qp->rq.max_post remain 0
even though qp->has_rq is 1.

Also, in calc_sq_size(), attr->cap.max_send_wr is silently clamped to
dev->xdev->caps.max_wqes with min_t() instead of returning -EINVAL when the
requested depth exceeds device limits.

Should set_rq_size() and calc_sq_size() validate init_attr->cap.max_recv_wr
and attr->cap.max_send_wr against dev->xdev->caps.max_wqes before calling
roundup_pow_of_two()?

[ ... ]
> +static int create_user_qp(struct xsc_ib_dev *dev, struct ib_pd *pd,
> +			  struct xsc_ib_qp *qp, struct ib_udata *udata,
> +			  struct xsc_ib_create_qp *ucmd,
> +			  struct xsc_create_qp_mbox_in **in, int *inlen)
> +{
> +	int page_shift;
> +	int npages;
> +	u32 offset;
> +	int ncont;
> +	int err;
> +	int hw_npages;
> +	u32 sq_shift, rq_shift;
> +	u32 sq_wqe_cnt, rq_wqe_cnt;
> +	struct ib_umem_dmabuf *umem_dmabuf;
> +
> +	ibdev_dbg(&dev->ib_dev, "buf_addr:0x%lx db_addr:0x%lx sq cnt:%u, rq cnt:%u, rq shift:%u\n",
> +		  (uintptr_t)ucmd->buf_addr, (uintptr_t)ucmd->db_addr,
> +		    ucmd->sq_wqe_count, ucmd->rq_wqe_count, ucmd->rq_wqe_shift);
> +
> +	sq_shift = dev->xdev->caps.send_wqe_shift;
> +	rq_shift = ucmd->rq_wqe_shift;
> +	if (sq_shift < XSC_BASE_WQE_SHIFT || sq_shift > 31 ||
> +	    (ucmd->rq_wqe_count &&
> +	     (rq_shift < XSC_BASE_WQE_SHIFT || rq_shift > 31))) {
> +		ibdev_err(&dev->ib_dev, "invalid user queue parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ucmd->sq_wqe_count) {
> +		if (!is_power_of_2(ucmd->sq_wqe_count) ||
> +		    ucmd->sq_wqe_count < (1u << (sq_shift - XSC_BASE_WQE_SHIFT))) {
> +			ibdev_err(&dev->ib_dev, "invalid user sq parameters\n");
> +			return -EINVAL;
> +		}
> +		sq_wqe_cnt = ucmd->sq_wqe_count >> (sq_shift - XSC_BASE_WQE_SHIFT);
> +	} else {
> +		sq_wqe_cnt = 0;
> +	}
> +
> +	if (ucmd->rq_wqe_count) {
> +		if (!is_power_of_2(ucmd->rq_wqe_count) ||
> +		    ucmd->rq_wqe_count < (1u << (rq_shift - XSC_BASE_WQE_SHIFT))) {
> +			ibdev_err(&dev->ib_dev, "invalid user rq parameters\n");
> +			return -EINVAL;
> +		}
> +		rq_wqe_cnt = ucmd->rq_wqe_count >> (rq_shift - XSC_BASE_WQE_SHIFT);
> +	} else {
> +		rq_wqe_cnt = 0;
> +	}
> +
> +	if (sq_wqe_cnt > dev->xdev->caps.max_wqes ||
> +	    rq_wqe_cnt > dev->xdev->caps.max_wqes) {
> +		ibdev_err(&dev->ib_dev, "user queue depth exceeds max %u\n",
> +			  dev->xdev->caps.max_wqes);
> +		return -EINVAL;
> +	}
> +
> +	qp->sq.ds_cnt = ucmd->sq_wqe_count;
> +	qp->sq.wqe_cnt = sq_wqe_cnt;
> +	qp->sq.wqe_shift = sq_wqe_cnt ? sq_shift : 0;
> +	qp->sq.max_gs = dev->xdev->caps.send_ds_num - XSC_CTRL_SEG_NUM - XSC_RADDR_SEG_NUM;
> +	qp->rq.ds_cnt = ucmd->rq_wqe_count;
> +	qp->rq.wqe_cnt = rq_wqe_cnt;
> +	qp->rq.wqe_shift = rq_wqe_cnt ? rq_shift : 0;
> +	if (rq_wqe_cnt) {
> +		qp->rq.max_gs = 1;
> +		qp->rq.max_post = rq_wqe_cnt;
> +	}
> +
> +	qp->rq.offset = 0;
> +	qp->sq.offset = (u32)qp->rq.wqe_cnt << qp->rq.wqe_shift;
> +	qp->buf_size = (u64)qp->sq.offset +
> +		       ((u64)qp->sq.wqe_cnt << qp->sq.wqe_shift);
> +	if (qp->buf_size > (u64)U16_MAX * XSC_PAGE_SIZE_4K) {
> +		ibdev_err(&dev->ib_dev, "QP buffer size too large\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Can the 32-bit shift in qp->sq.offset = (u32)qp->rq.wqe_cnt <<
qp->rq.wqe_shift overflow and bypass the qp->buf_size check in
create_user_qp()?

Because rq_shift is permitted up to 31 and struct xsc_ib_wq defines offset as
a signed 32-bit int, a user supplying ucmd->rq_wqe_shift = 31 and
ucmd->rq_wqe_count = 1U << 28 gets rq_wqe_cnt = 2 (which passes the max_wqes
check), while (u32)qp->rq.wqe_cnt << qp->rq.wqe_shift wraps in 32-bit
arithmetic to 0.

When qp->sq.offset wraps to 0, qp->buf_size only accounts for the SQ size and
passes the U16_MAX * XSC_PAGE_SIZE_4K check, so the driver pins and populates
PAS entries only for the SQ while create_qp_common() programs
in->req.log_rq_sz = ilog2(qp->rq.ds_cnt) (a 4 GiB RQ) into the hardware.
Similarly, if ucmd->sq_wqe_count is 0, create_user_qp() allocates 0 bytes for
the SQ in qp->buf_size, whereas create_qp_common() still sets
in->req.log_sq_sz = ilog2(0x80) in the firmware request.

> +	hw_npages = DIV_ROUND_UP(qp->buf_size, XSC_PAGE_SIZE_4K);
> +
> +	if (udata->inlen >= offsetofend(typeof(*ucmd), dmabuf_fd) &&
> +	    ucmd->dmabuf_fd != -1) {
> +		if (ucmd->dmabuf_sz < qp->buf_size) {

[Severity: Medium]
In create_user_qp(), does the dmabuf check verify enough of udata->inlen
before reading ucmd->dmabuf_sz?

Checking udata->inlen >= offsetofend(typeof(*ucmd), dmabuf_fd) only ensures
the buffer covers dmabuf_fd (36 bytes), not the following dmabuf_sz field (up
to 48 bytes). Also, in create_qp_common(), ib_copy_from_udata() copies
min(udata->inlen, sizeof(ucmd)) without checking that udata->inlen is at
least offsetofend(struct xsc_ib_create_qp, flags) for the mandatory base
fields.

Should create_qp_common() reject udata->inlen smaller than
offsetofend(typeof(ucmd), flags) and create_user_qp() check
offsetofend(typeof(*ucmd), dmabuf_sz)?

> +			ibdev_err(&dev->ib_dev, "dmabuf_sz %llu < qp buf_size %llu\n",
> +				  ucmd->dmabuf_sz, qp->buf_size);
> +			return -EINVAL;
> +		}
> +
> +		umem_dmabuf = ib_umem_dmabuf_get_pinned(&dev->ib_dev, 0, ucmd->dmabuf_sz,
> +							ucmd->dmabuf_fd, IB_ACCESS_LOCAL_WRITE);
[ ... ]
> +	err = xsc_ib_get_buf_offset(ucmd->buf_addr, page_shift, &offset);
> +	if (err) {
> +		ibdev_err(&dev->ib_dev, "bad offset:%d\n", offset);
> +		goto err_umem;
> +	}

[Severity: Medium]
In create_user_qp(), offset is declared on the stack without initialization.
Looking at xsc_ib_get_buf_offset() in drivers/infiniband/hw/xsc/mem.c:

	if (buf_off & off_mask)
		return -EINVAL;

	*offset = buf_off >> ilog2(off_size);
	return 0;

When ucmd->buf_addr fails the alignment check, xsc_ib_get_buf_offset()
returns -EINVAL without initializing *offset, so ibdev_err() prints
uninitialized kernel stack memory. Also, when offset is non-zero, hw_npages =
DIV_ROUND_UP(qp->buf_size, XSC_PAGE_SIZE_4K) does not account for the
sub-page offset if the buffer crosses a 4K page boundary, and offset is never
passed to firmware.

> +	ibdev_dbg(&dev->ib_dev, "npage:%d, page_shift:%d, ncont:%d, offset:%u, hw_npages %d\n",
> +		  npages, page_shift, ncont, offset, hw_npages);
> +
> +	*inlen = sizeof(**in) + (size_t)hw_npages * sizeof((*in)->req.pas[0]);
> +	*in = xsc_vzalloc(*inlen);
> +	if (!*in) {
> +		err = -ENOMEM;
> +		goto err_umem;
> +	}
> +	xsc_ib_populate_pas(dev, qp->umem, page_shift, (*in)->req.pas, hw_npages, true);

[Severity: Critical]
Can create_user_qp() trigger an out-of-bounds heap write into (*in)->req.pas
when hw_npages is 0?

If userspace passes ucmd->sq_wqe_count == 0 and ucmd->rq_wqe_count == 0 (or
triggers the 32-bit overflow in qp->sq.offset with sq_wqe_count == 0),
qp->buf_size is 0 and hw_npages is 0. If userspace also provides a valid
ucmd->dmabuf_fd != -1 with ucmd->dmabuf_sz >= PAGE_SIZE, the check
ucmd->dmabuf_sz < qp->buf_size is false and ib_umem_dmabuf_get_pinned()
returns a non-empty umem.

create_user_qp() then allocates *in with *inlen = sizeof(**in) (zero elements
in req.pas[]) and calls xsc_ib_populate_pas() -> __xsc_ib_populate_pas() with
num_pages = 0:

drivers/infiniband/hw/xsc/mem.c:__xsc_ib_populate_pas()
	pas[idx] = cpu_to_be64(cur);
	...
	i++;

	/* Stop after num_pages reached */
	if (i >> shift >= offset + num_pages)
		return;

Because __xsc_ib_populate_pas() writes pas[0] before checking whether
num_pages has been reached, this overwrites 8 bytes past the end of *in.
Should create_user_qp() reject qp->buf_size == 0 (or both sq_wqe_count == 0
and rq_wqe_count == 0)?

[ ... ]
> +static int create_qp_common(struct xsc_ib_dev *dev, struct ib_pd *pd,
> +			    struct ib_qp_init_attr *init_attr,
> +			    struct ib_udata *udata, struct xsc_ib_qp *qp)
> +{
[ ... ]
> +	if (init_attr->send_cq) {
> +		qp->send_cq = init_attr->send_cq;
> +		in->req.cqn_send = to_xcq(init_attr->send_cq)->xcq.cqn;
> +		in->req.cqn_send = cpu_to_be16(in->req.cqn_send);
> +	}
> +
> +	if (init_attr->recv_cq) {
> +		qp->recv_cq = init_attr->recv_cq;
> +		in->req.cqn_recv = to_xcq(init_attr->recv_cq)->xcq.cqn;
> +		in->req.cqn_recv = cpu_to_be16(in->req.cqn_recv);
> +	}
[ ... ]
> +	err = xsc_core_create_qp(dev->xdev, &qp->xqp, in, inlen);
> +	if (err) {
> +		ibdev_err(&dev->ib_dev, "create qp failed, err=%d\n", err);
> +		goto err_create;
> +	}
> +
> +	qp->doorbell_qpn = qp->xqp.qpn;
> +
> +	qp->xqp.qp_type = init_attr->qp_type;
> +	ret += snprintf(ptr + ret, 256 - ret, "pdn=%d,", to_xpd(pd)->pdn);
> +	ret += snprintf(ptr + ret, 256 - ret, "log_rq_sz=%d,", in->req.log_rq_sz);
> +	ret += snprintf(ptr + ret, 256 - ret, "log_sq_sz=%d,", in->req.log_sq_sz);
> +	ret += snprintf(ptr + ret, 256 - ret, "scqn=%d,", to_xcq(init_attr->send_cq)->xcq.cqn);
> +	ret += snprintf(ptr + ret, 256 - ret, "rcqn=%d", to_xcq(init_attr->recv_cq)->xcq.cqn);

[Severity: High]
Earlier in create_qp_common(), init_attr->send_cq and init_attr->recv_cq are
checked for NULL before dereferencing, and get_cqs() also guards both
pointers.

However, these two snprintf() calls unconditionally dereference
to_xcq(init_attr->send_cq) and to_xcq(init_attr->recv_cq). Since ibcq is at
offset 0 of struct xsc_ib_cq, won't creating a QP with a NULL send_cq or
recv_cq cause a NULL pointer dereference here right after xsc_core_create_qp()
has already created the QP in firmware?

[ ... ]
> +int xsc_ib_destroy_qp(struct ib_qp *qp, struct ib_udata *udata)
> +{
> +	struct xsc_ib_dev *dev = to_xdev(qp->device);
> +	struct xsc_ib_qp *xqp = to_xqp(qp);
> +	struct xsc_err_state_qp_node *tmp = NULL, *err_rq_node = NULL, *err_sq_node = NULL;
> +	struct xsc_srq_qp_node *qp_node = NULL, *tmp_qp_node = NULL;
> +	unsigned long flags;
> +	struct xsc_ib_srq *xsrq;
> +
> +	if (qp->qp_type == IB_QPT_RC) {
> +		if (xqp->recv_cq) {
> +			spin_lock_irq(&to_xcq(xqp->recv_cq)->err_state_qp_lock);
> +			list_for_each_entry_safe(err_rq_node, tmp,
> +						 &to_xcq(xqp->recv_cq)->err_state_qp_list, entry) {
> +				if (err_rq_node->qp_id == xqp->xqp.qpn && !err_rq_node->is_sq) {
> +					list_del(&err_rq_node->entry);
> +					kfree(err_rq_node);
> +				}
> +			}
> +			spin_unlock_irq(&to_xcq(xqp->recv_cq)->err_state_qp_lock);
> +		}
> +
> +		if (xqp->send_cq) {
> +			spin_lock_irq(&to_xcq(xqp->send_cq)->err_state_qp_lock);
> +			list_for_each_entry_safe(err_sq_node, tmp,
> +						 &to_xcq(xqp->send_cq)->err_state_qp_list, entry) {
> +				if (err_sq_node->qp_id == xqp->xqp.qpn && err_sq_node->is_sq) {
> +					list_del(&err_sq_node->entry);
> +					kfree(err_sq_node);
> +				}
> +			}
> +			spin_unlock_irq(&to_xcq(xqp->send_cq)->err_state_qp_lock);
> +		}
> +	}

[Severity: High]
Can xsc_ib_destroy_qp() leak err_state_qp_node entries or race with concurrent
CQ polling when cleaning err_state_qp_list?

First, xsc_ib_modify_qp() and xsc_poll_one() call xsc_ib_err_state_qp() for
any kernel QP transitioning to XSC_QP_STATE_ERR regardless of qp_type, but
xsc_ib_destroy_qp() only purges err_state_qp_list when qp->qp_type ==
IB_QPT_RC. If a non-RC kernel QP (such as IB_QPT_GSI, IB_QPT_SMI, or
IB_QPT_RAW_PACKET) enters IB_QPS_ERR and is destroyed, its err_rq_node and
err_sq_node entries remain on the CQs' err_state_qp_list.

Second, even for IB_QPT_RC QPs, xsc_ib_destroy_qp() purges err_state_qp_list
before destroy_qp_common() moves the hardware QP to XSC_QP_STATE_RST and
cleans the CQs under xsc_ib_lock_cqs(). While xsc_core_qp_modify() is
sleeping in destroy_qp_common(), a concurrent xsc_ib_poll_cq() on another CPU
can poll an error CQE for this QP and call xsc_ib_err_state_qp(), inserting
new nodes onto err_state_qp_list after this cleanup loop has already run, and
xsc_generate_flush_err_cqe() walks err_state_qp_list without holding cq->lock
while destroy_qp_kernel() frees the QP work queue buffers.

Should the err_state_qp_list removal happen for all kernel QP types inside
destroy_qp_common() while holding the CQ locks after the QP has been moved to
RESET?

[ ... ]
> +static inline void xsc_set_qp_access_flag(struct xsc_modify_qp_mbox_in *in,
> +					  struct xsc_qp_context *context, int access_flags)
> +{
> +	in->hdr.ver = cpu_to_be16(XSC_QP_CONTEXT_V1);
> +	context->qp_access_flags = 0;
[ ... ]
> +static int __xsc_ib_modify_qp(struct ib_qp *ibqp,
> +			      const struct ib_qp_attr *attr, int attr_mask,
> +			      enum ib_qp_state cur_state, enum ib_qp_state new_state,
> +			      const struct xsc_ib_modify_qp *ucmd,
> +			      struct ib_udata *udata)
> +{
> +	struct xsc_ib_dev *dev = to_xdev(ibqp->device);
> +	struct xsc_ib_qp *qp = to_xqp(ibqp);
> +	struct xsc_ib_cq *send_cq, *recv_cq;
> +	struct xsc_qp_context *context;
> +	struct xsc_modify_qp_mbox_in *in;
> +	struct xsc_qp_path path = {};
> +	int err;
> +	char buf[256] = {0};
> +	char *ptr = buf;
> +	int ret = 0;
> +	struct xsc_core_device *xdev = dev->xdev;
> +
> +	in = kzalloc_obj(*in, GFP_KERNEL);
> +	if (!in)
> +		return -ENOMEM;
> +
> +	context = &qp->ctx;

[Severity: Medium]
In __xsc_ib_modify_qp(), context points directly to &qp->ctx and mutates the
cached context in place before xsc_set_path() or xsc_core_qp_modify()
succeeds, and transitioning to IB_QPS_RESET never clears stale fields in
qp->ctx. In addition, xsc_set_qp_access_flag() sets in->hdr.ver =
cpu_to_be16(XSC_QP_CONTEXT_V1) only when IB_QP_ACCESS_FLAGS is present in
attr_mask, so on subsequent transitions where IB_QP_ACCESS_FLAGS is omitted,
in->hdr.ver remains 0 even though the cached context->qp_access_flags is
copied into in->ctx.

Also, in create_qp_common(), in->req.page_shift is set to PAGE_SHIFT even
though create_user_qp() and create_kernel_qp() always compute hw_npages
(in->req.pa_num) and populate in->req.pas[] in 4 KB units (XSC_PAGE_SIZE_4K /
XSC_PAGE_SHIFT_4K). On architectures with 64 KB pages (PAGE_SHIFT == 16),
won't passing PAGE_SHIFT to firmware mismatch the 4 KB PAS entries?

[ ... ]
> +	/*
> +	 * If we moved a kernel QP to RESET, clean up all old CQ
> +	 * entries and reinitialize the QP.
> +	 */
> +	if (new_state == IB_QPS_RESET && !ibqp->uobject) {
> +		get_cqs(qp->xqp.qp_type, qp->ibqp.send_cq, qp->ibqp.recv_cq, &send_cq, &recv_cq);
> +		xsc_ib_cq_clean(recv_cq, qp->xqp.qpn);
> +		if (send_cq != recv_cq)
> +			xsc_ib_cq_clean(send_cq, qp->xqp.qpn);
> +
> +		qp->rq.head = 0;
> +		qp->rq.tail = 0;
> +		qp->sq.head = 0;
> +		qp->sq.tail = 0;
> +		qp->sq.cur_post = 0;
> +		qp->sq.last_poll = 0;
> +		qp->sq.last_comp_ce_wqe = -1;
> +		qp->sq.last_post_ce_wqe = -1;
> +		qp->has_trig_cq_comp_handle = false;
> +	}
[ ... ]
> +	err = __xsc_ib_modify_qp(ibqp, attr, attr_mask, cur_state, new_state, &ucmd, udata);
> +
> +	if (!err && (attr_mask & IB_QP_STATE) && !udata) {
> +		bool trigger;
> +
> +		get_cqs(qp->xqp.qp_type, qp->send_cq, qp->recv_cq, &send_cq, &recv_cq);
> +		xsc_ib_lock_cqs(send_cq, recv_cq);
> +		err = xsc_ib_err_state_qp(dev, qp, to_xsc_state(cur_state),
> +					  to_xsc_state(new_state), &trigger);
> +		xsc_ib_unlock_cqs(send_cq, recv_cq);
> +
> +		if (!err && trigger)
> +			xsc_ib_trigger_cq_comp_handler(qp->send_cq, qp->recv_cq);
> +	}

[Severity: High]
When a kernel QP enters error state via a hardware error CQE in
xsc_poll_one(), xsc_ib_err_state_qp() queues err_rq_node and err_sq_node onto
the CQs' err_state_qp_list and sets qp->has_trig_cq_comp_handle = true, but
qp->state remains at its previous software state (such as IB_QPS_RTS).

When xsc_ib_modify_qp() later resets the QP to IB_QPS_RESET, cur_state is
taken from qp->state (not IB_QPS_ERR), so the removal branch in
xsc_ib_err_state_qp() (cur_state == XSC_QP_STATE_ERR && state !=
XSC_QP_STATE_ERR) is not taken. Because __xsc_ib_modify_qp() also resets
qp->has_trig_cq_comp_handle = false, won't the existing err_rq_node and
err_sq_node be leaked on err_state_qp_list and duplicated if the QP enters
error state again?

Furthermore, in __xsc_ib_modify_qp(), qp->rq and qp->sq indices are zeroed
without holding cq->lock, qp->rq.lock, or qp->sq.lock before
xsc_ib_err_state_qp() removes the QP from err_state_qp_list, and
qp->rq.flush_wqe_cnt, qp->sq.flush_wqe_cnt, and qp->sq.need_flush[] are not
cleared. Can a concurrent xsc_ib_poll_cq() running
xsc_generate_flush_err_cqe() race with this reset?

> +
> +	if (!err && !udata && (attr_mask & IB_QP_STATE) && ibqp->srq &&
> +	    cur_state == IB_QPS_RESET && new_state == IB_QPS_INIT) {
> +		srq = to_xsrq(ibqp->srq);
> +		spin_lock_irqsave(&srq->lock, flags);
> +		srq->qp_num++;
> +		wr_num = XSC_SRQ_MIN_WR_NUM;
> +		ibdev_dbg(&dev->ib_dev,
> +			  "SRQ %d: qp %d to INIT, post %d srq wrs, qp_num %d\n",
> +			   srq->xsrq.srqn, qp->xqp.qpn, wr_num, srq->qp_num);
> +		xsc_post_cache_wrs(srq, qp, wr_num, true);
> +		spin_unlock_irqrestore(&srq->lock, flags);
> +	}

[Severity: High]
Can kernel QPs attached to an SRQ leak cached WRs and corrupt srq->qp_num
across QP creation, reset, and destruction?

1. In create_qp_common(), a kernel QP with init_attr->srq is added to
   xsrq->xsrq.srq_qp_list while still in IB_QPS_RESET and without
   incrementing xsrq->qp_num. If xsc_ib_post_srq_recv() runs before the QP
   transitions to IB_QPS_INIT, it sees the QP on srq_qp_list with
   flush_wqe_cnt == 0 and calls xsc_post_cache_wrs(srq, qp, wr_num, false),
   posting WQEs and ringing the RX doorbell on a RESET QP. When
   xsc_ib_modify_qp() then transitions the QP from IB_QPS_RESET to
   IB_QPS_INIT, xsc_post_cache_wrs(srq, qp, wr_num, true) overwrites
   qp->srq_head_wr because init is true, leaking the previously attached
   ib_recv_wr and sg_list allocations.

2. When a kernel QP with an SRQ is modified back to IB_QPS_RESET in
   __xsc_ib_modify_qp(), qp->rq.head and qp->rq.tail are zeroed without
   calling xsc_restore_cache_srq_wrs(), resetting qp->rq.flush_wqe_cnt, or
   decrementing srq->qp_num. Transitioning from IB_QPS_RESET to IB_QPS_INIT
   again increments srq->qp_num a second time and overwrites qp->srq_head_wr.
   Conversely, if a QP is destroyed in IB_QPS_RESET before ever reaching
   IB_QPS_INIT, xsc_ib_destroy_qp() decrements srq->qp_num even though it was
   never incremented.

3. In xsc_ib_destroy_qp(), list_empty(&to_xsrq(qp->srq)->xsrq.srq_qp_list) is
   checked outside xsrq->lock (which races with list_sort() in
   xsc_ib_post_srq_recv()), and xsc_restore_cache_srq_wrs() is called before
   destroy_qp_common() transitions the QP to RESET.

[ ... ]
> +int xsc_ib_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *qp_attr, int qp_attr_mask,
> +		    struct ib_qp_init_attr *qp_init_attr)
> +{
> +	struct xsc_ib_dev *dev = to_xdev(ibqp->device);
> +	struct xsc_ib_qp *qp = to_xqp(ibqp);
> +	struct xsc_query_qp_mbox_out *outb;
> +	struct xsc_qp_context *context;
> +	int err = 0;
> +
> +	mutex_lock(&qp->mutex);

[Severity: High]
Does xsc_ib_query_qp() leak uninitialized kernel memory to userspace?

In drivers/infiniband/core/uverbs_cmd.c, ib_uverbs_query_qp() allocates attr
and init_attr with kmalloc_obj() (not kzalloc), and ib_query_qp() only zeroes
the two sgid_attr pointers before calling the driver's query_qp callback.

Because xsc_ib_query_qp() does not memset() qp_attr or qp_init_attr before
populating a subset of fields, uninitialized heap bytes in fields such as
path_mig_state, qkey, qp_access_flags, pkey_index, min_rnr_timer, timeout,
alt_port_num, alt_timeout, ah_attr (including 32 bytes of GRH/dgid data when
the uninitialized ah_flags byte has IB_AH_GRH set), and qp_init_attr are
copied back to userspace in struct ib_uverbs_query_qp_resp.

> +
> +	outb = kzalloc_obj(*outb, GFP_KERNEL);
> +	if (!outb) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	context = &outb->ctx;
> +	err = xsc_core_qp_query(dev->xdev, &qp->xqp, outb, sizeof(*outb));
> +	if (err)
> +		goto out_free;
> +
> +	if (qp->xqp.err_occurred) {
> +		qp->state = IB_QPS_ERR;
> +		qp->xqp.err_occurred = 0;
> +	}
> +	qp_attr->qp_state	     = qp->state;
> +	qp_attr->path_mtu	     = context->mtu_mode ? IB_MTU_4096 : IB_MTU_1024;
> +	qp_attr->rq_psn		     = be32_to_cpu(context->next_recv_psn) & 0xffffff;
> +	qp_attr->sq_psn		     = be32_to_cpu(context->next_send_psn) & 0xffffff;
> +	qp_attr->dest_qp_num	     = be32_to_cpu(context->remote_qpn) & 0xffffff;
> +	qp_attr->sq_draining = 0;
> +	qp_attr->retry_cnt	    = context->retry_cnt;
> +	qp_attr->rnr_retry	    = context->rnr_retry;
> +	qp_attr->cur_qp_state	     = qp_attr->qp_state;
> +	qp_attr->cap.max_recv_wr     = qp->rq.wqe_cnt;
> +	qp_attr->cap.max_recv_sge    = qp->rq.max_gs;
> +	qp_attr->max_rd_atomic = context->max_sra;
> +	qp_attr->max_dest_rd_atomic = context->max_rra;
> +	qp_attr->port_num = 1;
> +
> +	if (!ibqp->uobject) {
> +		qp_attr->cap.max_send_wr  = qp->sq.wqe_cnt;
> +		qp_attr->cap.max_send_sge = qp->sq.max_gs;
> +	} else {
> +		qp_attr->cap.max_send_wr  = qp->sq.wqe_cnt;
> +		qp_attr->cap.max_send_sge = qp->sq.max_gs;
> +	}

[Severity: Medium]
In __xsc_ib_modify_qp(), context->mtu_mode is set to 0 for IB_MTU_1024, 1 for
IB_MTU_4096, and 2 for IB_MTU_8192, but xsc_ib_query_qp() decodes
context->mtu_mode ? IB_MTU_4096 : IB_MTU_1024, reporting IB_MTU_4096 when
mtu_mode is 2. Should xsc_ib_query_qp() map mtu_mode == 2 to IB_MTU_8192?

Also, both branches of if (!ibqp->uobject) in xsc_ib_query_qp() are
identical, and the static helpers to_ib_qp_state() and to_ib_mig_state()
defined just above xsc_ib_query_qp() are unused.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=13