Skip to contentSkip to conversation

scsi: be_iscsi: fix possible memory leak and refactor code

PATCH Superseded Geyslan G. Bem 2 participants 10 messages Nov 17, 2013 – Nov 18, 2013

Subsystems (2)
SCSI Emulex 10Gbps iSCSI - OneConnect driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Geyslan G. Bem

author
#0 lore raw

PATCH 1/1 scsi: be_iscsi: fix possible memory leak and refactor code

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

Signed-off-by Geyslan G. Bem <[email protected]>
---
 drivers/scsi/be2iscsi/be_iscsi.c | 41 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)
drivers/scsi/be2iscsi/be_iscsi.c +22 -19
index ffadbee..7e909dc 100644
@@ -535,51 +535,53 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
 		char *buf)
 {
 	struct be_cmd_get_if_info_resp *if_info;
-	int len, ip_type = BE2_IPV4;
+	int ret, ip_type = BE2_IPV4;
 
 	if (iface->iface_type == ISCSI_IFACE_TYPE_IPV6)
 		ip_type = BE2_IPV6;
 
-	len = mgmt_get_if_info(phba, ip_type, &if_info);
-	if (len) {
-		kfree(if_info);
-		return len;
-	}
+	ret = mgmt_get_if_info(phba, ip_type, &if_info);
+	if (ret)
+		goto out;
 
 	switch (param) {
 	case ISCSI_NET_PARAM_IPV4_ADDR:
-		len = sprintf(buf, "%pI4\n", if_info->ip_addr.addr);
+		ret = sprintf(buf, "%pI4\n", if_info->ip_addr.addr);
 		break;
 	case ISCSI_NET_PARAM_IPV6_ADDR:
-		len = sprintf(buf, "%pI6\n", if_info->ip_addr.addr);
+		ret = sprintf(buf, "%pI6\n", if_info->ip_addr.addr);
 		break;
 	case ISCSI_NET_PARAM_IPV4_BOOTPROTO:
 		if (!if_info->dhcp_state)
-			len = sprintf(buf, "static\n");
+			ret = sprintf(buf, "static\n");
 		else
-			len = sprintf(buf, "dhcp\n");
+			ret = sprintf(buf, "dhcp\n");
 		break;
 	case ISCSI_NET_PARAM_IPV4_SUBNET:
-		len = sprintf(buf, "%pI4\n", if_info->ip_addr.subnet_mask);
+		ret = sprintf(buf, "%pI4\n", if_info->ip_addr.subnet_mask);
 		break;
 	case ISCSI_NET_PARAM_VLAN_ENABLED:
-		len = sprintf(buf, "%s\n",
+		ret = sprintf(buf, "%s\n",
 			     (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
 			     ? "Disabled\n" : "Enabled\n");
 		break;
 	case ISCSI_NET_PARAM_VLAN_ID:
-		if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-			return -EINVAL;
+		if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+			ret = -EINVAL;
+			goto out;
+		}
 		else
-			len = sprintf(buf, "%d\n",
+			ret = sprintf(buf, "%d\n",
 				     (if_info->vlan_priority &
 				     ISCSI_MAX_VLAN_ID));
 		break;
 	case ISCSI_NET_PARAM_VLAN_PRIORITY:
-		if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-			return -EINVAL;
+		if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+			ret = -EINVAL;
+			goto out;
+		}
 		else
-			len = sprintf(buf, "%d\n",
+			ret = sprintf(buf, "%d\n",
 				     ((if_info->vlan_priority >> 13) &
 				     ISCSI_MAX_VLAN_PRIORITY));
 		break;

@@ -587,8 +589,9 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
 		WARN_ON(1);
 	}
 
+out:
 	kfree(if_info);
-	return len;
+	return ret;
 }
 
 int be2iscsi_iface_get_param(struct iscsi_iface *iface,
Signature · 1 line
1.8.4.2
maintainer
#1 lore raw
Quoted 3 lines · Geyslan G. Bem

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

James

author
#2 lore raw

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

Quoted 7 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

Quoted 4 lines

James

Thanks.

Signature · 4 lines
Regards,

Geyslan G. Bem
hackingbits.com
maintainer
#3 lore raw
Quoted 11 lines · Geyslan Gregório Bem

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

drivers/scsi/be2iscsi/be_iscsi.c +2 -4
index ffadbee..9dcbdfa 100644
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
 		ip_type = BE2_IPV6;
 
 	len = mgmt_get_if_info(phba, ip_type, &if_info);
-	if (len) {
-		kfree(if_info);
+	if (len)
 		return len;
-	}
 
 	switch (param) {
 	case ISCSI_NET_PARAM_IPV4_ADDR:

@@ -569,7 +567,7 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
 		break;
 	case ISCSI_NET_PARAM_VLAN_ID:
 		if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-			return -EINVAL;
+			len = -EINVAL;
 		else
 			len = sprintf(buf, "%d\n",
 				     (if_info->vlan_priority &

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw

2013/11/17 James Bottomley <[email protected]>:

Quoted 37 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 1 line
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage. We can initialize the if_info with NULL and always kfree it without to care about junk.

Quoted 21 lines
drivers/scsi/be2iscsi/be_iscsi.c +2 -4
        len = mgmt_get_if_info(phba, ip_type, &if_info);
-       if (len) {
-               kfree(if_info);
+       if (len)
                return len;
-       }
 
        switch (param) {
        case ISCSI_NET_PARAM_IPV4_ADDR:

@@ -569,7 +567,7 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                break;
        case ISCSI_NET_PARAM_VLAN_ID:
                if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-                       return -EINVAL;
+                       len = -EINVAL;
                else
                        len = sprintf(buf, "%d\n",
                                     (if_info->vlan_priority &
Signature · 8 lines
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#5 lore raw
Quoted 33 lines · Geyslan Gregório Bem

2013/11/17 James Bottomley <[email protected]>:

Quoted 32 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

Quoted 8 lines

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 6 lines
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage.

I don't see why not. The -EINVAL case goes through the kfree() now too, no?

Quoted 2 lines

We can initialize the if_info with NULL and always kfree it without
to care about junk.

Why? Error return means no allocation.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#6 lore raw

2013/11/18 James Bottomley <[email protected]>:

Quoted 43 lines · Geyslan Gregório Bem
Quoted 33 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 32 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

Quoted 8 lines

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 6 lines
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage.

Quoted 2 lines

I don't see why not. The -EINVAL case goes through the kfree() now too, no?

I'm refering to the removal of kfree in your suggestion.

Quoted 5 lines
Quoted 3 lines

We can initialize the if_info with NULL and always kfree it without
to care about junk.

Why? Error return means no allocation.

Setting if_info to NULL allow to kfree without concerns.

Eg.:

- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;

...

+ if (len)
+ goto out;

...

- if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }

...

case ISCSI_NET_PARAM_VLAN_PRIORITY:
- if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }

+out:

        kfree(if_info);
        return len;

Thus, if if_info remains NULL (was not allocated after mgmt_get_if_info call) kfree returns without oops. And we can centralize the freeing in the out regarding chapter 7 of coding style
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n386).

What do you think?

Quoted 4 lines

James

Signature · 8 lines
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#7 lore raw
Quoted 44 lines · Geyslan Gregório Bem

2013/11/18 James Bottomley <[email protected]>:

Quoted 43 lines · Geyslan Gregório Bem
Quoted 41 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 37 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 1 line
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage.

Quoted 4 lines
Quoted 2 lines

I don't see why not. The -EINVAL case goes through the kfree() now too, no?

I'm refering to the removal of kfree in your suggestion.

That's the second bug I pointed out via code inspection. If the function returns an error (any non zero return) then the pointer isn't altered, so we return without the free. It's a standard error pattern.

Quoted 15 lines
Quoted 5 lines
Quoted 3 lines

We can initialize the if_info with NULL and always kfree it without
to care about junk.

Why? Error return means no allocation.

Setting if_info to NULL allow to kfree without concerns.

Eg.:

- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;

...

(quoted diff) +1 -0
+       if (len)

Review

Quoted 10 lines
(quoted diff) +5 -2
+               goto out;
 
...
 
-               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-                       return -EINVAL;
+               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+                       len = -EINVAL;
+                       goto out;
+               }

What's the point of that? Just removing the goto out; has the code going to the same place because of the break below.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#8 lore raw

2013/11/18 James Bottomley <[email protected]>:

Quoted 45 lines · Geyslan Gregório Bem
Quoted 44 lines

2013/11/18 James Bottomley <[email protected]>:

Quoted 43 lines · Geyslan Gregório Bem
Quoted 41 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 37 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 1 line
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

I don't see why not. The -EINVAL case goes through the kfree() now too, no?

I'm refering to the removal of kfree in your suggestion.

That's the second bug I pointed out via code inspection. If the function returns an error (any non zero return) then the pointer isn't altered, so we return without the free. It's a standard error pattern.

Ok. So kfree is useless until the code go to the function bail. Right?

Quoted 27 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines

We can initialize the if_info with NULL and always kfree it without
to care about junk.

Why? Error return means no allocation.

Setting if_info to NULL allow to kfree without concerns.

Eg.:

- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;

...

(quoted diff) +1 -0
+       if (len)

Review

Quoted 10 lines
(quoted diff) +5 -2
+               goto out;
 
...
 
-               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-                       return -EINVAL;
+               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+                       len = -EINVAL;
+                       goto out;
+               }
Quoted 6 lines

What's the point of that? Just removing the goto out; has the code going to the same place because of the break below.

James

I agree whit you that the code goes to same place with or without goto out. It's just a pattern to simplify future changes in the function.

But I can remove it if you want.

Signature · 8 lines
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#9 lore raw

2013/11/18 Geyslan Gregório Bem <[email protected]>:

Quoted 94 lines

2013/11/18 James Bottomley <[email protected]>:

Quoted 45 lines · Geyslan Gregório Bem
Quoted 44 lines

2013/11/18 James Bottomley <[email protected]>:

Quoted 43 lines · Geyslan Gregório Bem
Quoted 41 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 37 lines · Geyslan Gregório Bem
Quoted 11 lines

2013/11/17 James Bottomley <[email protected]>:

Quoted 7 lines · Geyslan G. Bem
Quoted 3 lines

This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary.

You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that.

Ok, I can agree. 'len' means length? What is returned in case of non error?

it returns the length of buf written to or negative error.

Quoted 8 lines
Quoted 6 lines

You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops.

There's no need for a goto; len = -EINVAL; does everything that's needed.

Well, that is a coverity catch. CID 1128954. Check it.

I didn't say coverity was wrong, I said your patch was (well not wrong, just over complex and incomplete). This is the way to fix both problems.

James

---

drivers/scsi/be2iscsi/be_iscsi.c +0 -0
index ffadbee..9dcbdfa 100644

drivers/scsi/be2iscsi/be_iscsi.c static int be2iscsi_get_if_param(struct beiscsi_hba *phba, · line 541

Quoted 1 line
drivers/scsi/be2iscsi/be_iscsi.c +0 -0
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
                ip_type = BE2_IPV6;

James, this approach will not prevent the leakage.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

I don't see why not. The -EINVAL case goes through the kfree() now too, no?

I'm refering to the removal of kfree in your suggestion.

That's the second bug I pointed out via code inspection. If the function returns an error (any non zero return) then the pointer isn't altered, so we return without the free. It's a standard error pattern.

Ok. So kfree is useless until the code go to the function bail. Right?

Quoted 27 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines

We can initialize the if_info with NULL and always kfree it without
to care about junk.

Why? Error return means no allocation.

Setting if_info to NULL allow to kfree without concerns.

Eg.:

- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;

...

(quoted diff) +1 -0
+       if (len)

Review

Quoted 10 lines
(quoted diff) +5 -2
+               goto out;
 
...
 
-               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
-                       return -EINVAL;
+               if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+                       len = -EINVAL;
+                       goto out;
+               }
Quoted 6 lines

What's the point of that? Just removing the goto out; has the code going to the same place because of the break below.

James

I agree whit you that the code goes to same place with or without goto out. It's just a pattern to simplify future changes in the function.

Quoted 8 lines

But I can remove it if you want.

--
Regards,

Geyslan G. Bem
hackingbits.com

Done:
[PATCH v2] scsi: be_iscsi: fix possible memory leak

Signature · 4 lines
Regards,

Geyslan G. Bem
hackingbits.com