Skip to contentSkip to conversation

Fixes for syzbot problem

PATCH Applied [email protected] 4 participants 12 messages Aug 21, 2022 – Aug 31, 2022

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Fixes for syzbot problem

From: Zhu Yanjun <[email protected]>

In the link:

https://syzkaller.appspot.com/bug?id=3efca8275142fbfdde6cf77e1b18b1d5c6794d76

a rxe problem occurred. I tried to reproduce this problem in local hosts. Finally I could reproduce this problem in local hosts.
The commit ("RDMA/rxe: Fix "kernel NULL pointer dereference" error") tries to fix this problem.

After this syzbot problem disappeared, another problem appeared when qp->sk failed to initialized.
The commit ("RDMA/rxe: Fix the error caused by qp->sk") tries to solve this problem.

When I delved into the source code to solve the above problems, I found that the member variable obj in struct rxe_task is not needed. So the commit ("RDMA/rxe: Remove the unused variable obj") removes this variable obj.

After the 3 commits, in locat hosts, the whole system can work well.

Zhu Yanjun (3):
RDMA/rxe: Fix "kernel NULL pointer dereference" error RDMA/rxe: Fix the error caused by qp->sk RDMA/rxe: Remove the unused variable obj

 drivers/infiniband/sw/rxe/rxe_qp.c   | 16 ++++++++++------
 drivers/infiniband/sw/rxe/rxe_task.c |  3 +--
 drivers/infiniband/sw/rxe/rxe_task.h |  3 +--
 3 files changed, 12 insertions(+), 10 deletions(-)
Signature · 1 line
2.31.1
#1 lore raw

PATCH 1/3 RDMA/rxe: Fix "kernel NULL pointer dereference" error

From: Zhu Yanjun <[email protected]>

When rxe_queue_init in the function rxe_qp_init_req fails, both qp->req.task.func and qp->req.task.arg are not initialized.

Because of creation of qp fails, the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before calling __rxe_do_task, both qp->req.task.func and qp->req.task.arg should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Reported-by [email protected]
---
 drivers/infiniband/sw/rxe/rxe_qp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/infiniband/sw/rxe/rxe_qp.c +3 -1
index 516bf9b95e48..f10b461b9963 100644
@@ -797,7 +797,9 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
 	rxe_cleanup_task(&qp->comp.task);
 
 	/* flush out any receive wr's or pending requests */
-	__rxe_do_task(&qp->req.task);
+	if (qp->req.task.func && qp->req.task.arg)
+		__rxe_do_task(&qp->req.task);
+
 	if (qp->sq.queue) {
 		__rxe_do_task(&qp->comp.task);
 		__rxe_do_task(&qp->req.task);
Signature · 1 line
2.31.1
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] RDMA/rxe: Fix "kernel NULL pointer dereference" error

Quoted 23 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

When rxe_queue_init in the function rxe_qp_init_req fails, both qp->req.task.func and qp->req.task.arg are not initialized.

Because of creation of qp fails, the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before calling __rxe_do_task, both qp->req.task.func and qp->req.task.arg should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Reported-by [email protected]
---
 drivers/infiniband/sw/rxe/rxe_qp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/infiniband/sw/rxe/rxe_qp.c +0 -0
index 516bf9b95e48..f10b461b9963 100644

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 800

Quoted 5 lines
drivers/infiniband/sw/rxe/rxe_qp.c +1 -1
@@ -797,7 +797,9 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
 	rxe_cleanup_task(&qp->comp.task);
 
 	/* flush out any receive wr's or pending requests */
-	__rxe_do_task(&qp->req.task);
+	if (qp->req.task.func && qp->req.task.arg)

func would be enough since they get set together. But, this is still fine since not performance critical.

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 805

