[PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[fgao]

From: Gao Feng <fgao@ikuai8.com>

When there is no existing macvlan port in lowdev, one new macvlan port
would be created. But it doesn't be destoried when something failed later.
It casues some memleak.

Now add one flag to indicate if new macvlan port is created.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 drivers/net/macvlan.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 3234fcd..d2d6f12 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1278,6 +1278,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	struct net_device *lowerdev;
 	int err;
 	int macmode;
+	bool create = false;
 
 	if (!tb[IFLA_LINK])
 		return -EINVAL;
@@ -1304,12 +1305,18 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 		err = macvlan_port_create(lowerdev);
 		if (err < 0)
 			return err;
+		create = true;
 	}
 	port = macvlan_port_get_rtnl(lowerdev);
 
 	/* Only 1 macvlan device can be created in passthru mode */
-	if (port->passthru)
-		return -EINVAL;
+	if (port->passthru) {
+		/* The macvlan port must be not created this time,
+		 * still goto destroy_macvlan_port for readability.
+		 */
+		err = -EINVAL;
+		goto destroy_macvlan_port;
+	}
 
 	vlan->lowerdev = lowerdev;
 	vlan->dev      = dev;
@@ -1325,24 +1332,28 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 
 	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
-		if (port->count)
-			return -EINVAL;
+		if (port->count) {
+			err = -EINVAL;
+			goto destroy_macvlan_port;
+		}
 		port->passthru = true;
 		eth_hw_addr_inherit(dev, lowerdev);
 	}
 
 	if (data && data[IFLA_MACVLAN_MACADDR_MODE]) {
-		if (vlan->mode != MACVLAN_MODE_SOURCE)
-			return -EINVAL;
+		if (vlan->mode != MACVLAN_MODE_SOURCE) {
+			err = -EINVAL;
+			goto destroy_macvlan_port;
+		}
 		macmode = nla_get_u32(data[IFLA_MACVLAN_MACADDR_MODE]);
 		err = macvlan_changelink_sources(vlan, macmode, data);
 		if (err)
-			return err;
+			goto destroy_macvlan_port;
 	}
 
 	err = register_netdevice(dev);
 	if (err < 0)
-		return err;
+		goto destroy_macvlan_port;
 
 	dev->priv_flags |= IFF_MACVLAN;
 	err = netdev_upper_dev_link(lowerdev, dev);
@@ -1357,7 +1368,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 
 unregister_netdev:
 	unregister_netdevice(dev);
-
+destroy_macvlan_port:
+	if (create)
+		macvlan_port_destroy(port->dev);
 	return err;
 }
 EXPORT_SYMBOL_GPL(macvlan_common_newlink);
-- 
1.9.1

