| Message ID | 1447686417-3979-7-git-send-email-aschultz@tpip.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Mon, Nov 16, 2015 at 04:06:47PM +0100, Andreas Schultz wrote: > Signed-off-by: Andreas Schultz <aschultz@tpip.net> > --- > gtp.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/gtp.c b/gtp.c > index 11f8fad..4f5729e 100644 > --- a/gtp.c > +++ b/gtp.c > @@ -756,8 +756,12 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, > > if (!tb[IFLA_MTU]) > dev->mtu = real_dev->mtu; > - else if (dev->mtu > real_dev->mtu) > - return -EINVAL; > + else if (dev->mtu > real_dev->mtu) { > + netdev_dbg(dev, "GTP mtu greater that transport MTU (%d > %d)\n", > + dev->mtu, real_dev->mtu); > + err = -EINVAL; > + goto out_err; This is function is using __dev_get_by_index(), so we're not holding a reference on the netdevice here. > + } > > gti = netdev_priv(dev); > gti->real_dev = real_dev; [...] > +out_err: > + dev_put(real_dev); > return err; > }
----- Original Message ----- > From: "Pablo Neira Ayuso" <pablo@soleta.eu> > To: "Andreas Schultz" <aschultz@tpip.net> > Cc: openbsc@lists.osmocom.org, "Harald Welte" <laforge@gnumonks.org> > Sent: Monday, November 16, 2015 6:46:29 PM > Subject: Re: [PATCH 06/16] gtp: add error handling to gtp_newlink > On Mon, Nov 16, 2015 at 04:06:47PM +0100, Andreas Schultz wrote: >> Signed-off-by: Andreas Schultz <aschultz@tpip.net> >> --- >> gtp.c | 32 +++++++++++++++++++++++--------- >> 1 file changed, 23 insertions(+), 9 deletions(-) >> >> diff --git a/gtp.c b/gtp.c >> index 11f8fad..4f5729e 100644 >> --- a/gtp.c >> +++ b/gtp.c >> @@ -756,8 +756,12 @@ static int gtp_newlink(struct net *src_net, struct >> net_device *dev, >> >> if (!tb[IFLA_MTU]) >> dev->mtu = real_dev->mtu; >> - else if (dev->mtu > real_dev->mtu) >> - return -EINVAL; >> + else if (dev->mtu > real_dev->mtu) { >> + netdev_dbg(dev, "GTP mtu greater that transport MTU (%d > %d)\n", >> + dev->mtu, real_dev->mtu); >> + err = -EINVAL; >> + goto out_err; > > This is function is using __dev_get_by_index(), so we're not holding a > reference on the netdevice here. But there is a 'dev_hold(real_dev);' right before that if condition. Doesn't that take a reference to the netdevice? Anyway, the conversion to the iptunnel framework makes this code largely obsolete. So I'm going to drop this change. Andreas > >> + } >> >> gti = netdev_priv(dev); >> gti->real_dev = real_dev; > [...] >> +out_err: >> + dev_put(real_dev); >> return err; > > }
On Mon, Nov 16, 2015 at 07:16:58PM +0100, Andreas Schultz wrote: > >> diff --git a/gtp.c b/gtp.c > >> index 11f8fad..4f5729e 100644 > >> --- a/gtp.c > >> +++ b/gtp.c > >> @@ -756,8 +756,12 @@ static int gtp_newlink(struct net *src_net, struct > >> net_device *dev, > >> > >> if (!tb[IFLA_MTU]) > >> dev->mtu = real_dev->mtu; > >> - else if (dev->mtu > real_dev->mtu) > >> - return -EINVAL; > >> + else if (dev->mtu > real_dev->mtu) { > >> + netdev_dbg(dev, "GTP mtu greater that transport MTU (%d > %d)\n", > >> + dev->mtu, real_dev->mtu); > >> + err = -EINVAL; > >> + goto out_err; > > > > This is function is using __dev_get_by_index(), so we're not holding a > > reference on the netdevice here. > > But there is a 'dev_hold(real_dev);' right before that if condition. Doesn't that > take a reference to the netdevice? Ah I see. Right, there is a leak there. > Anyway, the conversion to the iptunnel framework makes this code largely > obsolete. So I'm going to drop this change. OK.
On Mon, Nov 16, 2015 at 04:06:47PM +0100, Andreas Schultz wrote: > Signed-off-by: Andreas Schultz <aschultz@tpip.net> > --- > gtp.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/gtp.c b/gtp.c > index 11f8fad..4f5729e 100644 > --- a/gtp.c > +++ b/gtp.c > @@ -756,8 +756,12 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, > > if (!tb[IFLA_MTU]) > dev->mtu = real_dev->mtu; > - else if (dev->mtu > real_dev->mtu) > - return -EINVAL; > + else if (dev->mtu > real_dev->mtu) { > + netdev_dbg(dev, "GTP mtu greater that transport MTU (%d > %d)\n", "than" with n ~Neels
diff --git a/gtp.c b/gtp.c index 11f8fad..4f5729e 100644 --- a/gtp.c +++ b/gtp.c @@ -756,8 +756,12 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, if (!tb[IFLA_MTU]) dev->mtu = real_dev->mtu; - else if (dev->mtu > real_dev->mtu) - return -EINVAL; + else if (dev->mtu > real_dev->mtu) { + netdev_dbg(dev, "GTP mtu greater that transport MTU (%d > %d)\n", + dev->mtu, real_dev->mtu); + err = -EINVAL; + goto out_err; + } gti = netdev_priv(dev); gti->real_dev = real_dev; @@ -765,7 +769,9 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, fd0 = nla_get_u32(data[IFLA_GTP_FD0]); fd1 = nla_get_u32(data[IFLA_GTP_FD1]); - gtp_encap_enable(dev, gti, fd0, fd1); + err = gtp_encap_enable(dev, gti, fd0, fd1); + if (err < 0) + goto out_err; if (!data[IFLA_GTP_HASHSIZE]) hashsize = 1024; @@ -774,21 +780,29 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, err = gtp_hashtable_new(gti, hashsize); if (err < 0) - return err; + goto out_encap; err = register_netdevice(dev); - if (err < 0) - goto err1; + if (err < 0) { + netdev_dbg(dev, "failed to register new netdev %d\n", err); + goto out_hashtable; + } gn = net_generic(dev_net(dev), gtp_net_id); list_add_rcu(>i->list, &gn->gtp_instance_list); - netdev_dbg(dev, "registered new interface\n"); + netdev_dbg(dev, "registered new GTP interface\n"); return 0; -err1: - netdev_dbg(dev, "failed to register new netdev %d\n", err); + +out_hashtable: gtp_hashtable_free(gti); + +out_encap: + gtp_encap_disable(gti); + +out_err: + dev_put(real_dev); return err; }
Signed-off-by: Andreas Schultz <aschultz@tpip.net> --- gtp.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)