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

[Viral Mehta]

>> Michal Nazarewicz [m.nazarewicz@xxxxxxxxxxx]
>>> 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@xxxxxxxxxxxxxxx> 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.

______________________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/