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

[Jia-Ju Bai]

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@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)) {