Quoted 5 lines
drivers/infiniband/sw/rxe/rxe_qp.c +2 -0
+		__rxe_do_task(&qp->req.task);
+
 	if (qp->sq.queue) {
 		__rxe_do_task(&qp->comp.task);
 		__rxe_do_task(&qp->req.task);
Reviewed-by
#3 lore raw

Re: [PATCH 1/3] RDMA/rxe: Fix "kernel NULL pointer dereference" error

Quoted 26 lines · Bob Pearson
Quoted 25 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

When rxe_queue_init in the function rxe_qp_init_req fails, both qp->req.task.func and qp->req.task.arg are not initialized.

Because of creation of qp fails, the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before calling __rxe_do_task, both qp->req.task.func and qp->req.task.arg should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Reported-by [email protected]
---
  drivers/infiniband/sw/rxe/rxe_qp.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/infiniband/sw/rxe/rxe_qp.c +0 -0
index 516bf9b95e48..f10b461b9963 100644
@@ -797,7 +797,9 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
  	rxe_cleanup_task(&qp->comp.task);
 
Quoted 4 lines

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 800

Quoted 3 lines
drivers/infiniband/sw/rxe/rxe_qp.c +1 -1
  	/* flush out any receive wr's or pending requests */
-	__rxe_do_task(&qp->req.task);
+	if (qp->req.task.func && qp->req.task.arg)

func would be enough since they get set together.

Agreed

otherwise, looks good

Quoted 7 lines

But, this is still fine since not performance critical.

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 805

Quoted 5 lines
drivers/infiniband/sw/rxe/rxe_qp.c +2 -0
+		__rxe_do_task(&qp->req.task);
+
  	if (qp->sq.queue) {
  		__rxe_do_task(&qp->comp.task);
  		__rxe_do_task(&qp->req.task);
#4 lore raw

PATCH 2/3 RDMA/rxe: Fix the error caused by qp->sk

In reply to [email protected]

From: Zhu Yanjun <[email protected]>

When sock_create_kern in the function rxe_qp_init_req fails, qp->sk is set to NULL.

Then the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before handling qp->sk, this variable should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
---
 drivers/infiniband/sw/rxe/rxe_qp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +4 -2
index f10b461b9963..b229052ae91a 100644
@@ -835,8 +835,10 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
 
 	free_rd_atomic_resources(qp);
 
-	kernel_sock_shutdown(qp->sk, SHUT_RDWR);
-	sock_release(qp->sk);
+	if (qp->sk) {
+		kernel_sock_shutdown(qp->sk, SHUT_RDWR);
+		sock_release(qp->sk);
+	}
 }
 
 /* called when the last reference to the qp is dropped */
Signature · 1 line
2.31.1
Reviewed-by on v1 2/3
#5 lore raw

Re: [PATCH 2/3] RDMA/rxe: Fix the error caused by qp->sk

Quoted 23 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

When sock_create_kern in the function rxe_qp_init_req fails, qp->sk is set to NULL.

Then the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before handling qp->sk, this variable should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
---
 drivers/infiniband/sw/rxe/rxe_qp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +0 -0
index f10b461b9963..b229052ae91a 100644
@@ -835,8 +835,10 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
 
 	free_rd_atomic_resources(qp);

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 844

Quoted 10 lines
drivers/infiniband/sw/rxe/rxe_qp.c +4 -2
 
-	kernel_sock_shutdown(qp->sk, SHUT_RDWR);
-	sock_release(qp->sk);
+	if (qp->sk) {
+		kernel_sock_shutdown(qp->sk, SHUT_RDWR);
+		sock_release(qp->sk);
+	}
 }
 
 /* called when the last reference to the qp is dropped */
Reviewed-by
#6 lore raw

Re: [PATCH 2/3] RDMA/rxe: Fix the error caused by qp->sk

Quoted 6 lines · Bob Pearson
Quoted 5 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

When sock_create_kern in the function rxe_qp_init_req fails, qp->sk is set to NULL.

Quoted 7 lines
Quoted 7 lines

Then the function rxe_create_qp will call rxe_qp_do_cleanup to handle allocated resource.

Before handling qp->sk, this variable should be checked.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Quoted 22 lines
Quoted 11 lines
---
  drivers/infiniband/sw/rxe/rxe_qp.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +0 -0
