From: Zhu Yanjun <yanjun.zhu@linux.dev>
To: Jia-Ju Bai <baijiaju1990@gmail.com>,
zyjzyj2000@gmail.com, jgg@ziepe.ca, leon@kernel.org
Cc: linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org,
TOTE Robot <oslab@tsinghua.edu.cn>
Subject: Re: [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr
Date: Fri, 13 Jan 2023 15:53:46 +0800 [thread overview]
Message-ID: <ec0e983b-15fb-e43f-90e2-d4f79d27298a@linux.dev> (raw)
In-Reply-To: <20230113023527.728725-1-baijiaju1990@gmail.com>
在 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)) {
next prev parent reply other threads:[~2023-01-13 7:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 2:35 [PATCH] infiniband: sw: rxe: Add NULL checks for qp->resp.mr Jia-Ju Bai
2023-01-13 7:53 ` Zhu Yanjun [this message]
2023-01-13 8:47 ` Jia-Ju Bai
2023-01-16 1:48 ` Daisuke Matsuda (Fujitsu)
2023-01-15 12:09 ` Leon Romanovsky
2023-01-14 5:30 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ec0e983b-15fb-e43f-90e2-d4f79d27298a@linux.dev \
--to=yanjun.zhu@linux.dev \
--cc=baijiaju1990@gmail.com \
--cc=jgg@ziepe.ca \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=oslab@tsinghua.edu.cn \
--cc=zyjzyj2000@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox