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

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

在 2023/2/14 1:41, Bob Pearson 写道:
> On 2/9/23 04:20, lizhijian@fujitsu.com wrote:
>>
>>
>> On 09/02/2023 16:14, Matsuda, Daisuke/松田 大輔 wrote:
>>> On Wed, Feb 8, 2023 5:41 PM Li, Zhijian/李 智坚 wrote:
>>>>
>>>> On 02/02/2023 12:42, Bob Pearson wrote:
>>>>> +/* 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)
>>>>>     {
>>>>> @@ -473,10 +487,12 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
>>>>>     		return RESPST_EXECUTE;
>>>>>     	}
>>>>>
>>>>> -	/* A zero-byte op is not required to set an addr or rkey. See C9-88 */
>>>>> +	/* A zero-byte read or write 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;
>>>>
>>>> You are making sure 'qp->resp.mr = NULL', but I doubt the previous qp->resp.mr will leak after this assignment when its
>>>> value is not NULL.
>>>
>>> I do not see what you mean by " the previous qp->resp.mr ".
>>> As far as I understand, 'qp->resp.mr' is set to NULL in cleanup()
>>> before responder completes its work, so it is not supposed to be
>>> reused unlike 'res->read.rkey' being retained for multi-packet Read
>>> responses. Could you elaborate on your view?
>>
>> IMO every 'qp->resp.mr' assignment will take a mr reference, so we should drop the this reference if we want to assign it a new mr again.
>>
>>
>> Theoretically, it should be changed to something like
>> if (qp->resp.mr) {
>> 	rxe_put(qp->resp.mr)
>>           qp->resp.mr = NULL;
>> }

Without "qp->resp.mr = NULL;", it is possible to cause use after free 
error if qp->resp.mr is used again.

Zhu Yanjun

>>
> Generally I agree with the idea that a (few) assignments like this, even if after minutes of looking
> over the code can be shown to be unneeded because it is already NULL or will get deleted, are helpful
> if it makes the code easier to understand and prevents later changes to unintentionally break things.
> 
> Bob
>>
>>
>>
>>>>
>>>>>     		return RESPST_EXECUTE;
>>>>>     	}
>>>>>
>>>>> @@ -555,6 +571,7 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
>>>>>     	return RESPST_EXECUTE;
>>>>>
>>>>>     err:
>>>>> +	qp->resp.mr = NULL;
>>
>> ditto
>>
>> Thanks
>> Zhijian
>>
>>>>>     	if (mr)
>