On Sat, Feb 4, 2023 3:47 AM Bob Pearson wrote:
>
> On 2/3/23 04:00, Daisuke Matsuda (Fujitsu) wrote:
> > 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.
> >>
The change looks good. I will leave it to you whether to fix the comment.
Reviewed-by: Daisuke Matsuda <matsuda-daisuke@xxxxxxxxxxx>
Thanks,
Daisuke
> >
> > <...>
> >
> >> @@ -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);
> >> }
> >>
> >
> > <...>
> >
> Daisuke,
>
> The updated patch no longer sets mr = NULL for flush. This check is mainly to defend against someone changing
> it again in the future and is near where mr is dereferenced. You are correct about the comment I can change it.
> Will give it a day or two to see if anything else comes in.
>
> Regards,
>
> Bob Pearson
