Re: [PATCH v2] 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] rxe: replay of completed MW-based RDMA READ fails unconditionally
- [High] RDMA/rxe: spurious MW bounds check failure on RDMA Read retry
--

commit 75bedbaf8f14c7951915e41ef579bcbb28d9e507
Author: sung byeongchan <tjdqudcks0424@naver.com>
Subject: RDMA/rxe: enforce memory window ranges

This commit enforces memory window ranges by validating the window and taking
a reference on its backing MR while holding the lock. It ensures requests stay
within the delegated interval and requires later packets or replays to resolve
to the same MR and interval. The changes aim to prevent unauthorized access to
userspace memory outside the bounds of the delegated memory window.

> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index ba3b2d23b36b7..19eb60c317d00 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
[ ... ]
> @@ -892,27 +900,33 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
>   *
>   * Return: mr on success else NULL
>   */
> -static struct rxe_mr *rxe_recheck_mr(struct rxe_qp *qp, u32 rkey)
> +static struct rxe_mr *rxe_recheck_mr(struct rxe_qp *qp,
> +				     const struct resp_res *res)
>  {
>  	struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
> +	u32 rkey = res->read.rkey;
>  	struct rxe_mr *mr;
> -	struct rxe_mw *mw;
>  
>  	if (rkey_is_mw(rkey)) {
> -		mw = rxe_pool_get_index(&rxe->mw_pool, rkey >> 8);
> -		if (!mw)
> +		u64 addr, length;
> +		int access;
> +
> +		mr = rxe_lookup_mw(qp, IB_ACCESS_REMOTE_READ, rkey,
> +				   &addr, &length, &access);
> +		if (!mr)
>  			return NULL;
>  
> -		mr = mw->mr;
> -		if (mw->rkey != rkey || mw->state != RXE_MW_STATE_VALID ||
> -		    !mr || mr->state != RXE_MR_STATE_VALID) {
> -			rxe_put(mw);
> +		/* A replay or a later response packet must still refer to the
> +		 * same backing MR and the entire originally authorized interval.
> +		 * This catches invalidate/rebind even when the object survives.
> +		 */
> +		if (mr != res->read.mr || res->read.va_org < addr ||
> +		    res->read.length > length ||
> +		    res->read.va_org - addr > length - res->read.length) {
> +			rxe_put(mr);
>  			return NULL;
>  		}

[Severity: High]
Does this strict pointer comparison fail when a completed read is replayed?

When a Memory Window based RDMA READ completes normally, read_reply() drops the
MR reference and clears the pointer:

    if (res->read.mr) {
        rxe_put(res->read.mr);
        res->read.mr = NULL;
    }

If the final completion ACK is lost, the requester will time out and retransmit
the request, causing the responder to replay the READ response. During this
replay, read_reply() calls rxe_recheck_mr() to re-validate the backing MR.

Because res->read.mr was cleared to NULL upon completion, but rxe_lookup_mw()
returns a valid mr pointer, the condition mr != res->read.mr evaluates to true.
This causes validation to fail and returns NULL, leading to a spurious
RESPST_ERR_RKEY_VIOLATION that terminates the connection.

The direct MR check later in this same function safely handles the NULL state
via (res->read.mr && mr != res->read.mr). Should a similar check be used here?


[Severity: High]
This problem wasn't introduced by this patch, but does the new bounds check
cause a spurious failure during an RDMA Read retry?

The existing duplicate_request() function handles subset retries by advancing
the origin virtual address without modifying the read length:

    duplicate_request()
        ...
        /* Reset the resource, except length. */
        res->read.va_org = iova;
        res->read.va = iova;
        res->read.resid = resid;

Because va_org is advanced but read.length is not proportionally reduced, the
computed end of the request exceeds the MW boundaries when evaluated by the new
bounds check. Can this result in a spurious rkey violation on valid subset
retries that span to the end of the MW, incorrectly tearing down the connection?

>  
> -		rxe_get(mr);
> -		rxe_put(mw);
> -
>  		return mr;
>  	}

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