2023-01-13 02:40:43

by Jia-Ju Bai

[permalink] [raw]
Subject: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

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.

These results are reported by a static tool written by myself.

Signed-off-by: Jia-Ju Bai <[email protected]>
Reported-by: TOTE Robot <[email protected]>
---
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)) {
--
2.34.1


2023-01-13 08:24:21

by Zhu Yanjun

[permalink] [raw]
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

在 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 <[email protected]>
> Reported-by: TOTE Robot <[email protected]>
> ---
> 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)) {

2023-01-13 08:59:48

by Jia-Ju Bai

[permalink] [raw]
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

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 <[email protected]>
>> Reported-by: TOTE Robot <[email protected]>
>> ---
>>   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)) {
>

2023-01-14 06:02:02

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

Hi Jia-Ju,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.2-rc3 next-20230113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jia-Ju-Bai/infiniband-sw-rxe-Add-NULL-checks-for-qp-resp-mr/20230113-103654
patch link: https://lore.kernel.org/r/20230113023527.728725-1-baijiaju1990%40gmail.com
patch subject: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr
config: arm64-randconfig-r001-20230112
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 8d9828ef5aa9688500657d36cd2aefbe12bbd162)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/6182051db57e8c84d8d55bef726c9f028b3af9b0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jia-Ju-Bai/infiniband-sw-rxe-Add-NULL-checks-for-qp-resp-mr/20230113-103654
git checkout 6182051db57e8c84d8d55bef726c9f028b3af9b0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/infiniband/sw/rxe/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/infiniband/sw/rxe/rxe_resp.c:805:6: warning: variable 'dst' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (mr) {
^~
drivers/infiniband/sw/rxe/rxe_resp.c:813:20: note: uninitialized use occurs here
smp_store_release(dst, src);
^~~
include/asm-generic/barrier.h:172:75: note: expanded from macro 'smp_store_release'
#define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0)
^
arch/arm64/include/asm/barrier.h:122:19: note: expanded from macro '__smp_store_release'
typeof(p) __p = (p); \
^
drivers/infiniband/sw/rxe/rxe_resp.c:805:2: note: remove the 'if' if its condition is always true
if (mr) {
^~~~~~~~
drivers/infiniband/sw/rxe/rxe_resp.c:798:15: note: initialize the variable 'dst' to silence this warning
u64 src, *dst;
^
= NULL
1 warning generated.


vim +805 drivers/infiniband/sw/rxe/rxe_resp.c

791
792 #ifdef CONFIG_64BIT
793 static enum resp_states do_atomic_write(struct rxe_qp *qp,
794 struct rxe_pkt_info *pkt)
795 {
796 struct rxe_mr *mr = qp->resp.mr;
797 int payload = payload_size(pkt);
798 u64 src, *dst;
799
800 if (mr && mr->state != RXE_MR_STATE_VALID)
801 return RESPST_ERR_RKEY_VIOLATION;
802
803 memcpy(&src, payload_addr(pkt), payload);
804
> 805 if (mr) {
806 dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
807 /* check vaddr is 8 bytes aligned. */
808 if (!dst || (uintptr_t)dst & 7)
809 return RESPST_ERR_MISALIGNED_ATOMIC;
810 }
811
812 /* Do atomic write after all prior operations have completed */
813 smp_store_release(dst, src);
814
815 /* decrease resp.resid to zero */
816 qp->resp.resid -= sizeof(payload);
817
818 qp->resp.msn++;
819
820 /* next expected psn, read handles this separately */
821 qp->resp.psn = (pkt->psn + 1) & BTH_PSN_MASK;
822 qp->resp.ack_psn = qp->resp.psn;
823
824 qp->resp.opcode = pkt->opcode;
825 qp->resp.status = IB_WC_SUCCESS;
826 return RESPST_ACKNOWLEDGE;
827 }
828 #else
829 static enum resp_states do_atomic_write(struct rxe_qp *qp,
830 struct rxe_pkt_info *pkt)
831 {
832 return RESPST_ERR_UNSUPPORTED_OPCODE;
833 }
834 #endif /* CONFIG_64BIT */
835

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests


Attachments:
(No filename) (4.64 kB)
config (168.35 kB)
Download all attachments

2023-01-15 12:52:16

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

On Fri, Jan 13, 2023 at 03:53:46PM +0800, 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.

+1

Subject: RE: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr

On Fri, Jan 13, 2023 5:47 PM Jia-Ju Bai wrote:
>
> 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.

qp->resp.mr is not set for zero-length operations of RDMA Read and Write.
Otherwise, it is set in check_rkey() for these call traces, so we do not need
additional checks for them.

For RDMA Read and Write, rxe_mr_copy() returns immediately without
dereferencing the 'mr' if the length is zero, so we do not need the additional
checks for Read and Write either.

Daisuke


> 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 <[email protected]>
> >> Reported-by: TOTE Robot <[email protected]>
> >> ---
> >>   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)) {
> >