[06/16] gtp: add error handling to gtp_newlink
diff mbox

Message ID 1447686417-3979-7-git-send-email-aschultz@tpip.net
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Andreas Schultz Nov. 16, 2015, 3:06 p.m. UTC
Signed-off-by: Andreas Schultz <aschultz@tpip.net>
---
 gtp.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

Comments

Pablo Neira Ayuso Nov. 16, 2015, 5:46 p.m. UTC | #1
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;
>  }
Andreas Schultz Nov. 16, 2015, 6:16 p.m. UTC | #2
----- 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;
> >  }
Pablo Neira Ayuso Nov. 16, 2015, 9:57 p.m. UTC | #3
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.
Neels Hofmeyr Nov. 18, 2015, 2:57 p.m. UTC | #4
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

Patch
diff mbox

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(&gti->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;
 }