[KJ] patch to fix initialization code of edd.c

[KJ] patch to fix initialization code of edd.c

[Om.]

Hi,
BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
the init function causes memory leaks in src/drivers/firmware/edd.c
This is my first patch submission. It is generated against 2.6.18-rc2.

Review comments welcome.
Regards,
Om.

Signed-off-by: Om Nara<xhandle@gmail.com>

 drivers/firmware/edd.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index b4502ed..036dfc6 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -739,6 +739,7 @@ static int __init
 edd_init(void)
 {
 	unsigned int i;
+	unsigned int failno;
 	int rc=0;
 	struct edd_device *edev;

@@ -754,21 +755,31 @@ edd_init(void)
 	if (rc)
 		return rc;

-	for (i = 0; i < edd_num_devices() && !rc; i++) {
+	for (i = 0, failno = 0; i < edd_num_devices(); i++) {
 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
-		if (!edev)
-			return -ENOMEM;
-
+		if (!edev) {
+			failno = i;
+			rc = -ENOMEM;
+			goto alloc_fail;
+		}
 		rc = edd_device_register(edev, i);
 		if (rc) {
-			kfree(edev);
-			break;
+			failno = i;
+			goto devreg_fail;
 		}
 		edd_devices[i] = edev;
 	}
+	return rc;

-	if (rc)
-		firmware_unregister(&edd_subsys);
+devreg_fail:
+	kfree (edev);
+
+alloc_fail:
+	for (i = 0; i < failno; i++) {
+		edd_device_unregister (edd_devices[i]);
+		kfree (edd_devices[i]);
+	}
+	firmware_unregister(&edd_subsys);
 	return rc;
 }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Neil Horman]

On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
> Hi,
> BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> the init function causes memory leaks in src/drivers/firmware/edd.c
> This is my first patch submission. It is generated against 2.6.18-rc2.
> 
> Review comments welcome.
> Regards,
> Om.
> 
Looks good, but instead of adding a stack variable failno to free the allocated
devices on failure, why not just use the same i variable and count backwards in
the loop under alloc_fail?

Regards
Neil


> Signed-off-by: Om Nara<xhandle@gmail.com>
> 
>  drivers/firmware/edd.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index b4502ed..036dfc6 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -739,6 +739,7 @@ static int __init
>  edd_init(void)
>  {
>  	unsigned int i;
> +	unsigned int failno;
>  	int rc=0;
>  	struct edd_device *edev;
> 
> @@ -754,21 +755,31 @@ edd_init(void)
>  	if (rc)
>  		return rc;
> 
> -	for (i = 0; i < edd_num_devices() && !rc; i++) {
> +	for (i = 0, failno = 0; i < edd_num_devices(); i++) {
>  		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> -		if (!edev)
> -			return -ENOMEM;
> -
> +		if (!edev) {
> +			failno = i;
> +			rc = -ENOMEM;
> +			goto alloc_fail;
> +		}
>  		rc = edd_device_register(edev, i);
>  		if (rc) {
> -			kfree(edev);
> -			break;
> +			failno = i;
> +			goto devreg_fail;
>  		}
>  		edd_devices[i] = edev;
>  	}
> +	return rc;
> 
> -	if (rc)
> -		firmware_unregister(&edd_subsys);
> +devreg_fail:
> +	kfree (edev);
> +
> +alloc_fail:
> +	for (i = 0; i < failno; i++) {
> +		edd_device_unregister (edd_devices[i]);
> +		kfree (edd_devices[i]);
> +	}
> +	firmware_unregister(&edd_subsys);
>  	return rc;
>  }
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Jesper Juhl]

