Re: [PATCH] RDMA/rxe: enforce memory window ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rdma/rxe: Data race and read tearing on MW properties bypasses bounds check
- [Medium] rxe: false R_Key violations for IB_FLUSH_MR packets using MWs
--

commit b3dae41fdb80e5db85c15f3370a6417776daa69e
Author: sungbyeongchan <tjdqudcks0424@naver.com>

RDMA/rxe: enforce memory window ranges

RXE records the address and length authorized by an MW bind, but the responder
does not compare a remote request with that interval. This commit checks the
complete request extent against the MW interval before selecting the backing MR.

> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index ba3b2d23b36b7..e5b6c461665c4 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -522,6 +522,8 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
>  	pktlen	= payload_size(pkt);
>  
>  	if (rkey_is_mw(rkey)) {
> +		u64 mw_start;
> +
>  		mw = rxe_lookup_mw(qp, access, rkey);
>  		if (!mw) {
>  			rxe_dbg_qp(qp, "no MW matches rkey %#x\n", rkey);
> @@ -529,6 +531,14 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
>  			goto err;
>  		}
>  
> +		mw_start = (mw->access & IB_ZERO_BASED) ? 0 : mw->addr;
> +		if (unlikely(va < mw_start || resid > mw->length ||
> +			     va - mw_start > mw->length - resid)) {

[Severity: High]
Does this bounds check introduce a data race?

Since mw->addr and mw->length are 64-bit fields, can they be modified
concurrently by rxe_do_bind_mw() or rxe_do_invalidate_mw() running
independently in the requester tasklet?

Without holding mw->lock or using memory barriers, could this lead to read
tearing on 32-bit architectures?

Even on 64-bit architectures, this might allow the CPU to observe inconsistent
states (like an old rkey but a newly inflated length), which could defeat this
new bounds check.

[Severity: Medium]
Will this falsely reject IB_FLUSH_MR packets?

For an IB_FLUSH_MR request specifying an MW R_Key, the packet lacks a RETH
header, so the responder does not update qp->resp.va or qp->resp.resid.

Because this new strict MW bounds check runs before the existing FLUSH logic,
doesn't it evaluate those uninitialized or stale va and resid values against
the MW boundaries?

This would falsely trigger an out-of-bounds error and drop valid flush
requests.

Furthermore, if the stale values happen to fall within the MW bounds, wouldn't
the operation skip mr_check_range() based on the existing FLUSH logic further
down in the function, and improperly flush the entire backing MR rather than
confining the flush to the MW?

check_rkey() {
    ...
	if (pkt->mask & RXE_FLUSH_MASK) {
		/* FLUSH MR may not set va or resid
		 * no need to check range since we will flush whole mr
		 */
		if (feth_sel(pkt) == IB_FLUSH_MR)
			goto skip_check_range;
	}
    ...
}

Should the FLUSH check happen before evaluating the MW bounds?

> +			rxe_dbg_qp(qp, "request outside MW range\n");
> +			state = get_rkey_violation_state(pkt);
> +			goto err;
> +		}
> +
>  		mr = mw->mr;
>  		if (!mr) {
>  			rxe_dbg_qp(qp, "MW doesn't have an MR\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006183919.1304642-1-tjdqudcks0424@naver.com?part=1