[PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michal Nazarewicz]

In to places in fsg_common_init() an unconditional call to kfree()
on common was performed in error recovery which is not a valid
behaviour since fsg_common structure is not always allocated by
fsg_common_init().

To fix, the calls has been replaced with a goto to a proper error
recovery which does the correct thing.
---
 drivers/usb/gadget/f_mass_storage.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 6cfd2f4..9a59941 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2742,10 +2742,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 	/* Maybe allocate device-global string IDs, and patch descriptors */
 	if (fsg_strings[FSG_STRING_INTERFACE].id == 0) {
 		rc = usb_string_id(cdev);
-		if (rc < 0) {
-			kfree(common);
-			return ERR_PTR(rc);
-		}
+		if (unlikely(rc < 0))
+			goto error_release;
 		fsg_strings[FSG_STRING_INTERFACE].id = rc;
 		fsg_intf_desc.iInterface = rc;
 	}
@@ -2753,9 +2751,9 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 	/* Create the LUNs, open their backing files, and register the
 	 * LUN devices in sysfs. */
 	curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
-	if (!curlun) {
-		kfree(common);
-		return ERR_PTR(-ENOMEM);
+	if (unlikely(!curlun)) {
+		rc = -ENOMEM;
+		goto error_release;
 	}
 	common->luns = curlun;
 
-- 
1.7.1

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

Hi,
________________________________________
>From: linux-usb-owner@vger.kernel.org [linux-usb-owner@vger.kernel.org] On Behalf Of Michal Nazarewicz [m.nazarewicz@samsung.com]
>Sent: Wednesday, May 12, 2010 1:46 PM
>To: linux-usb@vger.kernel.org
>Cc: Kyungmin Park; Marek Szyprowski; linux-kernel@vger.kernel.org
>Subject: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

>In to places in fsg_common_init() an unconditional call to kfree()
>on common was performed in error recovery which is not a valid

If I am not wrong, common can not be NULL since it was already checked above.

2685         if (!common) {
2686                 common = kzalloc(sizeof *common, GFP_KERNEL);
2687                 if (!common)
2688                         return ERR_PTR(-ENOMEM);
2689                 common->free_storage_on_release = 1;


>behaviour since fsg_common structure is not always allocated by
>fsg_common_init().

>To fix, the calls has been replaced with a goto to a proper error
>recovery which does the correct thing.
---

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

Re: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michał Nazarewicz]

> Michal Nazarewicz [m.nazarewicz@samsung.com]
>> In to places in fsg_common_init() an unconditional call to kfree()
>> on common was performed in error recovery which is not a valid

On Wed, 12 May 2010 10:29:02 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> If I am not wrong, common can not be NULL since it was already checked above.
>
> 2685         if (!common) {
> 2686                 common = kzalloc(sizeof *common, GFP_KERNEL);
> 2687                 if (!common)
> 2688                         return ERR_PTR(-ENOMEM);
> 2689                 common->free_storage_on_release = 1;

That is correct but it is not the issue.

fsg_common_init() as a first argument takes a pointer to a fsg_common
structure which, if not NULL, is reused and it is then assumed that
caller is responsible far maintaining allocation and deallocation of
this structure.

The idea is that one can do:

static struct fsg_common fsg_common;
/* ... */
fsg_common_init(&fsg_common, cdev, fsg_config);

or allocate fsg_common structure as a part of a larger structure.  In such
cases kfree() cannot be called on the object.  Which case we are dealing
with is indicated by the common->free_storage_on_release flag.
fsg_common_release() consults it and either calls or does not call krfee().

>> behaviour since fsg_common structure is not always allocated by
>> fsg_common_init().
>>
>> To fix, the calls has been replaced with a goto to a proper error
>> recovery which does the correct thing.

Uh, I've just noticed I forgot about the:

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

>> Michal Nazarewicz [m.nazarewicz@samsung.com]
>>> In to places in fsg_common_init() an unconditional call to kfree()
>>> on common was performed in error recovery which is not a valid

>On Wed, 12 May 2010 10:29:02 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
>> If I am not wrong, common can not be NULL since it was already checked above.
>>
>> 2685         if (!common) {
>> 2686                 common = kzalloc(sizeof *common, GFP_KERNEL);
>> 2687                 if (!common)
>> 2688                         return ERR_PTR(-ENOMEM);
>> 2689                 common->free_storage_on_release = 1;

>That is correct but it is not the issue.

>fsg_common_init() as a first argument takes a pointer to a fsg_common
>structure which, if not NULL, is reused and it is then assumed that
>caller is responsible far maintaining allocation and deallocation of
>this structure.

>The idea is that one can do:

>static struct fsg_common fsg_common;
>/* ... */
>fsg_common_init(&fsg_common, cdev, fsg_config);

>or allocate fsg_common structure as a part of a larger structure.  In such
>cases kfree() cannot be called on the object.  Which case we are dealing
>with is indicated by the common->free_storage_on_release flag.
>fsg_common_release() consults it and either calls or does not call krfee().

Oh, makes sense. All instances has fsg_common_init(0, ...) and so i just missed...

But, still the central idea was, why should we go to whole error_release path which really does removing device file and closing luns and etc.
However, it will not make any difference since curlun->nluns will be zero and so there will be no loop in fsg_common_releas() function.

Apart from that, IMHO,
2894         kfree(common->luns);
should crash in case if your error path is followed.

common->luns is allocd
2712         /* Create the LUNs, open their backing files, and register the
2713          * LUN devices in sysfs. */
2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
2715         if (!curlun) {
2716                 kfree(common);
2717                 return ERR_PTR(-ENOMEM);
2718         }


Thanks,
Viral



______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

________________________________________
>From: linux-usb-owner@vger.kernel.org [linux-usb-owner@vger.kernel.org] On Behalf Of Viral Mehta [Viral.Mehta@lntinfotech.com]
>Sent: Wednesday, May 12, 2010 2:58 PM
>To: Michał Nazarewicz; linux-usb@vger.kernel.org
>Cc: Kyungmin Park; Marek Szyprowski; linux-kernel@vger.kernel.org
>Subject: RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

>>> Michal Nazarewicz [m.nazarewicz@samsung.com]
>>>> In to places in fsg_common_init() an unconditional call to kfree()
>>>> on common was performed in error recovery which is not a valid

>>On Wed, 12 May 2010 10:29:02 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
>>> If I am not wrong, common can not be NULL since it was already checked above.
>>>
>>> 2685         if (!common) {
>>> 2686                 common = kzalloc(sizeof *common, GFP_KERNEL);
>>> 2687                 if (!common)
>>> 2688                         return ERR_PTR(-ENOMEM);
>>> 2689                 common->free_storage_on_release = 1;

>>That is correct but it is not the issue.

>>fsg_common_init() as a first argument takes a pointer to a fsg_common
>>structure which, if not NULL, is reused and it is then assumed that
>>caller is responsible far maintaining allocation and deallocation of
>>this structure.

>>The idea is that one can do:

>>static struct fsg_common fsg_common;
>>/* ... */
>>fsg_common_init(&fsg_common, cdev, fsg_config);

>>or allocate fsg_common structure as a part of a larger structure.  In such
>>cases kfree() cannot be called on the object.  Which case we are dealing
>>with is indicated by the common->free_storage_on_release flag.
>>fsg_common_release() consults it and either calls or does not call krfee().

>Oh, makes sense. All instances has fsg_common_init(0, ...) and so i just missed...

>But, still the central idea was, why should we go to whole error_release path which really does removing device file and closing luns and etc.
>However, it will not make any difference since curlun->nluns will be zero and so there will be no loop in fsg_common_releas() function.

>Apart from that, IMHO,
>2894         kfree(common->luns);
>should crash in case if your error path is followed.

>common->luns is allocd
>2712         /* Create the LUNs, open their backing files, and register the
>2713          * LUN devices in sysfs. */
>2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
>2715         if (!curlun) {
>2716                 kfree(common);
>2717                 return ERR_PTR(-ENOMEM);
>2718         }


To me,
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index f4911c0..7ad9a89 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2702,7 +2702,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
        if (fsg_strings[FSG_STRING_INTERFACE].id == 0) {
                rc = usb_string_id(cdev);
                if (rc < 0) {
-                       kfree(common);
+                       if(common->free_storage_on_release)
+                               kfree(common);
                        return ERR_PTR(rc);
                }
                fsg_strings[FSG_STRING_INTERFACE].id = rc;
@@ -2713,7 +2714,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
         * LUN devices in sysfs. */
        curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
        if (!curlun) {
-               kfree(common);
+               if(common->free_storage_on_release)
+                       kfree(common);
                return ERR_PTR(-ENOMEM);
        }
        common->luns = curlun;


looks correct and simple fix.



Thanks,
Viral



______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

Re: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michał Nazarewicz]

On Wed, 12 May 2010 11:36:06 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index f4911c0..7ad9a89 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2702,7 +2702,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
>         if (fsg_strings[FSG_STRING_INTERFACE].id == 0) {
>                 rc = usb_string_id(cdev);
>                 if (rc < 0) {
> -                       kfree(common);
> +                       if(common->free_storage_on_release)
> +                               kfree(common);
>                         return ERR_PTR(rc);
>                 }
>                 fsg_strings[FSG_STRING_INTERFACE].id = rc;
> @@ -2713,7 +2714,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
>          * LUN devices in sysfs. */
>         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
>         if (!curlun) {
> -               kfree(common);
> +               if(common->free_storage_on_release)
> +                       kfree(common);
>                 return ERR_PTR(-ENOMEM);
>         }
>         common->luns = curlun;
>
> looks correct and simple fix.

Yes, it should work fine and in fact was my first approach but as I've written in
my previous mail:

> The way I see it, it does not matter that much -- it's error recovery so we assume
> it's unlikely to happen and as such speed optimisation is not really needed here --
> it's better to optimise for space and minimise the number of possible paths.

Because of that, I the "goto error_release" to be cleaner in the sense that
there is a single error recovery path and only one place where
free_storage_on_release flag is checked and common freed.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

>Because of that, I the "goto error_release" to be cleaner in the sense that
>there is a single error recovery path and only one place where
>free_storage_on_release flag is checked and common freed.

In that case, in addition to your previous patch, below hunk is needed


diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index f4911c0..b1648fc 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2883,15 +2883,16 @@ static void fsg_common_release(struct kref *ref)

        /* Beware tempting for -> do-while optimization: when in error
         * recovery nluns may be zero. */
+       if(common->luns) {
+               for (; i; --i, ++lun) {
+                       device_remove_file(&lun->dev, &dev_attr_ro);
+                       device_remove_file(&lun->dev, &dev_attr_file);
+                       fsg_lun_close(lun);
+                       device_unregister(&lun->dev);
+               }

-       for (; i; --i, ++lun) {
-               device_remove_file(&lun->dev, &dev_attr_ro);
-               device_remove_file(&lun->dev, &dev_attr_file);
-               fsg_lun_close(lun);
-               device_unregister(&lun->dev);
+               kfree(common->luns);
        }
-
-       kfree(common->luns);
        if (common->free_storage_on_release)
                kfree(common);
 }


Thanks,
Viral

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

[PATCH] USB: gadget: f_mass_storage: fix in error recovery

[Michal Nazarewicz]

In to places in fsg_common_init() an unconditional call to kfree()
on common was performed in error recovery which is not a valid
behaviour since fsg_common structure is not always allocated by
fsg_common_init().

To fix, the calls has been replaced with a goto to a proper error
recovery which does the correct thing.

Also, refactored fsg_common_release() function.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
On Wed, 12 May 2010 12:33:43 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> In that case, in addition to your previous patch, below hunk is needed
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index f4911c0..b1648fc 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2883,15 +2883,16 @@ static void fsg_common_release(struct kref *ref)
>
>         /* Beware tempting for -> do-while optimization: when in error
>          * recovery nluns may be zero. */
> +       if(common->luns) {
> +               for (; i; --i, ++lun) {
> +                       device_remove_file(&lun->dev, &dev_attr_ro);
> +                       device_remove_file(&lun->dev, &dev_attr_file);
> +                       fsg_lun_close(lun);
> +                       device_unregister(&lun->dev);
> +               }
>
> -       for (; i; --i, ++lun) {
> -               device_remove_file(&lun->dev, &dev_attr_ro);
> -               device_remove_file(&lun->dev, &dev_attr_file);
> -               fsg_lun_close(lun);
> -               device_unregister(&lun->dev);
> +               kfree(common->luns);
>         }
> -
> -       kfree(common->luns);
>         if (common->free_storage_on_release)
>                 kfree(common);
>  }

This is not entairly true but if you insist, attached is a bit
different version of your proposed chang.

 drivers/usb/gadget/f_mass_storage.c |   51 +++++++++++++++++------------------
 1 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 6cfd2f4..7d05a0b 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2742,10 +2742,8 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 	/* Maybe allocate device-global string IDs, and patch descriptors */
 	if (fsg_strings[FSG_STRING_INTERFACE].id == 0) {
 		rc = usb_string_id(cdev);
-		if (rc < 0) {
-			kfree(common);
-			return ERR_PTR(rc);
-		}
+		if (unlikely(rc < 0))
+			goto error_release;
 		fsg_strings[FSG_STRING_INTERFACE].id = rc;
 		fsg_intf_desc.iInterface = rc;
 	}
@@ -2753,9 +2751,9 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 	/* Create the LUNs, open their backing files, and register the
 	 * LUN devices in sysfs. */
 	curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
-	if (!curlun) {
-		kfree(common);
-		return ERR_PTR(-ENOMEM);
+	if (unlikely(!curlun)) {
+		rc = -ENOMEM;
+		goto error_release;
 	}
 	common->luns = curlun;
 
@@ -2914,11 +2912,7 @@ error_release:
 
 static void fsg_common_release(struct kref *ref)
 {
-	struct fsg_common *common =
-		container_of(ref, struct fsg_common, ref);
-	unsigned i = common->nluns;
-	struct fsg_lun *lun = common->luns;
-	struct fsg_buffhd *bh;
+	struct fsg_common *common = container_of(ref, struct fsg_common, ref);
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -2929,23 +2923,28 @@ static void fsg_common_release(struct kref *ref)
 		complete(&common->thread_notifier);
 	}
 
-	/* Beware tempting for -> do-while optimization: when in error
-	 * recovery nluns may be zero. */
+	if (likely(common->luns)) {
+		struct fsg_lun *lun = common->luns;
+		unsigned i = common->nluns;
 
-	for (; i; --i, ++lun) {
-		device_remove_file(&lun->dev, &dev_attr_ro);
-		device_remove_file(&lun->dev, &dev_attr_file);
-		fsg_lun_close(lun);
-		device_unregister(&lun->dev);
-	}
+		/* In error recovery common->nluns may be zero. */
+		for (; i; --i, ++lun) {
+			device_remove_file(&lun->dev, &dev_attr_ro);
+			device_remove_file(&lun->dev, &dev_attr_file);
+			fsg_lun_close(lun);
+			device_unregister(&lun->dev);
+		}
 
-	kfree(common->luns);
+		kfree(common->luns);
+	}
 
-	i = FSG_NUM_BUFFERS;
-	bh = common->buffhds;
-	do {
-		kfree(bh->buf);
-	} while (++bh, --i);
+	{
+		struct fsg_buffhd *bh = common->buffhds;
+		unsigned i = FSG_NUM_BUFFERS;
+		do {
+			kfree(bh->buf);
+		} while (++bh, --i);
+	}
 
 	if (common->free_storage_on_release)
 		kfree(common);
-- 
1.7.1

RE: [PATCH] USB: gadget: f_mass_storage: fix in error recovery

[Viral Mehta]

>In to places in fsg_common_init() an unconditional call to kfree()
>on common was performed in error recovery which is not a valid
>behaviour since fsg_common structure is not always allocated by
>fsg_common_init().
>
>To fix, the calls has been replaced with a goto to a proper error
>recovery which does the correct thing.
>
>Also, refactored fsg_common_release() function.
In that case,
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Reviewed-by: Viral Mehta <viral.mehta@lntinfotech.com>
>---
>On Wed, 12 May 2010 12:33:43 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
>> In that case, in addition to your previous patch, below hunk is needed
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
>> index f4911c0..b1648fc 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -2883,15 +2883,16 @@ static void fsg_common_release(struct kref *ref)

>This is not entairly true but if you insist, attached is a bit
>different version of your proposed chang.

But, no problem with the earlier patch as well.
______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

Re: [PATCH] USB: gadget: f_mass_storage: fix in error recovery

[Greg KH]

On Wed, May 12, 2010 at 12:51:13PM +0200, Michal Nazarewicz wrote:
> In to places in fsg_common_init() an unconditional call to kfree()
> on common was performed in error recovery which is not a valid
> behaviour since fsg_common structure is not always allocated by
> fsg_common_init().
> 
> To fix, the calls has been replaced with a goto to a proper error
> recovery which does the correct thing.
> 
> Also, refactored fsg_common_release() function.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Does this superseed your previous patch in this thread?

thanks,

greg k-h

Re: [PATCH] USB: gadget: f_mass_storage: fix in error recovery

[Michal Nazarewicz]

> On Wed, May 12, 2010 at 12:51:13PM +0200, Michal Nazarewicz wrote:
>> In to places in fsg_common_init() an unconditional call to kfree()
>> on common was performed in error recovery which is not a valid
>> behaviour since fsg_common structure is not always allocated by
>> fsg_common_init().
>> 
>> To fix, the calls has been replaced with a goto to a proper error
>> recovery which does the correct thing.
>> 
>> Also, refactored fsg_common_release() function.
>> 
>> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Greg KH <greg@kroah.com> writes:
> Does this superseed your previous patch in this thread?

Yes. 

The difference is with fsg_common_release() function which is
purely a style change.

-- 
Best regards,                                         _     _
 .o. | Liege of Serenly Enlightened Majesty of      o' \,=./ `o
 ..o | Computer Science,  Michal "mina86" Nazarewicz   (o o)
 ooo +--<mina86-tlen.pl>--<jid:mina86-jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michał Nazarewicz]

>> fsg_common_init() as a first argument takes a pointer to a fsg_common
>> structure which, if not NULL, is reused and it is then assumed that
>> caller is responsible far maintaining allocation and deallocation of
>> this structure.
>>
>> The idea is that one can do:
>>
>> static struct fsg_common fsg_common;
>> /* ... */
>> fsg_common_init(&fsg_common, cdev, fsg_config);
>>
>> or allocate fsg_common structure as a part of a larger structure.  In such
>> cases kfree() cannot be called on the object.  Which case we are dealing
>> with is indicated by the common->free_storage_on_release flag.
>> fsg_common_release() consults it and either calls or does not call krfee().

On Wed, 12 May 2010 11:28:31 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> Oh, makes sense. All instances has fsg_common_init(0, ...) and so i just missed...

I'm preparing a some patches for g_multi which change that.

Also, a change for g_mass_storage which uses a static memory would be like 10 line
patch which I may submit later.

> But, still the central idea was, why should we go to whole error_release path which
> really does removing device file and closing luns and etc.
> However, it will not make any difference since curlun->nluns will be zero and so
> there will be no loop in fsg_common_releas() function.

The way I see it, it does not matter that much -- it's error recovery so we assume
it's unlikely to happen and as such speed optimisation is not really needed here --
it's better to optimise for space and minimise the number of possible paths.

> Apart from that, IMHO,
> 2894         kfree(common->luns);
> should crash in case if your error path is followed.
>
> common->luns is allocd
> 2712         /* Create the LUNs, open their backing files, and register the
> 2713          * LUN devices in sysfs. */
> 2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
> 2715         if (!curlun) {
> 2716                 kfree(common);
> 2717                 return ERR_PTR(-ENOMEM);
> 2718         }

Yes, that's why the submitted patch changes that to "rc = -ENOMEM;
goto errer_release;".

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

>> Apart from that, IMHO,
>> 2894         kfree(common->luns);
>> should crash in case if your error path is followed.
>>
>> common->luns is allocd
>> 2712         /* Create the LUNs, open their backing files, and register the
>> 2713          * LUN devices in sysfs. */
>> 2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
>> 2715         if (!curlun) {
>> 2716                 kfree(common);
>> 2717                 return ERR_PTR(-ENOMEM);
>> 2718         }

>Yes, that's why the submitted patch changes that to "rc = -ENOMEM;
>goto errer_release;".

I think you misread.
In fsg_common_release(), we are freeing common->luns.
2894         kfree(common->luns);

However, these gets allocd in fsg_common_init()
2712         /* Create the LUNs, open their backing files, and register the
2713          * LUN devices in sysfs. */
2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
2715         if (!curlun) {
2716                 kfree(common);
2717                 return ERR_PTR(-ENOMEM);
2718         }


Now, if this kzalloc at line 2714 failed
And if your patch is applied, you will follow error_release path which in turn will call kfree on a pointer whose allocation is failed.

And this is true for the HUNK#1 as well.

Getting ? So, i still think the patch I sent looks simple and correct.




--
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

Re: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michał Nazarewicz]

On Wed, 12 May 2010 12:06:11 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> In fsg_common_release(), we are freeing common->luns.
> 2894         kfree(common->luns);
>
> However, these gets allocd in fsg_common_init()
> 2712         /* Create the LUNs, open their backing files, and register the
> 2713          * LUN devices in sysfs. */
> 2714         curlun = kzalloc(nluns * sizeof *curlun, GFP_KERNEL);
> 2715         if (!curlun) {
> 2716                 kfree(common);
> 2717                 return ERR_PTR(-ENOMEM);
> 2718         }
>
> Now, if this kzalloc at line 2714 failed
> And if your patch is applied, you will follow error_release path which in
> turn will call kfree on a pointer whose allocation is failed.

If luns allocation fails, common->luns will be NULL and freeing a NULL
pointer is a no-operation thus the code works just fine.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

RE: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Viral Mehta]

>If luns allocation fails, common->luns will be NULL and freeing a NULL
>pointer is a no-operation thus the code works just fine.

Oh, okie.
This was new to me. Sorry for the noise.

--
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

Re: [PATCH] USB: gadget: f_mass_storage: fix in fsg_common_init() error recovery

[Michał Nazarewicz]

>> If luns allocation fails, common->luns will be NULL and freeing a NULL
>> pointer is a no-operation thus the code works just fine.

On Wed, 12 May 2010 12:37:38 +0200, Viral Mehta <Viral.Mehta@lntinfotech.com> wrote:
> Oh, okie.
> This was new to me. Sorry for the noise.

Not a problem.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--