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.
>>
>
> <...>
>
>> @@ -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
