Re: [PATCH] RDMA/rxe: Fix responder length checking for UD request packets

[Zhu Yanjun <yanjun.zhu@linux.dev>]

On 23.05.24 14:06, Zhu Yanjun wrote:
>
> On 23.05.24 11:46, Honggang LI wrote:
>> According to the IBA specification:
>> If a UD request packet is detected with an invalid length, the request
>> shall be an invalid request and it shall be silently dropped by
>> the responder. The responder then waits for a new request packet.
>>
>> commit 689c5421bfe0 ("RDMA/rxe: Fix incorrect responder length 
>> checking")
>> defers responder length check for UD QPs in function `copy_data`.
>> But it introduces a regression issue for UD QPs.
>>
>> When the packet size is too large to fit in the receive buffer.
>> `copy_data` will return error code -EINVAL. Then `send_data_in`
>> will return RESPST_ERR_MALFORMED_WQE. UD QP will transfer into
>> ERROR state.
>>
>> Fixes: 689c5421bfe0 ("RDMA/rxe: Fix incorrect responder length 
>> checking")
>> Signed-off-by: Honggang LI <honggangli@163.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_resp.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c 
>> b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index 963382f625d7..a74f29dcfdc9 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -354,6 +354,18 @@ static enum resp_states 
>> rxe_resp_check_length(struct rxe_qp *qp,
>>        * receive buffer later. For rmda operations additional
>>        * length checks are performed in check_rkey.
>>        */
>> +    if ((qp_type(qp) == IB_QPT_GSI) || (qp_type(qp) == IB_QPT_UD)) {
>
> From IBA specification:
>
> "
>
> QP1, used for the General Services Interface (GSI).
> •This QP uses the Unreliable Datagram transport service.
> •All traffic to and from this QP uses any VL other than VL15.
> •GSI packets arriving before the current packet’s command completes 
> may be dropped (i.e. the minimum queue depth of QP1 is one).
>
> "
>
> GSI should be MAD packets. And it should have a fixed format. Not sure 
> if the payload of GSI packets will exceed the size of the recv buffer.

Sorry. My bad.

 From IBA specification:

"

C13-3: The data payload (as used in Chapter 9: Transport Layer on page 
233) for all MADs shall be exactly 256 bytes.

"

Zhu Yanjun

>
> Except the above, I am fine with this commit. Please Jason and Leon 
> also comment on it.
>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>
> Thanks,
>
> Zhu Yanjun
>
>> +        unsigned int recv_buffer_len = 0;
>> +        unsigned int payload = payload_size(pkt);
>> +
>> +        for (int i = 0; i < qp->resp.wqe->dma.num_sge; i++)
>> +            recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
>> +        if (payload + 40 > recv_buffer_len) {
>> +            rxe_dbg_qp(qp, "The receive buffer is too small for this 
>> UD packet.\n");
>> +            return RESPST_ERR_LENGTH;
>> +        }
>> +    }
>> +
>>       if (pkt->mask & RXE_PAYLOAD_MASK && ((qp_type(qp) == IB_QPT_RC) ||
>>                            (qp_type(qp) == IB_QPT_UC))) {
>>           unsigned int mtu = qp->mtu;
>
-- 
Best Regards,
Yanjun.Zhu