Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote: 2013/11/17 James Bottomley james.bottom...@hansenpartnership.com: On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote: 2013/11/17 James Bottomley james.bottom...@hansenpartnership.com: On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: 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. 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 --- diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c index ffadbee..9dcbdfa 100644 --- a/drivers/scsi/be2iscsi/be_iscsi.c +++ b/drivers/scsi/be2iscsi/be_iscsi.c @@ -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? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
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.
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
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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?
I'm refering to the removal of kfree in your suggestion.
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?
James
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
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.
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
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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?
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.
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;
+ }
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
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.
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
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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?
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?
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;
+ }
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.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 Geyslan Gregório Bem geys...@gmail.com:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
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.
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
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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?
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?
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;
+ }
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.
--
Regards,
Geyslan G. Bem
hackingbits.com
Done:
[PATCH v2] scsi: be_iscsi: fix possible memory leak
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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 geys...@gmail.com
---
drivers/scsi/be2iscsi/be_iscsi.c | 41 +---
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..7e909dc 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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,
--
1.8.4.2
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: 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 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com: On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: 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? 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. James Thanks. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
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.
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
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -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.
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
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
