* [PATCH net] ip6_tunnel: fix error code when tunnel exists
@ 2015-03-13 12:58 Nicolas Dichtel
2015-03-13 13:43 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dichtel @ 2015-03-13 12:58 UTC (permalink / raw)
To: davem; +Cc: netdev, me, Nicolas Dichtel, Steffen Klassert
After commit 2b0bb01b6edb, the kernel returns -ENOBUFS when user tries to add
an existing tunnel with ioctl API:
$ ip -6 tunnel add ip6tnl1 mode ip6ip6 dev eth1
add tunnel "ip6tnl0" failed: No buffer space available
It's confusing, the right error is EEXIST.
Fixes: 2b0bb01b6edb ("ip6_tunnel: Return an error when adding an existing tunnel.")
CC: Steffen Klassert <steffen.klassert@secunet.com>
Reported-by: Pierre Cheynier <me@pierre-cheynier.net>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/ip6_tunnel.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 266a264ec212..7ba5608d64c5 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -380,7 +380,7 @@ static struct ip6_tnl *ip6_tnl_locate(struct net *net,
if (ipv6_addr_equal(local, &t->parms.laddr) &&
ipv6_addr_equal(remote, &t->parms.raddr)) {
if (create)
- return NULL;
+ return ERR_PTR(-EEXIST);
return t;
}
@@ -1420,7 +1420,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
}
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, 0);
- if (t == NULL)
+ if (IS_ERR_OR_NULL(t))
t = netdev_priv(dev);
} else {
memset(&p, 0, sizeof(p));
@@ -1445,7 +1445,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, cmd == SIOCADDTUNNEL);
if (cmd == SIOCCHGTUNNEL) {
- if (t != NULL) {
+ if (!IS_ERR_OR_NULL(t)) {
if (t->dev != dev) {
err = -EEXIST;
break;
@@ -1457,14 +1457,17 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
else
err = ip6_tnl_update(t, &p1);
}
- if (t) {
+ if (!IS_ERR_OR_NULL(t)) {
err = 0;
ip6_tnl_parm_to_user(&p, &t->parms);
if (copy_to_user(ifr->ifr_ifru.ifru_data, &p, sizeof(p)))
err = -EFAULT;
- } else
+ } else if (IS_ERR(t)) {
+ err = PTR_ERR(t);
+ } else {
err = (cmd == SIOCADDTUNNEL ? -ENOBUFS : -ENOENT);
+ }
break;
case SIOCDELTUNNEL:
err = -EPERM;
@@ -1478,7 +1481,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
err = -ENOENT;
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, 0);
- if (t == NULL)
+ if (IS_ERR_OR_NULL(t))
break;
err = -EPERM;
if (t->dev == ip6n->fb_tnl_dev)
@@ -1672,12 +1675,13 @@ static int ip6_tnl_newlink(struct net *src_net, struct net_device *dev,
struct nlattr *tb[], struct nlattr *data[])
{
struct net *net = dev_net(dev);
- struct ip6_tnl *nt;
+ struct ip6_tnl *nt, *t;
nt = netdev_priv(dev);
ip6_tnl_netlink_parms(data, &nt->parms);
- if (ip6_tnl_locate(net, &nt->parms, 0))
+ t = ip6_tnl_locate(net, &nt->parms, 0);
+ if (!IS_ERR_OR_NULL(t))
return -EEXIST;
return ip6_tnl_create2(dev);
@@ -1698,7 +1702,7 @@ static int ip6_tnl_changelink(struct net_device *dev, struct nlattr *tb[],
t = ip6_tnl_locate(net, &p, 0);
- if (t) {
+ if (!IS_ERR_OR_NULL(t)) {
if (t->dev != dev)
return -EEXIST;
} else
--
2.2.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix error code when tunnel exists
2015-03-13 12:58 [PATCH net] ip6_tunnel: fix error code when tunnel exists Nicolas Dichtel
@ 2015-03-13 13:43 ` Eric Dumazet
2015-03-13 14:46 ` Nicolas Dichtel
2015-03-13 16:32 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Eric Dumazet @ 2015-03-13 13:43 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: davem, netdev, me, Steffen Klassert
On Fri, 2015-03-13 at 13:58 +0100, Nicolas Dichtel wrote:
> After commit 2b0bb01b6edb, the kernel returns -ENOBUFS when user tries to add
> an existing tunnel with ioctl API:
> $ ip -6 tunnel add ip6tnl1 mode ip6ip6 dev eth1
> add tunnel "ip6tnl0" failed: No buffer space available
>
> It's confusing, the right error is EEXIST.
>
> Fixes: 2b0bb01b6edb ("ip6_tunnel: Return an error when adding an existing tunnel.")
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> Reported-by: Pierre Cheynier <me@pierre-cheynier.net>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> net/ipv6/ip6_tunnel.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 266a264ec212..7ba5608d64c5 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -380,7 +380,7 @@ static struct ip6_tnl *ip6_tnl_locate(struct net *net,
> if (ipv6_addr_equal(local, &t->parms.laddr) &&
> ipv6_addr_equal(remote, &t->parms.raddr)) {
> if (create)
> - return NULL;
> + return ERR_PTR(-EEXIST);
>
> return t;
> }
> @@ -1420,7 +1420,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> }
> ip6_tnl_parm_from_user(&p1, &p);
> t = ip6_tnl_locate(net, &p1, 0);
> - if (t == NULL)
> + if (IS_ERR_OR_NULL(t))
These IS_ERR_OR_NULL(t) looks like defensive/lazy programming to me.
A NULL pointer should not be allowed here.
If t is not valid, it should be a plain error code mapping.
I wish we get rid of all IS_ERR_OR_NULL() uses in networking tree,
instead of adding plenty of them.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix error code when tunnel exists
2015-03-13 13:43 ` Eric Dumazet
@ 2015-03-13 14:46 ` Nicolas Dichtel
2015-03-13 16:32 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Nicolas Dichtel @ 2015-03-13 14:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, me, Steffen Klassert
Le 13/03/2015 14:43, Eric Dumazet a écrit :
[snip]
> These IS_ERR_OR_NULL(t) looks like defensive/lazy programming to me.
>
> A NULL pointer should not be allowed here.
>
> If t is not valid, it should be a plain error code mapping.
>
> I wish we get rid of all IS_ERR_OR_NULL() uses in networking tree,
> instead of adding plenty of them.
Ok, I agree. It was to minimize the patch.
I will rework it.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix error code when tunnel exists
2015-03-13 13:43 ` Eric Dumazet
2015-03-13 14:46 ` Nicolas Dichtel
@ 2015-03-13 16:32 ` David Miller
2015-03-16 14:56 ` [PATCH net v2] " Nicolas Dichtel
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2015-03-13 16:32 UTC (permalink / raw)
To: eric.dumazet; +Cc: nicolas.dichtel, netdev, me, steffen.klassert
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 13 Mar 2015 06:43:47 -0700
>
> These IS_ERR_OR_NULL(t) looks like defensive/lazy programming to me.
...
> I wish we get rid of all IS_ERR_OR_NULL() uses in networking tree,
> instead of adding plenty of them.
+1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net v2] ip6_tunnel: fix error code when tunnel exists
2015-03-13 16:32 ` David Miller
@ 2015-03-16 14:56 ` Nicolas Dichtel
2015-03-16 20:34 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dichtel @ 2015-03-16 14:56 UTC (permalink / raw)
To: davem; +Cc: netdev, me, eric.dumazet, Nicolas Dichtel, Steffen Klassert
After commit 2b0bb01b6edb, the kernel returns -ENOBUFS when user tries to add
an existing tunnel with ioctl API:
$ ip -6 tunnel add ip6tnl1 mode ip6ip6 dev eth1
add tunnel "ip6tnl0" failed: No buffer space available
It's confusing, the right error is EEXIST.
This patch also change a bit the code returned:
- ENOBUFS -> ENOMEM
- ENOENT -> ENODEV
Fixes: 2b0bb01b6edb ("ip6_tunnel: Return an error when adding an existing tunnel.")
CC: Steffen Klassert <steffen.klassert@secunet.com>
Reported-by: Pierre Cheynier <me@pierre-cheynier.net>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/ip6_tunnel.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 266a264ec212..ddd94eca19b3 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -314,7 +314,7 @@ out:
* Create tunnel matching given parameters.
*
* Return:
- * created tunnel or NULL
+ * created tunnel or error pointer
**/
static struct ip6_tnl *ip6_tnl_create(struct net *net, struct __ip6_tnl_parm *p)
@@ -322,7 +322,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net *net, struct __ip6_tnl_parm *p)
struct net_device *dev;
struct ip6_tnl *t;
char name[IFNAMSIZ];
- int err;
+ int err = -ENOMEM;
if (p->name[0])
strlcpy(name, p->name, IFNAMSIZ);
@@ -348,7 +348,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net *net, struct __ip6_tnl_parm *p)
failed_free:
ip6_dev_free(dev);
failed:
- return NULL;
+ return ERR_PTR(err);
}
/**
@@ -362,7 +362,7 @@ failed:
* tunnel device is created and registered for use.
*
* Return:
- * matching tunnel or NULL
+ * matching tunnel or error pointer
**/
static struct ip6_tnl *ip6_tnl_locate(struct net *net,
@@ -380,13 +380,13 @@ static struct ip6_tnl *ip6_tnl_locate(struct net *net,
if (ipv6_addr_equal(local, &t->parms.laddr) &&
ipv6_addr_equal(remote, &t->parms.raddr)) {
if (create)
- return NULL;
+ return ERR_PTR(-EEXIST);
return t;
}
}
if (!create)
- return NULL;
+ return ERR_PTR(-ENODEV);
return ip6_tnl_create(net, p);
}
@@ -1420,7 +1420,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
}
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, 0);
- if (t == NULL)
+ if (IS_ERR(t))
t = netdev_priv(dev);
} else {
memset(&p, 0, sizeof(p));
@@ -1445,7 +1445,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, cmd == SIOCADDTUNNEL);
if (cmd == SIOCCHGTUNNEL) {
- if (t != NULL) {
+ if (!IS_ERR(t)) {
if (t->dev != dev) {
err = -EEXIST;
break;
@@ -1457,14 +1457,15 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
else
err = ip6_tnl_update(t, &p1);
}
- if (t) {
+ if (!IS_ERR(t)) {
err = 0;
ip6_tnl_parm_to_user(&p, &t->parms);
if (copy_to_user(ifr->ifr_ifru.ifru_data, &p, sizeof(p)))
err = -EFAULT;
- } else
- err = (cmd == SIOCADDTUNNEL ? -ENOBUFS : -ENOENT);
+ } else {
+ err = PTR_ERR(t);
+ }
break;
case SIOCDELTUNNEL:
err = -EPERM;
@@ -1478,7 +1479,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
err = -ENOENT;
ip6_tnl_parm_from_user(&p1, &p);
t = ip6_tnl_locate(net, &p1, 0);
- if (t == NULL)
+ if (IS_ERR(t))
break;
err = -EPERM;
if (t->dev == ip6n->fb_tnl_dev)
@@ -1672,12 +1673,13 @@ static int ip6_tnl_newlink(struct net *src_net, struct net_device *dev,
struct nlattr *tb[], struct nlattr *data[])
{
struct net *net = dev_net(dev);
- struct ip6_tnl *nt;
+ struct ip6_tnl *nt, *t;
nt = netdev_priv(dev);
ip6_tnl_netlink_parms(data, &nt->parms);
- if (ip6_tnl_locate(net, &nt->parms, 0))
+ t = ip6_tnl_locate(net, &nt->parms, 0);
+ if (!IS_ERR(t))
return -EEXIST;
return ip6_tnl_create2(dev);
@@ -1697,8 +1699,7 @@ static int ip6_tnl_changelink(struct net_device *dev, struct nlattr *tb[],
ip6_tnl_netlink_parms(data, &p);
t = ip6_tnl_locate(net, &p, 0);
-
- if (t) {
+ if (!IS_ERR(t)) {
if (t->dev != dev)
return -EEXIST;
} else
--
2.2.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] ip6_tunnel: fix error code when tunnel exists
2015-03-16 14:56 ` [PATCH net v2] " Nicolas Dichtel
@ 2015-03-16 20:34 ` David Miller
2015-03-17 8:27 ` Nicolas Dichtel
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2015-03-16 20:34 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, me, eric.dumazet, steffen.klassert
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 16 Mar 2015 15:56:05 +0100
> @@ -1445,7 +1445,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> ip6_tnl_parm_from_user(&p1, &p);
> t = ip6_tnl_locate(net, &p1, cmd == SIOCADDTUNNEL);
> if (cmd == SIOCCHGTUNNEL) {
> - if (t != NULL) {
> + if (!IS_ERR(t)) {
> if (t->dev != dev) {
> err = -EEXIST;
> break;
Please convert that last assignment to "err = PTR_ERR(t);", thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] ip6_tunnel: fix error code when tunnel exists
2015-03-16 20:34 ` David Miller
@ 2015-03-17 8:27 ` Nicolas Dichtel
2015-03-17 19:02 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dichtel @ 2015-03-17 8:27 UTC (permalink / raw)
To: David Miller; +Cc: netdev, me, eric.dumazet, steffen.klassert
Le 16/03/2015 21:34, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Mon, 16 Mar 2015 15:56:05 +0100
>
>> @@ -1445,7 +1445,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>> ip6_tnl_parm_from_user(&p1, &p);
>> t = ip6_tnl_locate(net, &p1, cmd == SIOCADDTUNNEL);
>> if (cmd == SIOCCHGTUNNEL) {
>> - if (t != NULL) {
>> + if (!IS_ERR(t)) {
>> if (t->dev != dev) {
>> err = -EEXIST;
>> break;
>
> Please convert that last assignment to "err = PTR_ERR(t);", thanks.
It's not possible, the if() statement checks that 't' is *not* an error.
And in this case, we report to the user that the tunnel already exists.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] ip6_tunnel: fix error code when tunnel exists
2015-03-17 8:27 ` Nicolas Dichtel
@ 2015-03-17 19:02 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2015-03-17 19:02 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, me, eric.dumazet, steffen.klassert
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 17 Mar 2015 09:27:28 +0100
> Le 16/03/2015 21:34, David Miller a écrit :
>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Date: Mon, 16 Mar 2015 15:56:05 +0100
>>
>>> @@ -1445,7 +1445,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct
>>> ifreq *ifr, int cmd)
>>> ip6_tnl_parm_from_user(&p1, &p);
>>> t = ip6_tnl_locate(net, &p1, cmd == SIOCADDTUNNEL);
>>> if (cmd == SIOCCHGTUNNEL) {
>>> - if (t != NULL) {
>>> + if (!IS_ERR(t)) {
>>> if (t->dev != dev) {
>>> err = -EEXIST;
>>> break;
>>
>> Please convert that last assignment to "err = PTR_ERR(t);", thanks.
> It's not possible, the if() statement checks that 't' is *not* an
> error.
> And in this case, we report to the user that the tunnel already
> exists.
My bad, thanks for explaining.
Applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-03-17 19:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-13 12:58 [PATCH net] ip6_tunnel: fix error code when tunnel exists Nicolas Dichtel
2015-03-13 13:43 ` Eric Dumazet
2015-03-13 14:46 ` Nicolas Dichtel
2015-03-13 16:32 ` David Miller
2015-03-16 14:56 ` [PATCH net v2] " Nicolas Dichtel
2015-03-16 20:34 ` David Miller
2015-03-17 8:27 ` Nicolas Dichtel
2015-03-17 19:02 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.