index f10b461b9963..b229052ae91a 100644
@@ -835,8 +835,10 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
 
  	free_rd_atomic_resources(qp);

drivers/infiniband/sw/rxe/rxe_qp.c static void rxe_qp_do_cleanup(struct work_struct *work) · line 844

Quoted 10 lines
drivers/infiniband/sw/rxe/rxe_qp.c +4 -2
 
-	kernel_sock_shutdown(qp->sk, SHUT_RDWR);
-	sock_release(qp->sk);
+	if (qp->sk) {
+		kernel_sock_shutdown(qp->sk, SHUT_RDWR);
+		sock_release(qp->sk);
+	}
  }
 
  /* called when the last reference to the qp is dropped */
#7 lore raw

PATCH 3/3 RDMA/rxe: Remove the unused variable obj

In reply to [email protected]

From: Zhu Yanjun <[email protected]>

The member variable obj in struct rxe_task is not needed. So remove it to save memory.

---
 drivers/infiniband/sw/rxe/rxe_qp.c   | 6 +++---
 drivers/infiniband/sw/rxe/rxe_task.c | 3 +--
 drivers/infiniband/sw/rxe/rxe_task.h | 3 +--
 3 files changed, 5 insertions(+), 7 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +3 -3
index b229052ae91a..ee4f7a4a7e01 100644
@@ -242,9 +242,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 
 	skb_queue_head_init(&qp->req_pkts);
 
