[PATCH iproute2 0/2] Fix IPv6 tunnel add when dev param is used

[PATCH iproute2 0/2] Fix IPv6 tunnel add when dev param is used

[Andrea Claudi]

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(-)

-- 
2.20.1

[PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"

[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

dev parameter must be used to specify the device to which
the tunnel is binded, and not the tunnel itself.

Reported-by: Jianwen Ji <jiji@redhat.com>
Reviewed-by: Matteo Croce <mcroce@redhat.com>
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 ip/ip6tunnel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 56fd3466ed062..999408ed801b1 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -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;
 }
-- 
2.20.1

Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"

[David Ahern]

On 7/9/19 7:16 AM, Andrea Claudi wrote:
> 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
> 
> 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.

Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"

[Stephen Hemminger]

On Tue, 9 Jul 2019 11:14:00 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 7/9/19 7:16 AM, Andrea Claudi wrote:
> > 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
> > 
> > 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 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote:
>
> 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
>
> dev parameter must be used to specify the device to which
> the tunnel is binded, and not the tunnel itself.
>
> Reported-by: Jianwen Ji <jiji@redhat.com>
> Reviewed-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> ---
>  ip/ip6tunnel.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> index 56fd3466ed062..999408ed801b1 100644
> --- a/ip/ip6tunnel.c
> +++ b/ip/ip6tunnel.c
> @@ -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?

>         }
>         return 0;
>  }
> --
> 2.20.1
>

Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"

[Andrea Claudi]

On Tue, Jul 9, 2019 at 7:31 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote:
> >
> > 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
> >
> > dev parameter must be used to specify the device to which
> > the tunnel is binded, and not the tunnel itself.
> >
> > Reported-by: Jianwen Ji <jiji@redhat.com>
> > Reviewed-by: Matteo Croce <mcroce@redhat.com>
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> > ---
> >  ip/ip6tunnel.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> > index 56fd3466ed062..999408ed801b1 100644
> > --- a/ip/ip6tunnel.c
> > +++ b/ip/ip6tunnel.c
> > @@ -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?
>

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

> >         }
> >         return 0;
> >  }
> > --
> > 2.20.1
> >

[PATCH iproute2 2/2] ip tunnel: warn when changing IPv6 tunnel without tunnel name

[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)

Reviewed-by: Matteo Croce <mcroce@redhat.com>
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 ip/ip6tunnel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 999408ed801b1..e3da11eb4518e 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -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

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote:
>
> 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)
>
> Reviewed-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>

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).
---
>  ip/ip6tunnel.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> index 999408ed801b1..e3da11eb4518e 100644
> --- a/ip/ip6tunnel.c
> +++ b/ip/ip6tunnel.c
> @@ -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

[Andrea Claudi]

On Wed, Jul 10, 2019 at 12:15 AM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote:
> >
> > 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)
> >
> > Reviewed-by: Matteo Croce <mcroce@redhat.com>
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
>
> 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.

> 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')

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)

> <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).

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.

> ---
> >  ip/ip6tunnel.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> > index 999408ed801b1..e3da11eb4518e 100644
> > --- a/ip/ip6tunnel.c
> > +++ b/ip/ip6tunnel.c
> > @@ -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
> >

Regards,
Andrea

Re: [PATCH iproute2 0/2] Fix IPv6 tunnel add when dev param is used

[Stephen Hemminger]

On Tue,  9 Jul 2019 15:16:49 +0200
Andrea Claudi <aclaudi@redhat.com> 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.
> 
> 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(-)
> 

Both applied, thanks