Re: [PATCH 13/15] yunsilicon: add the RDMA send path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [Critical] rdma: inline data payload overwrites remote address segment in RDMA Read/Write
- [Critical] rdma: hardware DMA buffer overflow due to overriding SGE length with maximum MTU
- [High] rdma: silent drop of remaining WRs in chain on zero-length RDMA READ
- [High] rdma: stack use-after-free in xsc_ib_drain_rq_done and xsc_ib_drain_sq_done
- [High] rdma: stack out-of-bounds read and kernel panic during UD QP drain
- [High] rdma: unaligned 32-bit pointer assignment causes kernel panics
- [High] rdma: uninitialized receive WQE segments leave hardware exposed to stale physical addresses
- [High] rdma: early return in QP drain routines causes Use-After-Free by breaking synchronous drain contract
- [High] rdma: set_data_inl_seg buffer overflow and RAddr segment corruption
- [High] rdma/xsc: buffer overflow and data corruption in set_data_inl_seg for multiple inline SGEs
- [High] xsc: wild pointer dereference in xsc_ib_drain_sq() for UD QPs
- [High] xsc: silent data loss when posting zero-length RDMA READ in a chain
- [Medium] rdma: unaligned multi-byte memory accesses on stack array in xsc_icrc_hdr
- [Medium] rdma: read-modify-write on DMA memory and uninitialized WQE control fields
- [Medium] rdma: xsc_icrc_hdr unconditionally adds UDP header size, breaking RoCEv1 packets
- [Medium] rdma/xsc: QP drain routines deadlock on IB_POLL_DIRECT CQs
- [Low] rdma: xsc_icrc_hdr returns negative error code for unsigned return type
- [Low] xsc: global namespace pollution by redefining struct rxe_bth
--

commit fdfdd79a8a3fbba8cb02ffc2535c12c0197fac66
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add the RDMA send path

This commit introduces the RDMA send path for the Yunsilicon XSC driver. 
It implements the work request processing for various operation types, 
QP1 MAD header construction, doorbell updates, and completion queue drains.

