* [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.