The completer reads send WQEs from the same userspace-mapped shared queue as the requester. Even after the requester copies the WQE to a kernel-private buffer (previous patch), the completer still reads directly from shared memory, leaving it exposed to the same TOCTOU races. Fix by copying the WQE in get_wqe() to a completer-private buffer using the full queue element data size. The local copy is reused when the same WQE is being processed with unchanged state, which preserves DMA progress across multi-packet RDMA READ responses. A fresh copy is taken when the state changes or a new WQE appears. After copying, reject invalid opcodes (above IB_WR_REG_MR), out-of- bounds num_sge, and invalid cur_sge by transitioning the QP to error state through COMPST_ERROR. This prevents OOB SGE walks in copy_data() from corrupted DMA indices. The state read from the shared queue is preserved via smp_load_acquire() and restored after the memcpy to prevent a concurrent userspace write from overwriting it with a stale value. DMA progress (resid, cur_sge, sge_offset) is written back alongside completion status and rd_atomic state on completion and before retry invalidation, ensuring retries can resume from the correct offset rather than restarting from zero. The cache is invalidated on QP reset, when a completion advances to the next WQE, on error retry, and on RNR retry to ensure fresh state after the requester retransmits. Fixes: 8700e3e7c485 ("Soft RoCE driver") Cc: stable@vger.kernel.org Signed-off-by: Tristan Madani --- Changes since v7: - Write back DMA progress before invalidating completer cache on error retry, so req_retry() resumes multi-packet reads from the correct offset (sashiko finding). - Invalidate completer cache in COMPST_RNR_RETRY to prevent stale PSN comparison after RNR timer fires (sashiko finding). Changes since v6: - Fixed all sashiko review findings (2 Critical + 9 High). - FORTIFY_SOURCE: memcpy into enclosing struct, not inner member. - Copy size: max(max_sge * sizeof(ib_sge), max_inline). - Opcode bound: IB_WR_REG_MR (was IB_WR_ATOMIC_WRITE). - Validate num_sge + cur_sge, preserve smp_load_acquire state. - DMA progress writeback (resid, cur_sge, sge_offset). - *wqe_p = NULL for posted state, invalidate on retry. - Addressed Zhu Yanjun review feedback. Changes since v5: - Added opcode bounds check and num_sge clamp after copy. Changes since v4: - Resent from the correct sender address. Changes since v3: - Copy full queue element (max_sge SGEs) instead of per-WQE size. - Invalidate on QP reset, ERR flush, RESET state check. Changes since v2: - WRITE_ONCE/smp_store_release for field writeback. Changes since v1: - Split into per-path patches. Cache across multi-packet ops. drivers/infiniband/sw/rxe/rxe_comp.c | 75 ++++++++++++++++++++++++++- drivers/infiniband/sw/rxe/rxe_qp.c | 1 + drivers/infiniband/sw/rxe/rxe_verbs.h | 6 +++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c index 1390e861bd1d7..b8bc710dc75e9 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); +} + static inline enum comp_state get_wqe(struct rxe_qp *qp, struct rxe_pkt_info *pkt, struct rxe_send_wqe **wqe_p) { struct rxe_send_wqe *wqe; + u32 state; /* we come here whether or not we found a response packet to see if * there are any posted WQEs */ wqe = queue_head(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT); - *wqe_p = wqe; /* no WQE or requester has not started it yet */ - if (!wqe || wqe->state == wqe_state_posted) + if (!wqe) { + *wqe_p = NULL; return pkt ? COMPST_DONE : COMPST_EXIT; + } + + /* Pairs with smp_store_release() in rxe_req_writeback_wqe() */ + state = smp_load_acquire(&wqe->state); + if (state == wqe_state_posted) { + *wqe_p = NULL; + return pkt ? COMPST_DONE : COMPST_EXIT; + } + + /* Reuse existing local copy if still processing same WQE + * with unchanged state (preserves DMA progress for multi-packet ops) + */ + 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; + } + + /* Copy shared WQE to kernel-private buffer. + * Copy size matches the queue element data area. + */ + qp->comp.shared_wqe = wqe; + memcpy(&qp->comp.comp_wqe, wqe, + sizeof(*wqe) + max_t(int, + qp->sq.max_sge * sizeof(struct ib_sge), + qp->sq.max_inline)); + qp->comp.comp_wqe_valid = true; + + /* Preserve the state we observed via smp_load_acquire() + * since memcpy may have overwritten it with a stale read. + */ + qp->comp.comp_wqe.wqe.state = state; + + /* Reject corrupted opcode or SGE indices, error the QP */ + if (unlikely(qp->comp.comp_wqe.wqe.wr.opcode > IB_WR_REG_MR) || + qp->comp.comp_wqe.wqe.dma.num_sge > qp->sq.max_sge || + (qp->comp.comp_wqe.wqe.dma.resid && + qp->comp.comp_wqe.wqe.dma.cur_sge >= qp->sq.max_sge)) { + qp->comp.comp_wqe.wqe.status = IB_WC_LOC_QP_OP_ERR; + wqe = &qp->comp.comp_wqe.wqe; + *wqe_p = wqe; + return COMPST_ERROR; + } + + wqe = &qp->comp.comp_wqe.wqe; + *wqe_p = wqe; +check_state: /* WQE does not require an ack */ if (wqe->state == wqe_state_done) return COMPST_COMP_WQE; @@ -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,8 @@ int rxe_completer(struct rxe_qp *qp) RXE_CNT_COMP_RETRY); qp->req.need_retry = 1; qp->comp.started_retry = 1; + rxe_comp_writeback_wqe(qp); + qp->comp.comp_wqe_valid = false; qp->req.again = 1; } goto done; @@ -809,6 +878,8 @@ int rxe_completer(struct rxe_qp *qp) * rnr timer has fired */ qp->req.wait_for_rnr_timer = 1; + rxe_comp_writeback_wqe(qp); + qp->comp.comp_wqe_valid = false; rxe_dbg_qp(qp, "set rnr nak timer\n"); // TODO who protects from destroy_qp?? mod_timer(&qp->rnr_nak_timer, diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c index 77606c4a039b1..450254a131017 100644 --- a/drivers/infiniband/sw/rxe/rxe_qp.c +++ b/drivers/infiniband/sw/rxe/rxe_qp.c @@ -582,6 +582,7 @@ static void rxe_qp_reset(struct rxe_qp *qp) qp->req.wait_for_rnr_timer = 0; qp->req.noack_pkts = 0; qp->req.send_wqe_valid = false; + qp->comp.comp_wqe_valid = false; qp->resp.msn = 0; qp->resp.opcode = -1; qp->resp.drop_msg = 0; diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h index a22dfc6e5ae3c..6205dbc29dff6 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.h +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h @@ -130,6 +130,12 @@ struct rxe_comp_info { int started_retry; u32 retry_cnt; u32 rnr_retry; + struct rxe_send_wqe *shared_wqe; + bool comp_wqe_valid; + struct { + struct rxe_send_wqe wqe; + struct ib_sge sge[RXE_MAX_SGE]; + } comp_wqe; }; /* responder states */ -- 2.47.3