| Message ID | 20230113023527.728725-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | infiniband: sw: rxe: Add NULL checks for qp->resp.mr | expand |
在 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)) {
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)) { >
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 <lkp@intel.com>
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
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
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 <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)) { > >
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)) {
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 <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(-)