[05/12] net: smc911x: Fix potential memleak() in init fail path

Fix memleak in the init fail path, where if allocation or registration
of MDIO bus fails, then ethernet interface is not unregistered and the
private data are not freed, yet the probe function reports a failure.

Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
Cc: Joe Hershberger <joe.hershberger at ni.com>
Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
---
 drivers/net/smc911x.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

On Sun, Mar 15, 2020 at 11:59 AM Marek Vasut <marek.vasut at gmail.com> wrote:
>
> Fix memleak in the init fail path, where if allocation or registration
> of MDIO bus fails, then ethernet interface is not unregistered and the
> private data are not freed, yet the probe function reports a failure.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> Cc: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>

Acked-by: Joe Hershberger <joe.hershberger at ni.com>

On Mon, Mar 16, 2020 at 1:59 AM Marek Vasut <marek.vasut at gmail.com> wrote:
>
> Fix memleak in the init fail path, where if allocation or registration
> of MDIO bus fails, then ethernet interface is not unregistered and the
> private data are not freed, yet the probe function reports a failure.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> Cc: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> ---
>  drivers/net/smc911x.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
> index 81f8f0d017..44cb45af61 100644
> --- a/drivers/net/smc911x.c
> +++ b/drivers/net/smc911x.c
> @@ -249,7 +249,7 @@ int smc911x_initialize(u8 dev_num, int base_addr)
>
>         dev = calloc(1, sizeof(*dev));
>         if (!dev) {
> -               return -1;
> +               return -ENODEV;
>         }


Our convention is to return -ENOMEM
when memory allocation fails.

If you like to do some additional cleanups here,
you can remove the unneeded braces around the single
statement, which will fix the checkpatch warning.



>
>         dev->iobase = base_addr;
> @@ -283,15 +283,23 @@ int smc911x_initialize(u8 dev_num, int base_addr)
>  #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
>         int retval;
>         struct mii_dev *mdiodev = mdio_alloc();
> -       if (!mdiodev)
> +       if (!mdiodev) {
> +               eth_unregister(dev);
> +               free(dev);
>                 return -ENOMEM;
> +       }
> +
>         strncpy(mdiodev->name, dev->name, MDIO_NAME_LEN);
>         mdiodev->read = smc911x_miiphy_read;
>         mdiodev->write = smc911x_miiphy_write;
>
>         retval = mdio_register(mdiodev);
> -       if (retval < 0)
> +       if (retval < 0) {
> +               mdio_free(mdiodev);
> +               eth_unregister(dev);
> +               free(dev);
>                 return retval;


Using "goto <label>" is a general tip to
simplify the failure path.



> +       }
>  #endif
>
>         return 1;
> --
> 2.25.0
>

On 3/17/20 7:21 AM, Masahiro Yamada wrote:
[...]
>> @@ -283,15 +283,23 @@ int smc911x_initialize(u8 dev_num, int base_addr)
>>  #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
>>         int retval;
>>         struct mii_dev *mdiodev = mdio_alloc();
>> -       if (!mdiodev)
>> +       if (!mdiodev) {
>> +               eth_unregister(dev);
>> +               free(dev);
>>                 return -ENOMEM;
>> +       }
>> +
>>         strncpy(mdiodev->name, dev->name, MDIO_NAME_LEN);
>>         mdiodev->read = smc911x_miiphy_read;
>>         mdiodev->write = smc911x_miiphy_write;
>>
>>         retval = mdio_register(mdiodev);
>> -       if (retval < 0)
>> +       if (retval < 0) {
>> +               mdio_free(mdiodev);
>> +               eth_unregister(dev);
>> +               free(dev);
>>                 return retval;
> 
> 
> Using "goto <label>" is a general tip to
> simplify the failure path.

It's even better to pull the entire MII registration into a separate
function to avoid all the ifdeffery, so I'll rather do that in a
separate patch. And then it's possible to use the goto labels without it
looking ugly.