On Thu, Feb 2, 2023 1:43 PM Bob Pearson wrote:
>
> Currently the rxe driver does not handle all cases of zero length
> rdma operations correctly. The client does not have to provide an
> rkey for zero length RDMA read or write operations so the rkey
> provided may be invalid and should not be used to lookup an mr.
>
> This patch corrects the driver to ignore the provided rkey if the
> reth length is zero for read or write operations and make sure to
> set the mr to NULL. In read_reply() if length is zero rxe_recheck_mr()
> is not called. Warnings are added in the routines in rxe_mr.c to
> catch NULL MRs when the length is non-zero.
>
<...>
> @@ -432,6 +435,10 @@ int rxe_flush_pmem_iova(struct rxe_mr *mr, u64 iova, unsigned int length)
> int err;
> u8 *va;
>
> + /* mr must be valid even if length is zero */
> + if (WARN_ON(!mr))
> + return -EINVAL;
If 'mr' is NULL, NULL pointer dereference can occur in process_flush()
before reaching here. Isn't it better to do the check in process_flush()?
> +
> if (length == 0)
> return 0;
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index cccf7c6c21e9..c8e7b4ca456b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -420,13 +420,23 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
> return RESPST_CHK_RKEY;
> }
>
> +/* if the reth length field is zero we can assume nothing
> + * about the rkey value and should not validate or use it.
> + * Instead set qp->resp.rkey to 0 which is an invalid rkey
> + * value since the minimum index part is 1.
> + */
> static void qp_resp_from_reth(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
> {
> + unsigned int length = reth_len(pkt);
> +
> qp->resp.va = reth_va(pkt);
> qp->resp.offset = 0;
> - qp->resp.rkey = reth_rkey(pkt);
> - qp->resp.resid = reth_len(pkt);
> - qp->resp.length = reth_len(pkt);
> + qp->resp.resid = length;
> + qp->resp.length = length;
As you know, the comment above this function is applicable only
to RDMA Read and Write. What do you say to mentioning FLUSH
here rather than at the one in rxe_flush_pmem_iova().
Thanks,
Daisuke
> + if (pkt->mask & RXE_READ_OR_WRITE_MASK && length == 0)
> + qp->resp.rkey = 0;
> + else
> + qp->resp.rkey = reth_rkey(pkt);
> }
>
<...>