-	rxe_init_task(rxe, &qp->req.task, qp,
+	rxe_init_task(&qp->req.task, qp,
 		      rxe_requester, "req");
-	rxe_init_task(rxe, &qp->comp.task, qp,
+	rxe_init_task(&qp->comp.task, qp,
 		      rxe_completer, "comp");
 
 	qp->qp_timeout_jiffies = 0; /* Can't be set for UD/UC in modify_qp */
@@ -292,7 +292,7 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
 
 	skb_queue_head_init(&qp->resp_pkts);
 
-	rxe_init_task(rxe, &qp->resp.task, qp,
+	rxe_init_task(&qp->resp.task, qp,
 		      rxe_responder, "resp");
 
 	qp->resp.opcode		= OPCODE_NONE;
drivers/infiniband/sw/rxe/rxe_task.c +1 -2
index 2248cf33d776..ec2b7de1c497 100644
@@ -94,10 +94,9 @@ void rxe_do_task(struct tasklet_struct *t)
 	task->ret = ret;
 }
 
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
 		  void *arg, int (*func)(void *), char *name)
 {
-	task->obj	= obj;
 	task->arg	= arg;
 	task->func	= func;
 	snprintf(task->name, sizeof(task->name), "%s", name);
drivers/infiniband/sw/rxe/rxe_task.h +1 -2
index 11d183fd3338..7f612a1c68a7 100644
@@ -19,7 +19,6 @@ enum {
  * called again.
  */
 struct rxe_task {
-	void			*obj;
 	struct tasklet_struct	tasklet;
 	int			state;
 	spinlock_t		state_lock; /* spinlock for task state */
@@ -35,7 +34,7 @@ struct rxe_task {
  *	arg  => parameter to pass to fcn
  *	func => function to call until it returns != 0
  */
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
 		  void *arg, int (*func)(void *), char *name);
 
 /* cleanup task */
Signature · 1 line
2.31.1
Reviewed-by on v1 3/3
#8 lore raw

Re: [PATCH 3/3] RDMA/rxe: Remove the unused variable obj

Quoted 66 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

The member variable obj in struct rxe_task is not needed. So remove it to save memory.

---
 drivers/infiniband/sw/rxe/rxe_qp.c   | 6 +++---
 drivers/infiniband/sw/rxe/rxe_task.c | 3 +--
 drivers/infiniband/sw/rxe/rxe_task.h | 3 +--
 3 files changed, 5 insertions(+), 7 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +3 -3
index b229052ae91a..ee4f7a4a7e01 100644
@@ -242,9 +242,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 
 	skb_queue_head_init(&qp->req_pkts);
 
-	rxe_init_task(rxe, &qp->req.task, qp,
+	rxe_init_task(&qp->req.task, qp,
 		      rxe_requester, "req");
-	rxe_init_task(rxe, &qp->comp.task, qp,
+	rxe_init_task(&qp->comp.task, qp,
 		      rxe_completer, "comp");
 
 	qp->qp_timeout_jiffies = 0; /* Can't be set for UD/UC in modify_qp */
@@ -292,7 +292,7 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
 
 	skb_queue_head_init(&qp->resp_pkts);
 
-	rxe_init_task(rxe, &qp->resp.task, qp,
+	rxe_init_task(&qp->resp.task, qp,
 		      rxe_responder, "resp");
 
 	qp->resp.opcode		= OPCODE_NONE;
drivers/infiniband/sw/rxe/rxe_task.c +1 -2
index 2248cf33d776..ec2b7de1c497 100644
@@ -94,10 +94,9 @@ void rxe_do_task(struct tasklet_struct *t)
 	task->ret = ret;
 }
 
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
 		  void *arg, int (*func)(void *), char *name)
 {
-	task->obj	= obj;
 	task->arg	= arg;
 	task->func	= func;
 	snprintf(task->name, sizeof(task->name), "%s", name);
drivers/infiniband/sw/rxe/rxe_task.h +0 -1
index 11d183fd3338..7f612a1c68a7 100644
@@ -19,7 +19,6 @@ enum {
  * called again.
  */
 struct rxe_task {
-	void			*obj;
 	struct tasklet_struct	tasklet;
 	int			state;
 	spinlock_t		state_lock; /* spinlock for task state */

drivers/infiniband/sw/rxe/rxe_task.h struct rxe_task { · line 40

Quoted 8 lines
drivers/infiniband/sw/rxe/rxe_task.h +1 -1
@@ -35,7 +34,7 @@ struct rxe_task {
  *	arg  => parameter to pass to fcn
  *	func => function to call until it returns != 0
  */
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
 		  void *arg, int (*func)(void *), char *name);
 
 /* cleanup task */

Looks OK to me. Not sure why that was ever there.

Reviewed-by
#9 lore raw

Re: [PATCH 3/3] RDMA/rxe: Remove the unused variable obj

Quoted 7 lines · Bob Pearson
Quoted 6 lines · On 8/21/22 20:16, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

The member variable obj in struct rxe_task is not needed. So remove it to save memory.

Quoted 71 lines
Quoted 60 lines
---
  drivers/infiniband/sw/rxe/rxe_qp.c   | 6 +++---
  drivers/infiniband/sw/rxe/rxe_task.c | 3 +--
  drivers/infiniband/sw/rxe/rxe_task.h | 3 +--
  3 files changed, 5 insertions(+), 7 deletions(-)
drivers/infiniband/sw/rxe/rxe_qp.c +3 -3
index b229052ae91a..ee4f7a4a7e01 100644
@@ -242,9 +242,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 
  	skb_queue_head_init(&qp->req_pkts);
 
-	rxe_init_task(rxe, &qp->req.task, qp,
+	rxe_init_task(&qp->req.task, qp,
  		      rxe_requester, "req");
-	rxe_init_task(rxe, &qp->comp.task, qp,
+	rxe_init_task(&qp->comp.task, qp,
  		      rxe_completer, "comp");
 
  	qp->qp_timeout_jiffies = 0; /* Can't be set for UD/UC in modify_qp */
@@ -292,7 +292,7 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
 
  	skb_queue_head_init(&qp->resp_pkts);
 
-	rxe_init_task(rxe, &qp->resp.task, qp,
+	rxe_init_task(&qp->resp.task, qp,
  		      rxe_responder, "resp");
 
  	qp->resp.opcode		= OPCODE_NONE;
drivers/infiniband/sw/rxe/rxe_task.c +1 -2
index 2248cf33d776..ec2b7de1c497 100644
@@ -94,10 +94,9 @@ void rxe_do_task(struct tasklet_struct *t)
  	task->ret = ret;
  }
 
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
  		  void *arg, int (*func)(void *), char *name)
  {
-	task->obj	= obj;
  	task->arg	= arg;
  	task->func	= func;
  	snprintf(task->name, sizeof(task->name), "%s", name);
drivers/infiniband/sw/rxe/rxe_task.h +0 -1
index 11d183fd3338..7f612a1c68a7 100644
@@ -19,7 +19,6 @@ enum {
   * called again.
   */
  struct rxe_task {
-	void			*obj;
  	struct tasklet_struct	tasklet;
  	int			state;
  	spinlock_t		state_lock; /* spinlock for task state */

drivers/infiniband/sw/rxe/rxe_task.h struct rxe_task { · line 40

Quoted 8 lines
drivers/infiniband/sw/rxe/rxe_task.h +1 -1
@@ -35,7 +34,7 @@ struct rxe_task {
   *	arg  => parameter to pass to fcn
   *	func => function to call until it returns != 0
   */
-int rxe_init_task(void *obj, struct rxe_task *task,
+int rxe_init_task(struct rxe_task *task,
  		  void *arg, int (*func)(void *), char *name);
 
  /* cleanup task */

Looks OK to me. Not sure why that was ever there.

author
#10 lore raw
In reply to [email protected]
Quoted 16 lines · 在 2022/8/22 9:16, [email protected] 写道:

From: Zhu Yanjun <[email protected]>

In the link:

https://syzkaller.appspot.com/bug?id=3efca8275142fbfdde6cf77e1b18b1d5c6794d76

a rxe problem occurred. I tried to reproduce this problem in local hosts. Finally I could reproduce this problem in local hosts.
The commit ("RDMA/rxe: Fix "kernel NULL pointer dereference" error") tries to fix this problem.

After this syzbot problem disappeared, another problem appeared when qp->sk failed to initialized.
The commit ("RDMA/rxe: Fix the error caused by qp->sk") tries to solve this problem.

Quoted 8 lines

When I delved into the source code to solve the above problems, I found that the member variable obj in struct rxe_task is not needed. So the commit ("RDMA/rxe: Remove the unused variable obj") removes this variable obj.

After the 3 commits, in locat hosts, the whole system can work well.

Zhu Yanjun (3):

Gently ping

Zhu Yanjun

Quoted 9 lines

RDMA/rxe: Fix "kernel NULL pointer dereference" error RDMA/rxe: Fix the error caused by qp->sk RDMA/rxe: Remove the unused variable obj

  drivers/infiniband/sw/rxe/rxe_qp.c   | 16 ++++++++++------
  drivers/infiniband/sw/rxe/rxe_task.c |  3 +--
  drivers/infiniband/sw/rxe/rxe_task.h |  3 +--
  3 files changed, 12 insertions(+), 10 deletions(-)
#11 lore raw
In reply to [email protected]
Quoted 21 lines · On Sun, Aug 21, 2022 at 09:16:12PM -0400, [email protected] wrote:

From: Zhu Yanjun <[email protected]>

In the link:

https://syzkaller.appspot.com/bug?id=3efca8275142fbfdde6cf77e1b18b1d5c6794d76

a rxe problem occurred. I tried to reproduce this problem in local hosts. Finally I could reproduce this problem in local hosts.
The commit ("RDMA/rxe: Fix "kernel NULL pointer dereference" error") tries to fix this problem.

After this syzbot problem disappeared, another problem appeared when qp->sk failed to initialized.
The commit ("RDMA/rxe: Fix the error caused by qp->sk") tries to solve this problem.

When I delved into the source code to solve the above problems, I found that the member variable obj in struct rxe_task is not needed. So the commit ("RDMA/rxe: Remove the unused variable obj") removes this variable obj.

Quoted 6 lines

After the 3 commits, in locat hosts, the whole system can work well.

Zhu Yanjun (3):
RDMA/rxe: Fix "kernel NULL pointer dereference" error RDMA/rxe: Fix the error caused by qp->sk RDMA/rxe: Remove the unused variable obj

Thanks, applied to -next with applied suggestion from Bob.