> diff --git a/drivers/infiniband/hw/xsc/qp.c b/drivers/infiniband/hw/xsc/qp.c
> index 6a83adc9c833f..9c79e09a7fd1f 100644
> --- a/drivers/infiniband/hw/xsc/qp.c
> +++ b/drivers/infiniband/hw/xsc/qp.c
[ ... ]
> +static int set_data_inl_seg(struct xsc_ib_qp *qp, const struct ib_send_wr *wr, void *ctrl)
> +{
> +	struct xsc_wqe_data_seg *data_seg;
> +	unsigned int seg_index;
> +	void *addr;
> +	int len;
> +	int i;
> +
> +	for (i = 0, seg_index = 1; i < wr->num_sge; ++i, ++seg_index) {
> +		if (likely(wr->sg_list[i].length)) {
> +			addr = (void *)wr->sg_list[i].addr;
> +			len  = wr->sg_list[i].length;
> +
> +			if (unlikely(len > qp->max_inline_data))
> +				return -ENOMEM;
> +
> +			data_seg = get_seg_wqe(ctrl, seg_index);
> +			data_seg->in_line = 1;
> +			data_seg->len = len;
> +			memcpy(data_seg->in_line_data, addr, len);
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Critical]
By hardcoding seg_index = 1 here, does this overwrite the remote address 
(RAddr) segment for RDMA Write and Read requests? xsc_ib_post_send appears 
to place the RAddr at seg_index = 1 right before invoking this inline data 
logic.

Additionally, since seg_index is only incremented by 1 per SGE, what happens 
if an inline SGE is larger than 16 bytes? The memcpy will spill across 
multiple hardware WQE segments, but the loop will write the next SGE's data 
at seg_index + 1, overwriting the tail of the previous SGE's payload. 

Finally, the bounds check len > qp->max_inline_data only checks each SGE 
individually. If wr->num_sge > 1, does the sum of the SGE lengths overflow 
the pre-allocated WQE slot boundary and corrupt adjacent WQEs in the SQ 
ring buffer?

[ ... ]
> +static u32 xsc_icrc_hdr(struct xsc_ib_dev *dev, void *pkt, u32 size, u32 *icrc)
> +{
> +	struct iphdr *ip4h = NULL;
> +	struct ipv6hdr *ip6h = NULL;
> +	struct udphdr *udph;
> +	struct ib_unpacked_eth *eth;
> +	struct rxe_bth *bth;
> +	struct ib_unpacked_deth *deth;
> +	struct ib_unpacked_vlan *vlan;
> +	int crc;
> +	int crc_field_len;
> +	__be16 l3_type;
> +	u8 *l3_start;
> +
> +	int hdr_size;
> +
> +	/* pseudo header buffer size is calculate using ipv6 header size since
> +	 * it is bigger than ipv4
> +	 */
> +	u8 pshdr[sizeof(struct udphdr) +
> +		sizeof(struct ipv6hdr) +
> +		sizeof(*bth) + sizeof(*deth)];

[Severity: Medium]
Since pshdr is allocated as a byte array (u8) on the stack, it is only 
guaranteed to be 1-byte aligned. Does casting this buffer to struct iphdr * 
and struct rxe_bth * and performing multi-byte writes trigger alignment 
traps on architectures with strict memory alignment requirements?

[Severity: Low]
This function internally uses struct rxe_bth and struct rxe_deth. Does 
using another software driver's prefix (rxe_) pollute the global structure 
namespace and cause BTF deduplication conflicts? Should it use standard 
RDMA core structures like ib_unpacked_bth instead?

> +
> +	eth = pkt;
> +
> +	if (eth->type == htons(ETH_P_8021Q)) {
> +		vlan = (struct ib_unpacked_vlan *)(eth + 1);
> +		l3_type = vlan->type;
> +		l3_start = (u8 *)(vlan + 1);
> +		size -= 4;
> +	} else {
> +		l3_type = eth->type;
> +		l3_start = (u8 *)(eth + 1);
> +	}
> +
> +	hdr_size = sizeof(struct udphdr) +
> +		(l3_type == htons(ETH_P_IP) ?
> +		sizeof(struct iphdr) : sizeof(struct ipv6hdr));
> +
> +	crc_field_len = hdr_size + sizeof(*bth) + sizeof(*deth);
> +
> +	if (crc_field_len != size) {
> +		ibdev_err(&dev->ib_dev, "Unmatched hdr: expect %d actual %d\n",
> +			  crc_field_len, size);
> +		return -EINVAL;
> +	}

[Severity: Low]
Does returning -EINVAL from a function returning u32 implicitly cast the 
error code to a large positive integer, making it difficult for callers 
to properly check for errors?

[Severity: Medium]
Does hdr_size unconditionally adding sizeof(struct udphdr) break support 
for RoCEv1 (IBoE) packets? For RoCEv1, there is no UDP header, meaning 
crc_field_len != size will always evaluate to true, rejecting the packet.

[ ... ]
> +static void zero_send_ds(struct xsc_ib_qp *qp, int idx)
> +{
> +	void *seg;
> +	int i;
> +	int ds_num;
> +	u64 *p;
> +
> +	ds_num = XSC_SEND_SEG_NUM << (qp->sq.wqe_shift - XSC_SEND_WQE_SHIFT);
> +	seg = (void *)xsc_get_send_wqe(qp, idx);
> +	for (i = 1; i < ds_num; i++) {
> +		p = get_seg_wqe(seg, i);
> +		p[0] = 0;
> +		p[1] = 0;
> +	}
> +}

[Severity: Medium]
Since this loop explicitly starts at i = 1, the control segment (i = 0) is 
left uninitialized. Later in xsc_ib_post_send, the driver modifies bitfields 
(ctrl->se, ctrl->ce) on this uninitialized memory. Does this perform unsafe 
read-modify-write operations on DMA-mapped memory and leave unassigned 
fields with stale values from previous work requests?

[ ... ]
> +		if (unlikely(wr->opcode == IB_WR_RDMA_READ && msg_len == 0)) {
> +			ibdev_err(&dev->ib_dev, "rdma read, msg len should not be 0\n");
> +			/* workaround, return success for posting zero-length read */
> +			err = 0;
> +			goto out;
> +		}

[Severity: High]
Does this workaround silently drop the current work request and any remaining 
linked work requests in the wr->next chain? By returning success without 
generating completion entries for the dropped WRs, ULPs will hang 
indefinitely waiting for them. Zero-length RDMA READs are explicitly valid 
in the RDMA specification.

[ ... ]
> +			crc = xsc_crc32(dev, crc, mad_send_base + sg.length, ctrl->msg_len);
> +			ctrl->msg_len += sg.length;
> +			seg_index++;
> +
> +			*(u32 *)&mad_send_base[ctrl->msg_len] = ~crc;

[Severity: High]
Since ctrl->msg_len might not be 4-byte aligned, will this direct pointer 
cast and assignment cause unaligned 32-bit writes? Architectures lacking 
hardware support for unaligned access will panic here. Should this use 
put_unaligned instead?

[ ... ]
> +		for (i = 0; i < wr->num_sge; ++i) {
> +			if (unlikely(!wr->sg_list[i].length))
> +				continue;
> +			data_seg = get_seg_wqe(recv_head, i);
> +			data_seg->in_line = 0;
> +			WR_LE_64(data_seg->va, wr->sg_list[i].addr);
> +			WR_LE_32(data_seg->mkey, wr->sg_list[i].lkey);
> +			if (is_qp1(qp->xqp.qp_type))
> +				WR_LE_32(data_seg->seg_len, xdev->caps.rx_pkt_len_max);
> +			else
> +				WR_LE_32(data_seg->seg_len, wr->sg_list[i].length);
> +		}

[Severity: High]
If an SGE length is 0, the loop skips it via continue, failing to initialize 
the corresponding WQE segment. Because receive WQEs are reused from a ring 
buffer without zeroing, does this leave stale memory keys and physical 
addresses for the hardware to process, potentially DMAing incoming data 
into arbitrary memory?

[Severity: Critical]
For QP1 (GSI), the driver unconditionally overrides the consumer-provided 
buffer length (wr->sg_list[i].length) with the global max packet length 
xdev->caps.rx_pkt_len_max. Does this instruct the hardware that it can DMA 
data far beyond the actual bounds of the software-allocated scatter/gather 
buffer, causing memory corruption?

[ ... ]
> +static void xsc_ib_drain_rq_done(struct ib_cq *cq, struct ib_wc *wc)
> +{
> +	struct xsc_ib_drain_cqe *cqe = container_of(wc->wr_cqe,
> +						     struct xsc_ib_drain_cqe, cqe);
> +	struct xsc_ib_dev *dev = to_xdev(cq->device);
> +
> +	complete(&cqe->done);
> +	ibdev_dbg(&dev->ib_dev, "QP %d ib_drain_rq done!", cqe->qpn);
> +}

[Severity: High]
Since cqe is allocated on the stack in xsc_ib_drain_rq, the moment 
complete(&cqe->done) is called, the blocked thread can wake up and destroy 
the stack frame. Does the subsequent access to cqe->qpn dereference a 
potentially freed stack pointer, causing a use-after-free race? The same 
pattern is present in xsc_ib_drain_sq_done.

[ ... ]
> +void xsc_ib_drain_sq(struct ib_qp *qp)
> +{
[ ... ]
> +	const struct ib_send_wr swr = {
> +		.next = NULL,
> +		{ .wr_cqe	= &sdrain.cqe, },
> +		.opcode	= IB_WR_RDMA_WRITE,
> +		.send_flags = IB_SEND_SIGNALED,
> +	};
[ ... ]
> +	ret = xsc_ib_post_send(qp, &swr, &bad_swr);
> +	if (ret) {
> +		ibdev_err(&dev->ib_dev, "failed to drain send queue: %d\n", ret);
> +		return;
> +	}
> +	xqp->post_send_invalid = true;
> +
> +	xcq->xcq.comp(&xcq->xcq);
> +	wait_for_completion(&sdrain.done);
> +}

[Severity: High]
For UD QPs, xsc_ib_post_send unconditionally forwards this dummy WR to 
build_qp1_send_v2. Because swr is allocated purely as a base struct 
ib_send_wr, when build_qp1_send_v2 casts it to struct ib_ud_wr and 
dereferences ud_wr(wr)->ah, won't this read out-of-bounds stack garbage 
and cause a kernel panic?

[Severity: High]
The RDMA API requires the drain functions to block until all in-flight WQEs 
complete. By returning early when xsc_ib_post_send fails, does this break 
the synchronous drain contract, allowing the ULP to free memory while the 
hardware is still asynchronously processing in-flight WQEs?

[Severity: Medium]
If the CQ uses IB_POLL_DIRECT, there is no background task polling it. Will 
wait_for_completion block indefinitely unless a concurrent thread happens to 
poll the CQ? The core RDMA layer fallback handles this by calling 
ib_process_cq_direct(cq, -1) in a loop. Additionally, unconditionally calling 
xcq->xcq.comp(&xcq->xcq) will trigger a warning in ib_cq_completion_direct.

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