* net_device leak after "bridge: allow creating bridge devices with netlink"?
@ 2011-08-22 11:34 Jan Beulich
2011-08-22 12:50 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2011-08-22 11:34 UTC (permalink / raw)
To: shemminger; +Cc: davem, netdev
Stephen,
directly returning the result of register_netdev() from br_add_bridge()
would - to me - appear to be leaking "dev" in case of failure. Or am I
overlooking some implicit mechanism by which this would get freed?
Thanks, Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
2011-08-22 11:34 net_device leak after "bridge: allow creating bridge devices with netlink"? Jan Beulich
@ 2011-08-22 12:50 ` Eric Dumazet
2011-08-22 15:55 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2011-08-22 12:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: shemminger, davem, netdev
Le lundi 22 août 2011 à 12:34 +0100, Jan Beulich a écrit :
> Stephen,
>
> directly returning the result of register_netdev() from br_add_bridge()
> would - to me - appear to be leaking "dev" in case of failure. Or am I
> overlooking some implicit mechanism by which this would get freed?
>
> Thanks, Jan
You're 100 % right, there is a leak here.
(But "ip" or "brctl" commands first check link already exist before
trying create it, so you need to tweak them to actually hit this bug)
We have to add a free_netdev() call to free the net_device.
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 2cdf007..e738154 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
int br_add_bridge(struct net *net, const char *name)
{
struct net_device *dev;
+ int res;
dev = alloc_netdev(sizeof(struct net_bridge), name,
br_dev_setup);
@@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
dev_net_set(dev, net);
- return register_netdev(dev);
+ res = register_netdev(dev);
+ if (res)
+ free_netdev(dev);
+ return res;
}
int br_del_bridge(struct net *net, const char *name)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
2011-08-22 12:50 ` Eric Dumazet
@ 2011-08-22 15:55 ` Stephen Hemminger
2011-08-22 16:05 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2011-08-22 15:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jan Beulich, davem, netdev
On Mon, 22 Aug 2011 14:50:01 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 22 août 2011 à 12:34 +0100, Jan Beulich a écrit :
> > Stephen,
> >
> > directly returning the result of register_netdev() from br_add_bridge()
> > would - to me - appear to be leaking "dev" in case of failure. Or am I
> > overlooking some implicit mechanism by which this would get freed?
> >
> > Thanks, Jan
>
> You're 100 % right, there is a leak here.
>
> (But "ip" or "brctl" commands first check link already exist before
> trying create it, so you need to tweak them to actually hit this bug)
>
> We have to add a free_netdev() call to free the net_device.
>
>
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 2cdf007..e738154 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
> int br_add_bridge(struct net *net, const char *name)
> {
> struct net_device *dev;
> + int res;
>
> dev = alloc_netdev(sizeof(struct net_bridge), name,
> br_dev_setup);
> @@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
>
> dev_net_set(dev, net);
>
> - return register_netdev(dev);
> + res = register_netdev(dev);
> + if (res)
> + free_netdev(dev);
> + return res;
> }
>
> int br_del_bridge(struct net *net, const char *name)
Missing Signed-off-by:?
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
2011-08-22 15:55 ` Stephen Hemminger
@ 2011-08-22 16:05 ` Eric Dumazet
2011-08-22 23:48 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2011-08-22 16:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Jan Beulich, davem, netdev
Le lundi 22 août 2011 à 08:55 -0700, Stephen Hemminger a écrit :
> On Mon, 22 Aug 2011 14:50:01 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > Le lundi 22 août 2011 à 12:34 +0100, Jan Beulich a écrit :
> > > Stephen,
> > >
> > > directly returning the result of register_netdev() from br_add_bridge()
> > > would - to me - appear to be leaking "dev" in case of failure. Or am I
> > > overlooking some implicit mechanism by which this would get freed?
> > >
> > > Thanks, Jan
> >
> > You're 100 % right, there is a leak here.
> >
> > (But "ip" or "brctl" commands first check link already exist before
> > trying create it, so you need to tweak them to actually hit this bug)
> >
> > We have to add a free_netdev() call to free the net_device.
> >
> >
> >
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index 2cdf007..e738154 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
> > int br_add_bridge(struct net *net, const char *name)
> > {
> > struct net_device *dev;
> > + int res;
> >
> > dev = alloc_netdev(sizeof(struct net_bridge), name,
> > br_dev_setup);
> > @@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
> >
> > dev_net_set(dev, net);
> >
> > - return register_netdev(dev);
> > + res = register_netdev(dev);
> > + if (res)
> > + free_netdev(dev);
> > + return res;
> > }
> >
> > int br_del_bridge(struct net *net, const char *name)
>
> Missing Signed-off-by:?
>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
Well, its only a direct coding of Jan report ;)
Thanks
[PATCH] bridge: fix a possible net_device leak
Jan Beulich reported a possible net_device leak in bridge code after
commit bb900b27a2f4 (bridge: allow creating bridge devices with netlink)
Reported-by: Jan Beulich <JBeulich@novell.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
---
net/bridge/br_if.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 2cdf007..e738154 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
int br_add_bridge(struct net *net, const char *name)
{
struct net_device *dev;
+ int res;
dev = alloc_netdev(sizeof(struct net_bridge), name,
br_dev_setup);
@@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
dev_net_set(dev, net);
- return register_netdev(dev);
+ res = register_netdev(dev);
+ if (res)
+ free_netdev(dev);
+ return res;
}
int br_del_bridge(struct net *net, const char *name)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
2011-08-22 16:05 ` Eric Dumazet
@ 2011-08-22 23:48 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2011-08-22 23:48 UTC (permalink / raw)
To: eric.dumazet; +Cc: shemminger, JBeulich, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 22 Aug 2011 18:05:59 +0200
> [PATCH] bridge: fix a possible net_device leak
>
> Jan Beulich reported a possible net_device leak in bridge code after
> commit bb900b27a2f4 (bridge: allow creating bridge devices with netlink)
>
> Reported-by: Jan Beulich <JBeulich@novell.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-08-22 23:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-22 11:34 net_device leak after "bridge: allow creating bridge devices with netlink"? Jan Beulich
2011-08-22 12:50 ` Eric Dumazet
2011-08-22 15:55 ` Stephen Hemminger
2011-08-22 16:05 ` Eric Dumazet
2011-08-22 23:48 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).