On 30/07/06, Om. <om.turyx@gmail.com> wrote:
> Hi,
> BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> the init function causes memory leaks in src/drivers/firmware/edd.c
> This is my first patch submission. It is generated against 2.6.18-rc2.
>
> Review comments welcome.
> Regards,
> Om.
>
> Signed-off-by: Om Nara<xhandle@gmail.com>
>
>  drivers/firmware/edd.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index b4502ed..036dfc6 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -739,6 +739,7 @@ static int __init
>  edd_init(void)
>  {
>         unsigned int i;
> +       unsigned int failno;
>         int rc=0;
>         struct edd_device *edev;
>
> @@ -754,21 +755,31 @@ edd_init(void)
>         if (rc)
>                 return rc;
>
> -       for (i = 0; i < edd_num_devices() && !rc; i++) {
> +       for (i = 0, failno = 0; i < edd_num_devices(); i++) {
>                 edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> -               if (!edev)
> -                       return -ENOMEM;
> -
> +               if (!edev) {
> +                       failno = i;
> +                       rc = -ENOMEM;
> +                       goto alloc_fail;
> +               }
>                 rc = edd_device_register(edev, i);
>                 if (rc) {
> -                       kfree(edev);
> -                       break;
> +                       failno = i;
> +                       goto devreg_fail;
>                 }
>                 edd_devices[i] = edev;
>         }
> +       return rc;
>
> -       if (rc)
> -               firmware_unregister(&edd_subsys);
> +devreg_fail:
> +       kfree (edev);

No space before opening parenthesis when calling functions, please :

       kfree(edev);


> +
> +alloc_fail:
> +       for (i = 0; i < failno; i++) {
> +               edd_device_unregister (edd_devices[i]);
> +               kfree (edd_devices[i]);

here as well :

               edd_device_unregister(edd_devices[i]);
               kfree(edd_devices[i]);


> +       }
> +       firmware_unregister(&edd_subsys);
>         return rc;
>  }


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Om.]

On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
> > Hi,
> > BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> > the init function causes memory leaks in src/drivers/firmware/edd.c
> > This is my first patch submission. It is generated against 2.6.18-rc2.
> >
> > Review comments welcome.
> > Regards,
> > Om.
> >
> Looks good, but instead of adding a stack variable failno to free the allocated
> devices on failure, why not just use the same i variable and count backwards in
> the loop under alloc_fail?
I tried that first. But there would be some ambiguity between i=0 as a
failure and i=0 as a success.
What I meant is,
	int i;
...
	for (i = 0; i < edd_num_devices(); i++) {
		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
		if (!edev) {
			rc = -ENOMEM;
			goto alloc_fail;
		}
....
		edd_devices[i] = edev;
	}
...
alloc_fail:
	for (; i >= 0; i--) {
		edd_device_unregister (edd_devices[i]);
		kfree (edd_devices[i]);
	}
...
Such code crash for the failure case of i=0. To avoid without another
variable, I would have to resort to something like,
		if (!edev) {
			rc = -ENOMEM;
                        /* keep a flag, modify the value of i ...etc
to distinguish between success and failure*/
			goto alloc_fail;
		}
which I thought ugly.

What is your opinion on this?
Regards,
Om.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c [2nd try]

[Om.]

