Messages in this thread Patch in this message | | | Subject | Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr | From | Jia-Ju Bai <> | Date | Fri, 13 Jan 2023 16:47:07 +0800 |
| |
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)) { >
| |