* [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
@ 2013-11-17 18:51 Geyslan G. Bem
2013-11-17 19:42 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Geyslan G. Bem @ 2013-11-17 18:51 UTC (permalink / raw)
To: geyslan
Cc: Jayamohan Kallickal, James E.J. Bottomley,
open list:SERVER ENGINES 10...,
open list
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 <geyslan@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-17 18:51 [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code Geyslan G. Bem
@ 2013-11-17 19:42 ` James Bottomley
2013-11-17 21:09 ` Geyslan Gregório Bem
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2013-11-17 19:42 UTC (permalink / raw)
To: Geyslan G. Bem
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-17 19:42 ` James Bottomley
@ 2013-11-17 21:09 ` Geyslan Gregório Bem
2013-11-17 21:38 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-17 21:09 UTC (permalink / raw)
To: James Bottomley
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-17 21:09 ` Geyslan Gregório Bem
@ 2013-11-17 21:38 ` James Bottomley
2013-11-18 1:12 ` Geyslan Gregório Bem
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2013-11-17 21:38 UTC (permalink / raw)
To: Geyslan Gregório Bem
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
> 2013/11/17 James Bottomley <James.Bottomley@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;
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 &
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-17 21:38 ` James Bottomley
@ 2013-11-18 1:12 ` Geyslan Gregório Bem
2013-11-18 14:58 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-18 1:12 UTC (permalink / raw)
To: James Bottomley
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
> On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-18 1:12 ` Geyslan Gregório Bem
@ 2013-11-18 14:58 ` James Bottomley
2013-11-18 16:18 ` Geyslan Gregório Bem
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2013-11-18 14:58 UTC (permalink / raw)
To: Geyslan Gregório Bem
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
> 2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
> >> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-18 14:58 ` James Bottomley
@ 2013-11-18 16:18 ` Geyslan Gregório Bem
2013-11-18 16:24 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-18 16:18 UTC (permalink / raw)
To: James Bottomley
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
> On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
>> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> >> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-18 16:18 ` Geyslan Gregório Bem
@ 2013-11-18 16:24 ` James Bottomley
2013-11-18 16:42 ` Geyslan Gregório Bem
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2013-11-18 16:24 UTC (permalink / raw)
To: Geyslan Gregório Bem
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
> 2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
> > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
> >> 2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
> >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
> >> >> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-18 16:24 ` James Bottomley
@ 2013-11-18 16:42 ` Geyslan Gregório Bem
2013-11-18 19:51 ` Geyslan Gregório Bem
0 siblings, 1 reply; 10+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-18 16:42 UTC (permalink / raw)
To: James Bottomley
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
> On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
>> > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>> >> 2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
>> >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> >> >> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013-11-18 16:42 ` Geyslan Gregório Bem
@ 2013-11-18 19:51 ` Geyslan Gregório Bem
0 siblings, 0 replies; 10+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-18 19:51 UTC (permalink / raw)
To: James Bottomley
Cc: Jayamohan Kallickal, open list:SERVER ENGINES 10..., open list
2013/11/18 Geyslan Gregório Bem <geyslan@gmail.com>:
> 2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
>> On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
>>> 2013/11/18 James Bottomley <James.Bottomley@hansenpartnership.com>:
>>> > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>>> >> 2013/11/17 James Bottomley <James.Bottomley@hansenpartnership.com>:
>>> >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>>> >> >> 2013/11/17 James Bottomley <James.Bottomley@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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-18 19:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-17 18:51 [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code Geyslan G. Bem
2013-11-17 19:42 ` James Bottomley
2013-11-17 21:09 ` Geyslan Gregório Bem
2013-11-17 21:38 ` James Bottomley
2013-11-18 1:12 ` Geyslan Gregório Bem
2013-11-18 14:58 ` James Bottomley
2013-11-18 16:18 ` Geyslan Gregório Bem
2013-11-18 16:24 ` James Bottomley
2013-11-18 16:42 ` Geyslan Gregório Bem
2013-11-18 19:51 ` Geyslan Gregório Bem
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®