Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr
To:     Zhu Yanjun <yanjun.zhu@linux.dev>, zyjzyj2000@gmail.com,
        jgg@ziepe.ca, leon@kernel.org
Cc:     linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org,
        TOTE Robot <oslab@tsinghua.edu.cn>
References: <20230113023527.728725-1-baijiaju1990@gmail.com>
 <ec0e983b-15fb-e43f-90e2-d4f79d27298a@linux.dev>
From:   Jia-Ju Bai <baijiaju1990@gmail.com>
Message-ID: <c7748a2b-2c27-d171-810c-f3eb56120944@gmail.com>
Date:   Fri, 13 Jan 2023 16:47:07 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.0
MIME-Version: 1.0
In-Reply-To: <ec0e983b-15fb-e43f-90e2-d4f79d27298a@linux.dev>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Thanks for the reply :)

I checked the commit 3282a549cf9b, and it said:
"If responder get a zero-byte RDMA Read request, qp->resp.mr is not set 
in check_rkey() (see IBA C9-88)."
Thus, this commit added a NULL check for mr (aliased with qp->resp.mr) 
in read_reply().

The buggy call trace of the commit 3282a549cf9b is rxe_responder() -> 
read_reply(qp).
In the code, there are some possible call traces from rxe_responder():

rxe_responder()
   execute(qp)
     write_data_in(qp)
       err = rxe_mr_copy(qp->resp.mr)

rxe_responder()
   process_flush(qp)
     mr = qp->resp.mr
     start = mr->ibmr.iova;
     length = mr->ibmr.length
     rxe_flush_pmem_iova(mr)

rxe_responder()
   atomic_reply(qp)
     mr = qp->resp.mr
     if (mr->state != RXE_MR_STATE_VALID)

rxe_responder()
   atomic_write_reply(qp)
     do_atomic_write(qp)
       mr = qp->resp.mr
       if (mr->state != RXE_MR_STATE_VALID)
       dst = iova_to_vaddr(mr)

rxe_responder()
   read_reply(qp)
     mr = qp->resp.mr
     err = rxe_mr_copy(mr)

For these possible call traces, they may share the same qp->resp.mr in 
the commit 3282a549cf9b.
Thus, qp->resp.mr should be checked in these call traces.
I am not quite sure of this, so I am looking forward to your opinions, 
thanks :)


Best wishes,
Jia-Ju Bai


On 2023/1/13 15:53, Zhu Yanjun wrote:
> 在 2023/1/13 10:35, Jia-Ju Bai 写道:
>> In a previous commit 3282a549cf9b, qp->resp.mr could be NULL. Moreover,
>> in many functions, qp->resp.mr is checked before its dereferences.
>> However, in some functions, this variable is not checked, and thus NULL
>> checks should be added.
>
> IMO， we should analyze the code snippet one by one.
> And it is not good to add "NULL check" without futher investigations.
>
> Zhu Yanjun
>>
>> These results are reported by a static tool written by myself.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_resp.c | 47 ++++++++++++++++------------
>>   1 file changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c 
>> b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index c74972244f08..2eafa1667a9e 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -621,11 +621,13 @@ static enum resp_states write_data_in(struct 
>> rxe_qp *qp,
>>       int    err;
>>       int data_len = payload_size(pkt);
>>   -    err = rxe_mr_copy(qp->resp.mr, qp->resp.va + qp->resp.offset,
>> -              payload_addr(pkt), data_len, RXE_TO_MR_OBJ);
>> -    if (err) {
>> -        rc = RESPST_ERR_RKEY_VIOLATION;
>> -        goto out;
>> +    if (qp->resp.mr) {
>> +        err = rxe_mr_copy(qp->resp.mr, qp->resp.va + qp->resp.offset,
>> +                  payload_addr(pkt), data_len, RXE_TO_MR_OBJ);
>> +        if (err) {
>> +            rc = RESPST_ERR_RKEY_VIOLATION;
>> +            goto out;
>> +        }
>>       }
>>         qp->resp.va += data_len;
>> @@ -699,11 +701,13 @@ static enum resp_states process_flush(struct 
>> rxe_qp *qp,
>>           start = res->flush.va;
>>           length = res->flush.length;
>>       } else { /* level == IB_FLUSH_MR */
>> -        start = mr->ibmr.iova;
>> -        length = mr->ibmr.length;
>> +        if (mr) {
>> +            start = mr->ibmr.iova;
>> +            length = mr->ibmr.length;
>> +        }
>>       }
>>   -    if (res->flush.type & IB_FLUSH_PERSISTENT) {
>> +    if (mr && res->flush.type & IB_FLUSH_PERSISTENT) {
>>           if (rxe_flush_pmem_iova(mr, start, length))
>>               return RESPST_ERR_RKEY_VIOLATION;
>>           /* Make data persistent. */
>> @@ -742,7 +746,7 @@ static enum resp_states atomic_reply(struct 
>> rxe_qp *qp,
>>           qp->resp.res = res;
>>       }
>>   -    if (!res->replay) {
>> +    if (!res->replay && mr) {
>>           if (mr->state != RXE_MR_STATE_VALID) {
>>               ret = RESPST_ERR_RKEY_VIOLATION;
>>               goto out;
>> @@ -793,15 +797,17 @@ static enum resp_states do_atomic_write(struct 
>> rxe_qp *qp,
>>       int payload = payload_size(pkt);
>>       u64 src, *dst;
>>   -    if (mr->state != RXE_MR_STATE_VALID)
>> +    if (mr && mr->state != RXE_MR_STATE_VALID)
>>           return RESPST_ERR_RKEY_VIOLATION;
>>         memcpy(&src, payload_addr(pkt), payload);
>>   -    dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
>> -    /* check vaddr is 8 bytes aligned. */
>> -    if (!dst || (uintptr_t)dst & 7)
>> -        return RESPST_ERR_MISALIGNED_ATOMIC;
>> +    if (mr) {
>> +        dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, 
>> payload);
>> +        /* check vaddr is 8 bytes aligned. */
>> +        if (!dst || (uintptr_t)dst & 7)
>> +            return RESPST_ERR_MISALIGNED_ATOMIC;
>> +    }
>>         /* Do atomic write after all prior operations have completed */
>>       smp_store_release(dst, src);
>> @@ -1002,13 +1008,14 @@ static enum resp_states read_reply(struct 
>> rxe_qp *qp,
>>           return RESPST_ERR_RNR;
>>       }
>>   -    err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
>> -              payload, RXE_FROM_MR_OBJ);
>> -    if (mr)
>> +    if (mr) {
>> +        err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
>> +                  payload, RXE_FROM_MR_OBJ);
>>           rxe_put(mr);
>> -    if (err) {
>> -        kfree_skb(skb);
>> -        return RESPST_ERR_RKEY_VIOLATION;
>> +        if (err) {
>> +            kfree_skb(skb);
>> +            return RESPST_ERR_RKEY_VIOLATION;
>> +        }
>>       }
>>         if (bth_pad(&ack_pkt)) {
>