RDMA/siw: Fix a memory leak in siw_init_cpulist()

PATCH Superseded Dan Carpenter 3 participants 6 messages Aug 9, 2019 – Aug 12, 2019

Subsystems (3)

Kernel janitors InfiniBand Soft-iWARP driver (siw)

lore

Share & tools

lore

Dan Carpenter

author

2019-08-09 10:16 UTC #0 lore raw

PATCH 1/1 RDMA/siw: Fix a memory leak in siw_init_cpulist()

The error handling code doesn't free siw_cpu_info.tx_valid_cpus[0]. The first iteration through the loop is a no-op so this is sort of an off by one bug.

Fixes bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/infiniband/sw/siw/siw_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/sw/siw/siw_main.c +2 -2

index d0f140daf659..95ace3967391 100644
@@ -160,9 +160,9 @@ static int siw_init_cpulist(void)
 
 out_err:
 	siw_cpu_info.num_nodes = 0;
-	while (i) {
+	while (--i >= 0) {
 		kfree(siw_cpu_info.tx_valid_cpus[i]);
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;
+		siw_cpu_info.tx_valid_cpus[i] = NULL;
 	}
 	kfree(siw_cpu_info.tx_valid_cpus);
 	siw_cpu_info.tx_valid_cpus = NULL;

Signature · 1 line

2.20.1

I suggest you provide an even simpler fix, taking the chance to remove the redundant "siw_cpu_info.tx_valid_cpus[i] = NULL;" line (since the whole structure gets kfree'd a line further down...).
This shall be suffcient:

- while (i) {
+ while (--i >= 0)
kfree(siw_cpu_info.tx_valid_cpus[i]);
- siw_cpu_info.tx_valid_cpus[i--] = NULL;
- }
+

Reviewed-by Bernard Metzler <[email protected]>

Dan Carpenter

author

2019-08-09 11:59 UTC #2 lore raw

Quoted 53 lines · Bernard Metzler

-----"Dan Carpenter" <[email protected]> wrote: -----
Quoted 35 lines
To: "Bernard Metzler" <[email protected]>
From: "Dan Carpenter" <[email protected]>
Date: 08/09/2019 12:16PM
Cc: "Doug Ledford" <[email protected]>, "Jason Gunthorpe" <[email protected]>, [email protected], [email protected]
Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix a memory leak in siw_init_cpulist()
The error handling code doesn't free siw_cpu_info.tx_valid_cpus[0]. The
first iteration through the loop is a no-op so this is sort of an off by
one bug.
Fixes bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")
Signed-off-by Dan Carpenter <[email protected]>
---
drivers/infiniband/sw/siw/siw_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/sw/siw/siw_main.c +0 -0
index d0f140daf659..95ace3967391 100644
@@ -160,9 +160,9 @@ static int siw_init_cpulist(void)
 
out_err:
	siw_cpu_info.num_nodes = 0;
drivers/infiniband/sw/siw/siw_main.c +1 -1
-	while (i) {
+	while (--i >= 0) {
		kfree(siw_cpu_info.tx_valid_cpus[i]);
drivers/infiniband/sw/siw/siw_main.c +1 -1
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;
+		siw_cpu_info.tx_valid_cpus[i] = NULL;
Quoted 7 lines
	}
	kfree(siw_cpu_info.tx_valid_cpus);
	siw_cpu_info.tx_valid_cpus = NULL;
Signature · 1 line
2.20.1
Dan, many thanks for catching this one!
I suggest you provide an even simpler fix, taking the chance to remove the redundant "siw_cpu_info.tx_valid_cpus[i] = NULL;" line (since the whole structure gets kfree'd a line further down...).
This shall be suffcient:

drivers/infiniband/sw/siw/siw_main.c static int siw_init_cpulist(void) · line 165

Quoted 4 lines

drivers/infiniband/sw/siw/siw_main.c +1 -2

-	while (i) {
+	while (--i >= 0)
 		kfree(siw_cpu_info.tx_valid_cpus[i]);
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;

Yeah that's a good point. I'll resend.

regards,
dan carpenter

Dan Carpenter

author

2019-08-09 14:09 UTC #3 lore raw

PATCH v2 1/1 RDMA/siw: Fix a memory leak in siw_init_cpulist()

In reply to Bernard Metzler

The error handling code doesn't free siw_cpu_info.tx_valid_cpus[0]. The first iteration through the loop is a no-op so this is sort of an off by one bug. Also Bernard pointed out that we can remove the NULL assignment and simplify the code a bit.

Fixes bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Bernard Metzler <[email protected]>

---
v2: Remove the NULL assignment like Bernard Metzler pointed out.

 drivers/infiniband/sw/siw/siw_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/infiniband/sw/siw/siw_main.c +1 -3

index d0f140daf659..05a92f997f60 100644
@@ -160,10 +160,8 @@ static int siw_init_cpulist(void)
 
 out_err:
 	siw_cpu_info.num_nodes = 0;
-	while (i) {
+	while (--i >= 0)
 		kfree(siw_cpu_info.tx_valid_cpus[i]);
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;
-	}
 	kfree(siw_cpu_info.tx_valid_cpus);
 	siw_cpu_info.tx_valid_cpus = NULL;

Signature · 1 line

2.20.1

Bernard Metzler

Reviewed-by on v2 1/1

2019-08-09 14:28 UTC #4 lore raw

-----"Dan Carpenter" <[email protected]> wrote: -----

Quoted 38 lines

To: "Bernard Metzler" <[email protected]>
From: "Dan Carpenter" <[email protected]>
Date: 08/09/2019 04:09PM
Cc: "Doug Ledford" <[email protected]>, "Jason Gunthorpe" <[email protected]>, [email protected], [email protected]
Subject: [EXTERNAL] [PATCH v2] RDMA/siw: Fix a memory leak in siw_init_cpulist()
The error handling code doesn't free siw_cpu_info.tx_valid_cpus[0]. The
first iteration through the loop is a no-op so this is sort of an off by one bug. Also Bernard pointed out that we can remove the NULL assignment and simplify the code a bit.
Fixes bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Bernard Metzler <[email protected]>
---
v2: Remove the NULL assignment like Bernard Metzler pointed out.
drivers/infiniband/sw/siw/siw_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/infiniband/sw/siw/siw_main.c +0 -0
index d0f140daf659..05a92f997f60 100644
@@ -160,10 +160,8 @@ static int siw_init_cpulist(void)
 
out_err:
	siw_cpu_info.num_nodes = 0;
drivers/infiniband/sw/siw/siw_main.c +1 -1
-	while (i) {
+	while (--i >= 0)
		kfree(siw_cpu_info.tx_valid_cpus[i]);
drivers/infiniband/sw/siw/siw_main.c +0 -2
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;
-	}

Quoted 7 lines

	kfree(siw_cpu_info.tx_valid_cpus);
	siw_cpu_info.tx_valid_cpus = NULL;

Signature · 1 line

2.20.1

Dan, many thanks for finding and fixing this!

Reviewed-by Bernard Metzler <[email protected]>

Doug Ledford

Applied

2019-08-12 14:53 UTC #5 lore raw

Quoted 52 lines · Bernard Metzler

-----"Dan Carpenter" <[email protected]> wrote: -----
Quoted 39 lines
To: "Bernard Metzler" <[email protected]>
From: "Dan Carpenter" <[email protected]>
Date: 08/09/2019 04:09PM
Cc: "Doug Ledford" <[email protected]>, "Jason Gunthorpe" <[email protected]>, [email protected], [email protected]
Subject: [EXTERNAL] [PATCH v2] RDMA/siw: Fix a memory leak in siw_init_cpulist()
The error handling code doesn't free siw_cpu_info.tx_valid_cpus[0]. The
first iteration through the loop is a no-op so this is sort of an off
by one bug. Also Bernard pointed out that we can remove the NULL assignment and simplify the code a bit.
Fixes bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Bernard Metzler <[email protected]>
---
v2: Remove the NULL assignment like Bernard Metzler pointed out.
drivers/infiniband/sw/siw/siw_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/infiniband/sw/siw/siw_main.c +0 -0
index d0f140daf659..05a92f997f60 100644
@@ -160,10 +160,8 @@ static int siw_init_cpulist(void)
 
out_err:
	siw_cpu_info.num_nodes = 0;
drivers/infiniband/sw/siw/siw_main.c +1 -1
-	while (i) {
+	while (--i >= 0)
		kfree(siw_cpu_info.tx_valid_cpus[i]);
drivers/infiniband/sw/siw/siw_main.c +0 -2
-		siw_cpu_info.tx_valid_cpus[i--] = NULL;
-	}
Quoted 7 lines
	kfree(siw_cpu_info.tx_valid_cpus);
	siw_cpu_info.tx_valid_cpus = NULL;
Signature · 1 line
2.20.1
Dan, many thanks for finding and fixing this!

Quoted 2 lines

Reviewed-by Bernard Metzler <[email protected]>

Thanks, applied to for-rc.

Signature · 3 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD