Fix IPv6 tunnel add when dev param is used
Subsystems (2)
PATCH 1/2 Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
In reply to Andrea ClaudiThis reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123. It breaks tunnel creation when using 'dev' parameter:
$ ip link add type dummy $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 add tunnel "ip6tnl0" failed: File exists
dev parameter must be used to specify the device to which the tunnel is binded, and not the tunnel itself.
--- ip/ip6tunnel.c | 2 -- 1 file changed, 2 deletions(-)
ip/ip6tunnel.c +0 -2
@@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) p->link =ll_name_to_index (medium);if (!p->link)return nodev (medium); -else -strlcpy (p->name, medium,sizeof (p->name)); }return 0 ; }
Signature · 1 line
2.20.1
cover Fix IPv6 tunnel add when dev param is used
Commit ba126dcad20e6 ("ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds") breaks IPv6 tunnel creation when dev parameter is used.
This series revert the original commit, which mistakenly use dev for tunnel name, while addressing a issue on tunnel change when no interface name is specified.
Andrea Claudi (2):
Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds" ip tunnel: warn when changing IPv6 tunnel without tunnel name
ip/ip6tunnel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Signature · 1 line
2.20.1
PATCH 2/2 ip tunnel: warn when changing IPv6 tunnel without tunnel name
Tunnel change fails if a tunnel name is not specified while using 'ip -6 tunnel change'. However, no warning message is printed and no error code is returned.
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
This commit checks if tunnel interface name is equal to an empty
string: in this case, it prints a warning message to the user. It intentionally avoids to return an error to not break existing script setup.
This is the output after this commit:
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 Tunnel interface name not specified $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
--- ip/ip6tunnel.c | 3 +++ 1 file changed, 3 insertions(+)
ip/ip6tunnel.c +3 -0
@@ -386,6 +386,9 @@ static int do_add(int cmd, int argc, char **argv)if (parse_args (argc, argv, cmd, &p) <0 )return -1 ; +if (!*p.name) +fprintf (stderr,"Tunnel interface name not specified\n" ); +if (p.proto ==IPPROTO_GRE ) basedev ="ip6gre0" ;else if (p.i_flags &VTI_ISVTI )
Signature · 1 line
2.20.1
Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
In reply to Andrea ClaudiQuoted 10 lines · Andrea Claudi
This reverts commit
ba126dcad20e6d0e472586541d78bdd1ac4f1123. It breaks tunnel creation when using 'dev' parameter:$ ip link add type dummy $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 add tunnel "ip6tnl0" failed: File existsdev parameter must be used to specify the device to which the tunnel is binded, and not the tunnel itself.
Stephen: since this reverts a commit in 5.2 it should be in 5.2 as well.
Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
In reply to Andrea ClaudiQuoted 23 lines · Andrea Claudi
This reverts commit
ba126dcad20e6d0e472586541d78bdd1ac4f1123. It breaks tunnel creation when using 'dev' parameter:$ ip link add type dummy $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 add tunnel "ip6tnl0" failed: File existsdev parameter must be used to specify the device to which the tunnel is binded, and not the tunnel itself.
Reported-by Jianwen Ji <[email protected]>Reviewed-by Matteo Croce <[email protected]>Signed-off-by Andrea Claudi <[email protected]>--- ip/ip6tunnel.c | 2 -- 1 file changed, 2 deletions(-)ip/ip6tunnel.c +0
-0
ip/ip6tunnel.c static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) · line 302
Quoted 5 lines
ip/ip6tunnel.c +0
-2@@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) p->link =ll_name_to_index (medium);if (!p->link)return nodev (medium); -else -strlcpy (p->name, medium,sizeof (p->name));
NACK
I see that ba126dcad20e6d0e472586541d78bdd1ac4f1123 has broke something but that doesn't mean revert of the original fix is correct way of fixing it. The original fix is fixing the show and change command. Shouldn't you try fixing the add command so that all (show, change, and add) work correctly?
Quoted 6 lines
ip/ip6tunnel.c +0
-1}return 0 ; } --2.20.1
Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
In reply to David AhernQuoted 8 lines · On Tue, 9 Jul 2019 11:14:00 -0600 David Ahern <[email protected]> wrote:
Quoted 7 lines · Andrea Claudi
This reverts commit
ba126dcad20e6d0e472586541d78bdd1ac4f1123. It breaks tunnel creation when using 'dev' parameter:$ ip link add type dummy $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 add tunnel "ip6tnl0" failed: File exists
Quoted 5 lines
Quoted 3 lines
dev parameter must be used to specify the device to which the tunnel is binded, and not the tunnel itself.
Stephen: since this reverts a commit in 5.2 it should be in 5.2 as well.
5.2 has been released. Probably not worth doing 5.2.1 for corner case like this.
Re: [PATCH iproute2 2/2] ip tunnel: warn when changing IPv6 tunnel without tunnel name
In reply to Andrea ClaudiQuoted 16 lines · Andrea Claudi
Tunnel change fails if a tunnel name is not specified while using 'ip -6 tunnel change'. However, no warning message is printed and no error code is returned.
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)This commit checks if tunnel interface name is equal to an empty
string: in this case, it prints a warning message to the user. It intentionally avoids to return an error to not break existing script setup.This is the output after this commit:
Quoted 8 lines
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 Tunnel interface name not specified $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)Reviewed-by Matteo Croce <[email protected]>Signed-off-by Andrea Claudi <[email protected]>
I tried your patch and the commands that I posted in my (previous) patch.
Here is the output after reverting my patch and applying your patch
<show command>
------------------------
vm0:/tmp# ./ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none vm0:/tmp# ./ip -6 tunnel show dev ip6tnl1 vm0:/tmp# echo $? 0
here the output is NULL and return code is 0. This is wrong and I would expect to see the tunnel info (as displayed in 'ip -6 tunnel show ip6tnl1')
<change command>
lpaa10:/tmp# ip -6 tunnel change dev ip6tnl1 local 2001:1234::1 remote 2001:1234::2 encaplimit none ttl 127 tos inherit allow-localremote lpaa10:/tmp# echo $? 0 lpaa10:/tmp# ip -6 tunnel show dev ip6tnl1 lpaa10:/tmp# ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
the change command appeared to be successful but change wasn't applied (expecting the allow-localremote to be present on the tunnel).
---
Quoted 20 lines
ip/ip6tunnel.c | 3 +++ 1 file changed, 3 insertions(+)ip/ip6tunnel.c +3
-1@@ -386,6 +386,9 @@ static int do_add(int cmd, int argc, char **argv)if (parse_args (argc, argv, cmd, &p) <0 )return -1 ; +if (!*p.name) +fprintf (stderr,"Tunnel interface name not specified\n" ); +if (p.proto ==IPPROTO_GRE ) basedev ="ip6gre0" ;else if (p.i_flags &VTI_ISVTI ) --2.20.1
Re: [PATCH iproute2 2/2] ip tunnel: warn when changing IPv6 tunnel without tunnel name
Quoted 24 lines · Mahesh Bandewar (महेश बंडेवार)
Quoted 22 lines · Andrea Claudi
Tunnel change fails if a tunnel name is not specified while using 'ip -6 tunnel change'. However, no warning message is printed and no error code is returned.
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)This commit checks if tunnel interface name is equal to an empty
string: in this case, it prints a warning message to the user. It intentionally avoids to return an error to not break existing script setup.This is the output after this commit:
$ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0 $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2 Tunnel interface name not specified $ ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
Quoted 5 lines
Quoted 2 lines
Reviewed-by Matteo Croce <[email protected]>Signed-off-by Andrea Claudi <[email protected]>I tried your patch and the commands that I posted in my (previous) patch.
Hi Mahesh,
Thank you for taking the time to review my patch.
Quoted 10 lines
Here is the output after reverting my patch and applying your patch
<show command>
------------------------vm0:/tmp# ./ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none vm0:/tmp# ./ip -6 tunnel show dev ip6tnl1 vm0:/tmp# echo $? 0
Quoted 3 lines
here the output is NULL and return code is 0. This is wrong and I would expect to see the tunnel info (as displayed in 'ip -6 tunnel show ip6tnl1')
It seems to me there is a bit of misunderstanding here. Looking at man page for ip tunnel:
dev NAME
bind the tunnel to the device NAME so that tunneled packets
will only be routed via this device and will not be able to escape to another device when the route to endpoint changes.
From what I read, dev parameter should not be used as an alias to the tunnel device, but to indicate the device to which the tunnel should be binded.
As such, ip -6 tunnel show dev <name> is a legitimate query that must show the tunnel device(s) binded to <name> interface.
With the query ip -6 tunnel show <name> you instead obtain the tunnel itself.
By the way, the proper selector for the tunnel device name is the default "name" parameter. From the man page:
name NAME (default)
select the tunnel device name.
For example:
$ ip -6 tunnel show name ip6tnl0 ip6tnl0: ipv6/ipv6 remote :: local :: encaplimit 0 hoplimit inherit tclass 0x00 flowlabel 0x00000 (flowinfo 0x00000000)
Quoted 10 lines
<change command>
lpaa10:/tmp# ip -6 tunnel change dev ip6tnl1 local 2001:1234::1 remote 2001:1234::2 encaplimit none ttl 127 tos inherit allow-localremote lpaa10:/tmp# echo $? 0 lpaa10:/tmp# ip -6 tunnel show dev ip6tnl1 lpaa10:/tmp# ip -6 tunnel show ip6tnl1 ip6tnl1: gre/ipv6 remote fd::2 local fd::1 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
Quoted 2 lines
the change command appeared to be successful but change wasn't applied (expecting the allow-localremote to be present on the tunnel).
As you can read from the commit message, here I am not changing the return code intentionally to not break existing script setup. However, I can easily change this to return an error code in a v2.
Quoted 12 lines
---
Quoted 11 lines
ip/ip6tunnel.c | 3 +++ 1 file changed, 3 insertions(+)ip/ip6tunnel.c +0
-0@@ -386,6 +386,9 @@ static int do_add(int cmd, int argc, char **argv)if (parse_args (argc, argv, cmd, &p) <0 )return -1 ;
Quoted 9 lines
Quoted 9 lines
ip/ip6tunnel.c +3
-1+if (!*p.name) +fprintf (stderr,"Tunnel interface name not specified\n" ); +if (p.proto ==IPPROTO_GRE ) basedev ="ip6gre0" ;else if (p.i_flags &VTI_ISVTI ) --2.20.1
Regards,
Andrea
Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
In reply to Mahesh Bandewar (महेश बंडेवार)Quoted 32 lines · Mahesh Bandewar (महेश बंडेवार)
Quoted 23 lines · Andrea Claudi
This reverts commit
ba126dcad20e6d0e472586541d78bdd1ac4f1123. It breaks tunnel creation when using 'dev' parameter:$ ip link add type dummy $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 add tunnel "ip6tnl0" failed: File existsdev parameter must be used to specify the device to which the tunnel is binded, and not the tunnel itself.
Reported-by Jianwen Ji <[email protected]>Reviewed-by Matteo Croce <[email protected]>Signed-off-by Andrea Claudi <[email protected]>--- ip/ip6tunnel.c | 2 -- 1 file changed, 2 deletions(-)ip/ip6tunnel.c +0
-0ip/ip6tunnel.c static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) · line 302
Quoted 5 lines
ip/ip6tunnel.c +0
-2@@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) p->link =ll_name_to_index (medium);if (!p->link)return nodev (medium); -else -strlcpy (p->name, medium,sizeof (p->name));NACK
Quoted 6 lines
I see that
ba126dcad20e6d0e472586541d78bdd1ac4f1123has broke something but that doesn't mean revert of the original fix is correct way of fixing it. The original fix is fixing the show and change command. Shouldn't you try fixing the add command so that all (show, change, and add) work correctly?
Hi Mahesh,
Thanks for sharing your opinion. I think I already answered this replying to your review of patch 2/2 of this series.
To summarize that up I think there is a misunderstanding on the meaning of "dev" param, and "name" param (which is the default) must be used instead in the cases highlighted in your commit.
Regards,
Andrea
Quoted 6 lines
Quoted 6 lines
ip/ip6tunnel.c +0
-1}return 0 ; } --2.20.1
Quoted 8 lines · On Tue, 9 Jul 2019 15:16:49 +0200 Andrea Claudi <[email protected]> wrote:
Commit
ba126dcad20e6("ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds") breaks IPv6 tunnel creation when dev parameter is used.This series revert the original commit, which mistakenly use dev for tunnel name, while addressing a issue on tunnel change when no interface name is specified.
Quoted 7 lines
Andrea Claudi (2):
Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds" ip tunnel: warn when changing IPv6 tunnel without tunnel nameip/ip6tunnel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Both applied, thanks