RE: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr
From: Daisuke Matsuda (Fujitsu)
Date: Sun Jan 15 2023 - 20:48:30 EST
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@xxxxxxxxx>
> >> Reported-by: TOTE Robot <oslab@xxxxxxxxxxxxxxx>
> >> ---
> >> 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)) {
> >