On 7/29/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 30/07/06, Om. <om.turyx@gmail.com> wrote:
> > Hi,
> > BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> > the init function causes memory leaks in src/drivers/firmware/edd.c
> > This is my first patch submission. It is generated against 2.6.18-rc2.
> > +       kfree (edev);
>
> No space before opening parenthesis when calling functions, please :
>
>        kfree(edev);
>
>
> > +
> > +alloc_fail:
> > +       for (i = 0; i < failno; i++) {
> > +               edd_device_unregister (edd_devices[i]);
> > +               kfree (edd_devices[i]);

Patch modified.

Signed-off-by: Om Turyx <xhandle@gmail.com>

 drivers/firmware/edd.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index b4502ed..4e9c3e1 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -739,6 +739,7 @@ static int __init
 edd_init(void)
 {
 	unsigned int i;
+	unsigned int failno;
 	int rc=0;
 	struct edd_device *edev;

@@ -754,21 +755,31 @@ edd_init(void)
 	if (rc)
 		return rc;

-	for (i = 0; i < edd_num_devices() && !rc; i++) {
+	for (i = 0, failno = 0; i < edd_num_devices(); i++) {
 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
-		if (!edev)
-			return -ENOMEM;
-
+		if (!edev) {
+			failno = i;
+			rc = -ENOMEM;
+			goto alloc_fail;
+		}
 		rc = edd_device_register(edev, i);
 		if (rc) {
-			kfree(edev);
-			break;
+			failno = i;
+			goto devreg_fail;
 		}
 		edd_devices[i] = edev;
 	}
+	return rc;

-	if (rc)
-		firmware_unregister(&edd_subsys);
+devreg_fail:
+	kfree(edev);
+
+alloc_fail:
+	for (i = 0; i < failno; i++) {
+		edd_device_unregister(edd_devices[i]);
+		kfree(edd_devices[i]);
+	}
+	firmware_unregister(&edd_subsys);
 	return rc;
 }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 1752 bytes --]

Om. wrote:
> On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
>>On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
>>
>>>Hi,
>>>BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
>>>the init function causes memory leaks in src/drivers/firmware/edd.c
>>>This is my first patch submission. It is generated against 2.6.18-rc2.
>>>
>>>Review comments welcome.
>>>Regards,
>>>Om.
>>>
>>
>>Looks good, but instead of adding a stack variable failno to free the allocated
>>devices on failure, why not just use the same i variable and count backwards in
>>the loop under alloc_fail?
> 
> I tried that first. But there would be some ambiguity between i=0 as a
> failure and i=0 as a success.
> What I meant is,
> 	int i;
> ...
> 	for (i = 0; i < edd_num_devices(); i++) {
> 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> 		if (!edev) {
> 			rc = -ENOMEM;
> 			goto alloc_fail;
> 		}
> ....
> 		edd_devices[i] = edev;
> 	}
> ...
> alloc_fail:
> 	for (; i >= 0; i--) {
> 		edd_device_unregister (edd_devices[i]);
> 		kfree (edd_devices[i]);
> 	}

i is unsigned and this code would crash for _all_ error values of i.
I'm thinking something like this might work better:

while(i-- > 0) {
        edd_device_unregister(edd_devices[i]);
        kfree(edd_devices[i]);
}

The reasoning here is that the device indicated by i is the one that
failed, in other words, it already didn't allocate, so if we get here
with i == 0 then we don't really have anything to de-allocate.  If we
get here with i == 1 then it means edd_devices[1] has _not_ been
allocated, but we do want to free for i == 0.

Comments?

Jaco
-- 
There are only 10 kinds of people in this world,
  those that understand binary and those that don't.
http://www.kroon.co.za/

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c [3rd try]

[Om.]

On 7/29/06, Jaco Kroon <jaco@kroon.co.za> wrote:
> Om. wrote:
> > On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> >>On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
> >>
> >>>Hi,
> >>>BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> >>>the init function causes memory leaks in src/drivers/firmware/edd.c
> >>>This is my first patch submission. It is generated against 2.6.18-rc2.
> >>>
> >>>Review comments welcome.
> >>>Regards,
> >>>Om.
> >>>
> >>
> >>Looks good, but instead of adding a stack variable failno to free the allocated
> >>devices on failure, why not just use the same i variable and count backwards in
> >>the loop under alloc_fail?
> >
> > I tried that first. But there would be some ambiguity between i=0 as a
> > failure and i=0 as a success.
> > What I meant is,
> >       int i;
> > ...
> >       for (i = 0; i < edd_num_devices(); i++) {
> >               edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> >               if (!edev) {
> >                       rc = -ENOMEM;
> >                       goto alloc_fail;
> >               }
> > ....
> >               edd_devices[i] = edev;
> >       }
> > ...
> > alloc_fail:
> >       for (; i >= 0; i--) {
> >               edd_device_unregister (edd_devices[i]);
> >               kfree (edd_devices[i]);
> >       }
>
> i is unsigned and this code would crash for _all_ error values of i.
I had changed i to int i;
> I'm thinking something like this might work better:
>
> while(i-- > 0) {
>         edd_device_unregister(edd_devices[i]);
>         kfree(edd_devices[i]);
> }
Works fine. The updated patch is here. Thanks to Jaco for the fix.

Signed-off-by: Om Turyx <xhandle@gmail.com>

 drivers/firmware/edd.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index b4502ed..878f135 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -738,7 +738,7 @@ static inline int edd_num_devices(void)
 static int __init
 edd_init(void)
 {
-	unsigned int i;
+	int i;
 	int rc=0;
 	struct edd_device *edev;

@@ -754,21 +754,29 @@ edd_init(void)
 	if (rc)
 		return rc;

-	for (i = 0; i < edd_num_devices() && !rc; i++) {
+	for (i = 0; i < edd_num_devices(); i++) {
 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
-		if (!edev)
-			return -ENOMEM;
-
+		if (!edev) {
+			rc = -ENOMEM;
+			goto alloc_fail;
+		}
 		rc = edd_device_register(edev, i);
 		if (rc) {
-			kfree(edev);
-			break;
+			goto devreg_fail;
 		}
 		edd_devices[i] = edev;
 	}
+	return rc;

-	if (rc)
-		firmware_unregister(&edd_subsys);
+devreg_fail:
+	kfree(edev);
+
+alloc_fail:
+    while (i-- > 0) {
+		edd_device_unregister(edd_devices[i]);
+		kfree(edd_devices[i]);
+	}
+	firmware_unregister(&edd_subsys);
 	return rc;
 }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Neil Horman]

On Sat, Jul 29, 2006 at 10:52:37PM -0700, Om. wrote:
> On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> >On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
> >> Hi,
> >> BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> >> the init function causes memory leaks in src/drivers/firmware/edd.c
> >> This is my first patch submission. It is generated against 2.6.18-rc2.
> >>
> >> Review comments welcome.
> >> Regards,
> >> Om.
> >>
> >Looks good, but instead of adding a stack variable failno to free the 
> >allocated
> >devices on failure, why not just use the same i variable and count 
> >backwards in
> >the loop under alloc_fail?
> I tried that first. But there would be some ambiguity between i=0 as a
> failure and i=0 as a success.
Ok, so remove the ambiguity from inside the for loop.  You have a few choices.
You can:

for (i=-1; i< edd_num_devices; ) {
....
i++;
edd_devices[i] = edev;

better still, right before you execute either of your goto's, just replace the
failno = i with a i--;, that way if you fail on the first iteration, i will
underflow to -1, and the freeing for loop will just get skipped.  And you save
an int worth of stack, which, while small, can be significant given that we're
building with single page stacks now.

thanks & regards
Neil

> What I meant is,
> 	int i;
> ...
> 	for (i = 0; i < edd_num_devices(); i++) {
> 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> 		if (!edev) {
> 			rc = -ENOMEM;
> 			goto alloc_fail;
> 		}
> ....
> 		edd_devices[i] = edev;
> 	}
> ...
> alloc_fail:
> 	for (; i >= 0; i--) {
> 		edd_device_unregister (edd_devices[i]);
> 		kfree (edd_devices[i]);
> 	}
> ...
> Such code crash for the failure case of i=0. To avoid without another
> variable, I would have to resort to something like,
> 		if (!edev) {
> 			rc = -ENOMEM;
>                        /* keep a flag, modify the value of i ...etc
> to distinguish between success and failure*/
> 			goto alloc_fail;
> 		}
> which I thought ugly.
> 
> What is your opinion on this?
> Regards,
> Om.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] patch to fix initialization code of edd.c

[Om.]

On 7/30/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Sat, Jul 29, 2006 at 10:52:37PM -0700, Om. wrote:
> > On 7/29/06, Neil Horman <nhorman@tuxdriver.com> wrote:
> > >On Sat, Jul 29, 2006 at 06:31:49PM -0700, Om. wrote:
> > >> Hi,
> > >> BIOS Enhanced Disk Drive Services (EDD) driver's failed return from
> > >> the init function causes memory leaks in src/drivers/firmware/edd.c
> > >> This is my first patch submission. It is generated against 2.6.18-rc2.
> > >>
> > >> Review comments welcome.
> > >> Regards,
> > >> Om.
> > >>
> > >Looks good, but instead of adding a stack variable failno to free the
> > >allocated
> > >devices on failure, why not just use the same i variable and count
> > >backwards in
> > >the loop under alloc_fail?
> > I tried that first. But there would be some ambiguity between i=0 as a
> > failure and i=0 as a success.
> Ok, so remove the ambiguity from inside the for loop.  You have a few choices.
> You can:
>
> for (i=-1; i< edd_num_devices; ) {
> ....
> i++;
> edd_devices[i] = edev;
>
> better still, right before you execute either of your goto's, just replace the
> failno = i with a i--;, that way if you fail on the first iteration, i will
> underflow to -1, and the freeing for loop will just get skipped.  And you save
> an int worth of stack, which, while small, can be significant given that we're
> building with single page stacks now.
Yeah.. Jaco Kroon suggested something on the same line and I
implemented it. I have already sent it, but I think it went out of
thread because I added [3rd try] to the subject line.

From now on, I would not modify the subject line in threads.

Regards,
Om.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors