[v2] RDMA/erdma: Fix exception handling in erdma_accept_newconn()

Commit Message

Markus Elfring March 13, 2025, 12:10 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Thu, 13 Mar 2025 11:44:50 +0100

The label “error” was used to jump to another pointer check despite of
the detail in the implementation of the function “erdma_accept_newconn”
that it was determined already that corresponding variables contained
still null pointers.

1. Thus return directly if
   * the cep state is not the value “ERDMA_EPSTATE_LISTENING”
     or
   * a call of the function “erdma_cep_alloc” failed.

2. Use more appropriate labels instead.

3. Delete two questionable checks.

4. Omit extra initialisations (for the variables “new_cep”, “new_s” and “ret”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 920d93eac8b9 ("RDMA/erdma: Add connection management (CM) support")
Cc: [e-mail]
Signed-off-by: Markus Elfring <[e-mail]>
---

See also:
* https://lore.kernel.org/cocci/[e-mail]/
* https://lkml.org/lkml/2023/3/19/191


V2:
The change suggestion was rebased on source files of the software “Linux next-20250313”.
A corresponding implementation detail was improved by the commit 83437689249e6a17b25e27712fbee292e42e7855
("RDMA/erdma: Prevent use-after-free in erdma_accept_newconn()") on 2025-03-06.


 drivers/infiniband/hw/erdma/erdma_cm.c | 37 +++++++++++---------------
 1 file changed, 16 insertions(+), 21 deletions(-)

--
2.48.1

Comments

Cheng Xu March 17, 2025, 2:27 a.m. UTC | #1

On 3/13/25 8:10 PM, Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> Date: Thu, 13 Mar 2025 11:44:50 +0100
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “erdma_accept_newconn”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>    * the cep state is not the value “ERDMA_EPSTATE_LISTENING”
>      or
>    * a call of the function “erdma_cep_alloc” failed.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “new_cep”, “new_s” and “ret”)
>    which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 920d93eac8b9 ("RDMA/erdma: Add connection management (CM) support")


I think this patch does not fix issues, so fix line is not needed.

Thanks,
Cheng Xu


> Cc: [e-mail]
> Signed-off-by: Markus Elfring <[e-mail]>
> ---

<...>

Patch

diff --git a/drivers/infiniband/hw/erdma/erdma_cm.c b/drivers/infiniband/hw/erdma/erdma_cm.c
index e0acc185e719..a7a79722e940 100644
--- a/drivers/infiniband/hw/erdma/erdma_cm.c
+++ b/drivers/infiniband/hw/erdma/erdma_cm.c
@@ -642,16 +642,16 @@  static int erdma_proc_mpareply(struct erdma_cep *cep)
 static void erdma_accept_newconn(struct erdma_cep *cep)
 {
 	struct socket *s = cep->sock;
-	struct socket *new_s = NULL;
-	struct erdma_cep *new_cep = NULL;
-	int ret = 0;
+	struct socket *new_s;
+	struct erdma_cep *new_cep;
+	int ret;

 	if (cep->state != ERDMA_EPSTATE_LISTENING)
-		goto error;
+		return;

 	new_cep = erdma_cep_alloc(cep->dev);
 	if (!new_cep)
-		goto error;
+		return;

 	/*
 	 * 4: Allocate a sufficient number of work elements
@@ -659,7 +659,7 @@  static void erdma_accept_newconn(struct erdma_cep *cep)
 	 * events, MPA header processing + MPA timeout.
 	 */
 	if (erdma_cm_alloc_work(new_cep, 4) != 0)
-		goto error;
+		goto put_cep;

 	/*
 	 * Copy saved socket callbacks from listening CEP
@@ -671,7 +671,7 @@  static void erdma_accept_newconn(struct erdma_cep *cep)

 	ret = kernel_accept(s, &new_s, O_NONBLOCK);
 	if (ret != 0)
-		goto error;
+		goto put_cep;

 	new_cep->sock = new_s;
 	erdma_cep_get(new_cep);
@@ -682,7 +682,7 @@  static void erdma_accept_newconn(struct erdma_cep *cep)

 	ret = erdma_cm_queue_work(new_cep, ERDMA_CM_WORK_MPATIMEOUT);
 	if (ret)
-		goto error;
+		goto disassoc_socket;

 	new_cep->listen_cep = cep;
 	erdma_cep_get(cep);
@@ -696,25 +696,20 @@  static void erdma_accept_newconn(struct erdma_cep *cep)
 			new_cep->listen_cep = NULL;
 			if (ret) {
 				erdma_cep_set_free(new_cep);
-				goto error;
+				goto disassoc_socket;
 			}
 		}
 		erdma_cep_set_free(new_cep);
 	}
 	return;

-error:
-	if (new_cep) {
-		new_cep->state = ERDMA_EPSTATE_CLOSED;
-		erdma_cancel_mpatimer(new_cep);
-
-		erdma_cep_put(new_cep);
-	}
-
-	if (new_s) {
-		erdma_socket_disassoc(new_s);
-		sock_release(new_s);
-	}
+disassoc_socket:
+	erdma_socket_disassoc(new_s);
+	sock_release(new_s);
+	new_cep->state = ERDMA_EPSTATE_CLOSED;
+	erdma_cancel_mpatimer(new_cep);
+put_cep:
+	erdma_cep_put(new_cep);
 }

 static int erdma_newconn_connected(struct erdma_cep *cep)