Re: [PATCH v7 2/2] RDMA/rxe: copy send WQE to kernel buffer in completer path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RDMA/rxe: missing rxe_comp_writeback_wqe() before retry loses READ progress
- [High] RDMA/rxe: missing comp_wqe_valid invalidation in COMPST_RNR_RETRY
--

commit 7163a14999e322c25c58445d22fa6ea5241151a2
Author: Tristan Madani <tristmd@gmail.com>

RDMA/rxe: copy send WQE to kernel buffer in completer path

This commit copies the send WQE from userspace-mapped shared memory into a
completer-private kernel buffer in get_wqe() to prevent TOCTOU races, while
caching the copy across multi-packet responses and writing back modified WQE
fields on completion.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..cd4085a1bf2f8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -137,22 +137,87 @@ void rxe_comp_queue_pkt(struct rxe_qp *qp, struct sk_buff *skb)
>  	rxe_sched_task(&qp->send_task);
>  }
>  
> +/* Write back completer WQE fields to shared memory */
> +static void rxe_comp_writeback_wqe(struct rxe_qp *qp)
> +{
> +	struct rxe_send_wqe *shared = qp->comp.shared_wqe;
> +	struct rxe_send_wqe *local = &qp->comp.comp_wqe.wqe;
> +
> +	if (!qp->comp.comp_wqe_valid || !shared)
> +		return;
> +
> +	WRITE_ONCE(shared->status, local->status);
> +	WRITE_ONCE(shared->has_rd_atomic, local->has_rd_atomic);
> +	WRITE_ONCE(shared->dma.resid, local->dma.resid);
> +	WRITE_ONCE(shared->dma.cur_sge, local->dma.cur_sge);
> +	WRITE_ONCE(shared->dma.sge_offset, local->dma.sge_offset);
> +}

[ ... ]

> @@ -454,6 +519,8 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
>  	if (post)
>  		make_send_cqe(qp, wqe, &cqe);
>  
> +	rxe_comp_writeback_wqe(qp);
> +	qp->comp.comp_wqe_valid = false;
>  	queue_advance_consumer(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT);
>  
>  	if (post)
> @@ -788,6 +855,7 @@ int rxe_completer(struct rxe_qp *qp)
>  							RXE_CNT_COMP_RETRY);
>  					qp->req.need_retry = 1;
>  					qp->comp.started_retry = 1;
> +					qp->comp.comp_wqe_valid = false;
>  					qp->req.again = 1;
>  				}
>  				goto done;

[Severity: High]
As reported by Sashiko on v6 [1], does invalidating qp->comp.comp_wqe_valid
in COMPST_ERROR_RETRY without calling rxe_comp_writeback_wqe(qp) first lose
DMA progress for partially completed multi-packet RDMA read operations?

During a multi-packet RDMA read, rxe_requester() sends a request packet with
payload == 0 and leaves shared_wqe->dma.resid equal to wqe->dma.length in
qp->sq.queue. As non-final read response packets arrive in rxe_completer(),
do_read() calls copy_data(), advancing wqe->dma.resid, wqe->dma.cur_sge, and
wqe->dma.sge_offset in the local qp->comp.comp_wqe.wqe copy, and returns
COMPST_UPDATE_COMP:

drivers/infiniband/sw/rxe/rxe_comp.c:do_read() {
	ret = copy_data(qp->pd, IB_ACCESS_LOCAL_WRITE,
			&wqe->dma, payload_addr(pkt),
			payload_size(pkt), RXE_TO_MR_OBJ);
	if (ret) {
		wqe->status = IB_WC_LOC_PROT_ERR;
		return COMPST_ERROR;
	}

	if (wqe->dma.resid == 0 && (pkt->mask & RXE_END_MASK))
		return COMPST_COMP_ACK;

	return COMPST_UPDATE_COMP;
}

Because rxe_comp_writeback_wqe() is only called in do_complete() when the WQE
finishes and is popped from the send queue, clearing qp->comp.comp_wqe_valid
in COMPST_ERROR_RETRY discards the updated local DMA state before req_retry()
runs on the shared WQE:

drivers/infiniband/sw/rxe/rxe_req.c:req_retry() {
			if (mask & WR_READ_MASK) {
				npsn = (wqe->dma.length - wqe->dma.resid) /
					qp->mtu;
				wqe->iova += npsn * qp->mtu;
			}
}

With shared_wqe->dma.resid still equal to wqe->dma.length, npsn evaluates to
0 and the retried read restarts from offset 0 with cur_sge == 0 and
sge_offset == 0 instead of resuming from the partial offset. Should
rxe_comp_writeback_wqe(qp) be called before clearing comp_wqe_valid on retry
(or after copy_data() in do_read())?

[Severity: High]
As reported by Sashiko on v6 [2], what happens in rxe_completer() when an RNR
NAK triggers COMPST_RNR_RETRY right below this hunk?

drivers/infiniband/sw/rxe/rxe_comp.c:rxe_completer() {
		case COMPST_RNR_RETRY:
			/* we come here if we received an RNR NAK */
			if (qp->comp.rnr_retry > 0) {
				if (qp->comp.rnr_retry != 7)
					qp->comp.rnr_retry--;

				/* don't start a retry flow until the
				 * rnr timer has fired
				 */
				qp->req.wait_for_rnr_timer = 1;
				rxe_dbg_qp(qp, "set rnr nak timer\n");
				// TODO who protects from destroy_qp??
				mod_timer(&qp->rnr_nak_timer,
					  jiffies + rnrnak_jiffies(aeth_syn(pkt)
						& ~AETH_TYPE_MASK));
				goto exit;
			}
}

Unlike COMPST_ERROR_RETRY, COMPST_RNR_RETRY does not clear
qp->comp.comp_wqe_valid, and when rnr_nak_timer expires and rxe_requester()
calls req_retry(), only qp->req.send_wqe_valid is cleared. Once
rxe_requester() retransmits the WQE and sets shared_wqe->state back to
wqe_state_pending with updated first_psn and last_psn values, won't get_wqe()
see:

	if (qp->comp.comp_wqe_valid && qp->comp.shared_wqe == wqe &&
	    qp->comp.comp_wqe.wqe.state == state) {
		wqe = &qp->comp.comp_wqe.wqe;
		*wqe_p = wqe;
		goto check_state;
	}

and reuse the stale pre-retry qp->comp.comp_wqe copy, causing check_ack() to
compare the retried response against stale first_psn and last_psn values and
fail with COMPST_ERROR?

[1] https://lore.kernel.org/r/20261026001405.1698455-3-tristan@talencesecurity.com
[2] https://lore.kernel.org/r/20261026001405.1698455-3-tristan@talencesecurity.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030151558.1458444-1-tristan@talencesecurity.com?part=2