Re: [PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[Feng Gao]

On Fri, Nov 4, 2016 at 10:28 AM,  <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> When there is no existing macvlan port in lowdev, one new macvlan port
> would be created. But it doesn't be destoried when something failed later.
> It casues some memleak.
>
> Now add one flag to indicate if new macvlan port is created.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  drivers/net/macvlan.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 3234fcd..d2d6f12 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1278,6 +1278,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>         struct net_device *lowerdev;
>         int err;
>         int macmode;
> +       bool create = false;
>
>         if (!tb[IFLA_LINK])
>                 return -EINVAL;
> @@ -1304,12 +1305,18 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>                 err = macvlan_port_create(lowerdev);
>                 if (err < 0)
>                         return err;
> +               create = true;
>         }
>         port = macvlan_port_get_rtnl(lowerdev);
>
>         /* Only 1 macvlan device can be created in passthru mode */
> -       if (port->passthru)
> -               return -EINVAL;
> +       if (port->passthru) {
> +               /* The macvlan port must be not created this time,
> +                * still goto destroy_macvlan_port for readability.
> +                */
> +               err = -EINVAL;
> +               goto destroy_macvlan_port;
> +       }
>
>         vlan->lowerdev = lowerdev;
>         vlan->dev      = dev;
> @@ -1325,24 +1332,28 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>                 vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
>
>         if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> -               if (port->count)
> -                       return -EINVAL;
> +               if (port->count) {
> +                       err = -EINVAL;
> +                       goto destroy_macvlan_port;
> +               }
>                 port->passthru = true;
>                 eth_hw_addr_inherit(dev, lowerdev);
>         }
>
>         if (data && data[IFLA_MACVLAN_MACADDR_MODE]) {
> -               if (vlan->mode != MACVLAN_MODE_SOURCE)
> -                       return -EINVAL;
> +               if (vlan->mode != MACVLAN_MODE_SOURCE) {
> +                       err = -EINVAL;
> +                       goto destroy_macvlan_port;
> +               }
>                 macmode = nla_get_u32(data[IFLA_MACVLAN_MACADDR_MODE]);
>                 err = macvlan_changelink_sources(vlan, macmode, data);
>                 if (err)
> -                       return err;
> +                       goto destroy_macvlan_port;
>         }
>
>         err = register_netdevice(dev);
>         if (err < 0)
> -               return err;
> +               goto destroy_macvlan_port;
>
>         dev->priv_flags |= IFF_MACVLAN;
>         err = netdev_upper_dev_link(lowerdev, dev);
> @@ -1357,7 +1368,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>
>  unregister_netdev:
>         unregister_netdevice(dev);
> -
> +destroy_macvlan_port:
> +       if (create)
> +               macvlan_port_destroy(port->dev);
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(macvlan_common_newlink);
> --
> 1.9.1
>
>

Hi all,

How about this fix ? Is there any issue ?

Regards
Feng

Re: [PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[David Miller]

From: fgao@ikuai8.com
Date: Fri,  4 Nov 2016 10:28:49 +0800

> From: Gao Feng <fgao@ikuai8.com>
> 
> When there is no existing macvlan port in lowdev, one new macvlan port
> would be created. But it doesn't be destoried when something failed later.
> It casues some memleak.
> 
> Now add one flag to indicate if new macvlan port is created.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>

You need to be more patient, it sometimes take several days before
your get patch reviewed or applied.  Sometimes nobody reviews a change
for some time because it is obscure or everyone is busy.

All patches are tracked in patchwork, so it is never an issue of a
change getting "lost".  Therefore, it never makes sense to ping the
list again and ask if a change is "ok".

Personally, when people ping like that, it makes me want to review
that patch _less_ not more.  So please do not do it.

Thank you.

Re: [PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[Gao Feng]

Hi David,

On Tue, Nov 8, 2016 at 9:33 AM, David Miller <davem@davemloft.net> wrote:
> From: fgao@ikuai8.com
> Date: Fri,  4 Nov 2016 10:28:49 +0800
>
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> When there is no existing macvlan port in lowdev, one new macvlan port
>> would be created. But it doesn't be destoried when something failed later.
>> It casues some memleak.
>>
>> Now add one flag to indicate if new macvlan port is created.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>
> You need to be more patient, it sometimes take several days before
> your get patch reviewed or applied.  Sometimes nobody reviews a change
> for some time because it is obscure or everyone is busy.

Sorry, it is my fault.
I thought this patch may be lost because there are too many emails every day.
So ping just as a reminder.

>
> All patches are tracked in patchwork, so it is never an issue of a
> change getting "lost".  Therefore, it never makes sense to ping the
> list again and ask if a change is "ok".
>
> Personally, when people ping like that, it makes me want to review
> that patch _less_ not more.  So please do not do it.
>
> Thank you.
>

Now I get it. I violated some rules as a fresh man in kernel community.
Sorry again.


Best Regards
Feng

Re: [PATCH net 1/1] driver: macvlan: Destroy new macvlan port if macvlan_common_newlink failed.

[David Miller]

From: fgao@ikuai8.com
Date: Fri,  4 Nov 2016 10:28:49 +0800

> From: Gao Feng <fgao@ikuai8.com>
> 
> When there is no existing macvlan port in lowdev, one new macvlan port
> would be created. But it doesn't be destoried when something failed later.
> It casues some memleak.
> 
> Now add one flag to indicate if new macvlan port is created.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>

Looks good, applied, thanks.