Re: [PATCH for-next v3] Subject: RDMA/rxe: Handle zero length rdma

[Tom Talpey <tom@xxxxxxxxxx>]

On 2/1/2023 6:06 AM, Daisuke Matsuda (Fujitsu) wrote:
> On Sat, Jan 28, 2023 6:10 AM 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 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 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.
> >
> > Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
> > ---
>
> When I applied this change, a testcase in rdma-core failed as shown below:
> ======================================================================
> ERROR: test_qp_ex_rc_flush (tests.test_qpex.QpExTestCase)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>    File "/root/rdma-core/tests/test_qpex.py", line 258, in test_qp_ex_rc_flush
>      raise PyverbsError(f'Unexpected {wc_status_to_str(wcs[0].status)}')
> pyverbs.pyverbs_error.PyverbsError: Unexpected Remote access error
>
> ----------------------------------------------------------------------
>
> In my opinion, your change makes sense within the range of traditional
> RDMA operations, but conflicts with the new RDMA FLUSH operation.
> Responder cannot access the target MR because of invalid rkey. The
> root cause is written in IBA Annex A19, especially 'oA19-2'.
> We thus cannot set qp->resp.rkey to 0 in qp_resp_from_reth().
>
> Do you have anything to say about this? > Li Zhijian
>
> Thanks,
> Daisuke Matsuda

I'm confused too, Bob can you point to the section of the spec
that allows the rkey to be zero? It's my understanding that
a zero-length RDMA Read must always check for access, even
though no data is actually fetched. That would not be possible
without an rkey.

Tom.

> > v3:
> >    Fixed my fat finger typing on v2. Mangled the patch.
> >
> > v2:
> >    Rebased to current for-next.
> >    Cleaned up description to be a little more accurate.
> > ---
> >   drivers/infiniband/sw/rxe/rxe_mr.c   |  6 +++
> >   drivers/infiniband/sw/rxe/rxe_resp.c | 55 +++++++++++++++++++++-------
> >   2 files changed, 47 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> > index c80458634962..5b7ede1d2b08 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > @@ -314,6 +314,9 @@ int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr,
> >   	if (length == 0)
> >   		return 0;
> >
> > +	if (WARN_ON(!mr))
> > +		return -EINVAL;
> > +
> >   	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
> >   		rxe_mr_copy_dma(mr, iova, addr, length, dir);
> >   		return 0;
> > @@ -435,6 +438,9 @@ int rxe_flush_pmem_iova(struct rxe_mr *mr, u64 iova, unsigned int length)
> >   	if (length == 0)
> >   		return 0;
> >
> > +	if (WARN_ON(!mr))
> > +		return -EINVAL;
> > +
> >   	if (mr->ibmr.type == IB_MR_TYPE_DMA)
> >   		return -EFAULT;
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> > index cd2d88de287c..b13ae98400c1 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;
> > +	if (length)
> > +		qp->resp.rkey = reth_rkey(pkt);
> > +	else
> > +		qp->resp.rkey = 0;
> >   }
> >
> >   static void qp_resp_from_atmeth(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
> > @@ -437,6 +447,10 @@ static void qp_resp_from_atmeth(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
> >   	qp->resp.resid = sizeof(u64);
> >   }
> >
> > +/* resolve the packet rkey to qp->resp.mr or set qp->resp.mr to NULL
> > + * if an invalid rkey is received or the rdma length is zero. For middle
> > + * or last packets use the stored value of mr.
> > + */
> >   static enum resp_states check_rkey(struct rxe_qp *qp,
> >   				   struct rxe_pkt_info *pkt)
> >   {
> > @@ -475,8 +489,8 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
> >
> >   	/* A zero-byte op is not required to set an addr or rkey. See C9-88 */
> >   	if ((pkt->mask & RXE_READ_OR_WRITE_MASK) &&
> > -	    (pkt->mask & RXE_RETH_MASK) &&
> > -	    reth_len(pkt) == 0) {
> > +	    (pkt->mask & RXE_RETH_MASK) && reth_len(pkt) == 0) {
> > +		qp->resp.mr = NULL;
> >   		return RESPST_EXECUTE;
> >   	}
> >
> > @@ -555,6 +569,7 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
> >   	return RESPST_EXECUTE;
> >
> >   err:
> > +	qp->resp.mr = NULL;
> >   	if (mr)
> >   		rxe_put(mr);
> >   	if (mw)
> > @@ -885,7 +900,11 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> >   	}
> >
> >   	if (res->state == rdatm_res_state_new) {
> > -		if (!res->replay) {
> > +		if (!res->replay || qp->resp.length == 0) {
> > +			/* if length == 0 mr will be NULL (is ok)
> > +			 * otherwise qp->resp.mr holds a ref on mr
> > +			 * which we transfer to mr and drop below.
> > +			 */
> >   			mr = qp->resp.mr;
> >   			qp->resp.mr = NULL;
> >   		} else {
> > @@ -899,6 +918,10 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> >   		else
> >   			opcode = IB_OPCODE_RC_RDMA_READ_RESPONSE_FIRST;
> >   	} else {
> > +		/* re-lookup mr from rkey on all later packets.
> > +		 * length will be non-zero. This can fail if someone
> > +		 * modifies or destroys the mr since the first packet.
> > +		 */
> >   		mr = rxe_recheck_mr(qp, res->read.rkey);
> >   		if (!mr)
> >   			return RESPST_ERR_RKEY_VIOLATION;
> > @@ -916,18 +939,16 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> >   	skb = prepare_ack_packet(qp, &ack_pkt, opcode, payload,
> >   				 res->cur_psn, AETH_ACK_UNLIMITED);
> >   	if (!skb) {
> > -		if (mr)
> > -			rxe_put(mr);
> > -		return RESPST_ERR_RNR;
> > +		state = RESPST_ERR_RNR;
> > +		goto err_out;
> >   	}
> >
> >   	err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
> >   			  payload, RXE_FROM_MR_OBJ);
> > -	if (mr)
> > -		rxe_put(mr);
> >   	if (err) {
> >   		kfree_skb(skb);
> > -		return RESPST_ERR_RKEY_VIOLATION;
> > +		state = RESPST_ERR_RKEY_VIOLATION;
> > +		goto err_out;
> >   	}
> >
> >   	if (bth_pad(&ack_pkt)) {
> > @@ -936,9 +957,12 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> >   		memset(pad, 0, bth_pad(&ack_pkt));
> >   	}
> >
> > +	/* rxe_xmit_packet always consumes the skb */
> >   	err = rxe_xmit_packet(qp, &ack_pkt, skb);
> > -	if (err)
> > -		return RESPST_ERR_RNR;
> > +	if (err) {
> > +		state = RESPST_ERR_RNR;
> > +		goto err_out;
> > +	}
> >
> >   	res->read.va += payload;
> >   	res->read.resid -= payload;
> > @@ -955,6 +979,9 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> >   		state = RESPST_CLEANUP;
> >   	}
> >
> > +err_out:
> > +	if (mr)
> > +		rxe_put(mr);
> >   	return state;
> >   }
> >
> > --
> > 2.37.2