[Patch net-next] gre: fix a regression in ioctl

[Patch net-next] gre: fix a regression in ioctl

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

This is a regression introduced by commit c54419321455631079c7d
("GRE: Refactor GRE tunneling code.") because previously we
only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
after that commit, the check is moved for all commands.

So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 

Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c326e86..354d78c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
-	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);
 
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 394cebc..eb9dd96 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
 	case SIOCADDTUNNEL:
 	case SIOCCHGTUNNEL:
+		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+		    ((p->i_flags|p->o_flags)&(GRE_VERSION|GRE_ROUTING)))
+			return -EINVAL;
+
 		err = -EPERM;
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			goto done;

Re: [Patch net-next] gre: fix a regression in ioctl

[Cong Wang]

On Sat, 2013-06-29 at 10:04 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> When testing GRE tunnel, I got:
> 
>  # ip tunnel show
>  get tunnel gre0 failed: Invalid argument
>  get tunnel gre1 failed: Invalid argument
> 
> This is a regression introduced by commit c54419321455631079c7d
> ("GRE: Refactor GRE tunneling code.") because previously we
> only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> after that commit, the check is moved for all commands.
> 
> So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.
> 
> After this patch I got:
> 
>  # ip tunnel show
>  gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
>  gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 
> 
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> ---
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index c326e86..354d78c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
>  
>  	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
>  		return -EFAULT;
> -	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
> -	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
> -	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
> -		return -EINVAL;
> -	}
>  	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
>  	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

Self-NAK. p.i_flags is converted to tunnel flags, therefore should
probably check TUNNEL_* instead of GRE_*.

I will send v2.

[Patch net-next v2] gre: fix a regression in ioctl

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

This is a regression introduced by commit c54419321455631079c7d
("GRE: Refactor GRE tunneling code.") because previously we
only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
after that commit, the check is moved for all commands.

So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 

Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
v2: check TUNNEL_* flags

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c326e86..354d78c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
-	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);
 
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 394cebc..dc7d7ac 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 
 	case SIOCADDTUNNEL:
 	case SIOCCHGTUNNEL:
+		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
+		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
+		    ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))
+			return -EINVAL;
+
 		err = -EPERM;
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			goto done;

Re: [Patch net-next v2] gre: fix a regression in ioctl

[Pravin Shelar]

On Fri, Jun 28, 2013 at 7:24 PM, Cong Wang <amwang@redhat.com> wrote:
> From: Cong Wang <amwang@redhat.com>
>
> When testing GRE tunnel, I got:
>
>  # ip tunnel show
>  get tunnel gre0 failed: Invalid argument
>  get tunnel gre1 failed: Invalid argument
>
> This is a regression introduced by commit c54419321455631079c7d
> ("GRE: Refactor GRE tunneling code.") because previously we
> only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> after that commit, the check is moved for all commands.
>

right, that API got changed. But these checks can not be added to
generic ip_tunnel layer, which is also used by other tunnel modules.
Can you keep that check in ip_gre module but do it only for add and
del tunnel commands?

> So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.
>
> After this patch I got:
>
>  # ip tunnel show
>  gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
>  gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit
>
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
> v2: check TUNNEL_* flags
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index c326e86..354d78c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -314,11 +314,6 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
>
>         if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
>                 return -EFAULT;
> -       if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
> -           p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
> -           ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
> -               return -EINVAL;
> -       }
>         p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
>         p.o_flags = gre_flags_to_tnl_flags(p.o_flags);
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 394cebc..dc7d7ac 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
>
>         case SIOCADDTUNNEL:
>         case SIOCCHGTUNNEL:
> +               if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
> +                   p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
> +                   ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))
> +                       return -EINVAL;
> +
>                 err = -EPERM;
>                 if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
>                         goto done;

Re: [Patch net-next v2] gre: fix a regression in ioctl

[Cong Wang]

On Fri, 2013-06-28 at 20:28 -0700, Pravin Shelar wrote:
> On Fri, Jun 28, 2013 at 7:24 PM, Cong Wang <amwang@redhat.com> wrote:
> > From: Cong Wang <amwang@redhat.com>
> >
> > When testing GRE tunnel, I got:
> >
> >  # ip tunnel show
> >  get tunnel gre0 failed: Invalid argument
> >  get tunnel gre1 failed: Invalid argument
> >
> > This is a regression introduced by commit c54419321455631079c7d
> > ("GRE: Refactor GRE tunneling code.") because previously we
> > only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> > after that commit, the check is moved for all commands.
> >
> 
> right, that API got changed. But these checks can not be added to
> generic ip_tunnel layer, which is also used by other tunnel modules.
> Can you keep that check in ip_gre module but do it only for add and
> del tunnel commands?

Yeah.

BTW, ipip has the same bug... I will send a patch for ipip too.

Re: [Patch net-next v2] gre: fix a regression in ioctl

[Cong Wang]

On Sat, 2013-06-29 at 11:33 +0800, Cong Wang wrote:
> BTW, ipip has the same bug... I will send a patch for ipip too.

Pravin,

Any reason to add the following check for ipip?

       if (p.i_key || p.o_key || p.i_flags || p.o_flags)
               return -EINVAL;

It seems the old code doesn't have this check, you added it in commit
fd58156e456d9f68fe04486be378d0bc93641532 (IPIP: Use ip-tunneling code.).

This breaks the API?

[Patch net-next v3] gre: fix a regression in ioctl

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

When testing GRE tunnel, I got:

 # ip tunnel show
 get tunnel gre0 failed: Invalid argument
 get tunnel gre1 failed: Invalid argument

This is a regression introduced by commit c54419321455631079c7d
("GRE: Refactor GRE tunneling code.") because previously we
only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
after that commit, the check is moved for all commands.

So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.

After this patch I got:

 # ip tunnel show
 gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
 gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 

Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
v2: check TUNNEL_* flags
v3: the check should be kept in gre module

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c326e86..1f6eab6 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -314,10 +314,11 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 
 	if (copy_from_user(&p, ifr->ifr_ifru.ifru_data, sizeof(p)))
 		return -EFAULT;
-	if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
-	    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
-	    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING))) {
-		return -EINVAL;
+	if (cmd == SIOCADDTUNNEL || cmd == SIOCCHGTUNNEL) {
+		if (p.iph.version != 4 || p.iph.protocol != IPPROTO_GRE ||
+		    p.iph.ihl != 5 || (p.iph.frag_off&htons(~IP_DF)) ||
+		    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING)))
+			return -EINVAL;
 	}
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);

Re: [Patch net-next v3] gre: fix a regression in ioctl

[Dmitry Kravkov]

On Sat, Jun 29, 2013 at 7:02 AM, Cong Wang <amwang@redhat.com> wrote:
> From: Cong Wang <amwang@redhat.com>
>
> When testing GRE tunnel, I got:
>
>  # ip tunnel show
>  get tunnel gre0 failed: Invalid argument
>  get tunnel gre1 failed: Invalid argument
>
> This is a regression introduced by commit c54419321455631079c7d
> ("GRE: Refactor GRE tunneling code.") because previously we
> only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> after that commit, the check is moved for all commands.
>
> So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.
>
> After this patch I got:
>
>  # ip tunnel show
>  gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
>  gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit
>
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
> v2: check TUNNEL_* flags
> v3: the check should be kept in gre module
>

Shouldn't this be addressed in net-tree?

Re: [Patch net-next v3] gre: fix a regression in ioctl

[Cong Wang]

On Sat, 2013-06-29 at 21:42 +0300, Dmitry Kravkov wrote:
> 
> Shouldn't this be addressed in net-tree?

Oh, yeah, I thought only net-next contains that commit...

David, please apply this to net (I tried, the patch can be applied to
net).

Thanks!

Re: [Patch net-next v3] gre: fix a regression in ioctl

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Sat, 29 Jun 2013 12:02:59 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> When testing GRE tunnel, I got:
> 
>  # ip tunnel show
>  get tunnel gre0 failed: Invalid argument
>  get tunnel gre1 failed: Invalid argument
> 
> This is a regression introduced by commit c54419321455631079c7d
> ("GRE: Refactor GRE tunneling code.") because previously we
> only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> after that commit, the check is moved for all commands.
> 
> So, just check for SIOCADDTUNNEL and SIOCCHGTUNNEL.
> 
> After this patch I got:
> 
>  # ip tunnel show
>  gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
>  gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit 
> 
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Applied and queued up for -stable, thanks.

Re: [Patch net-next v2] gre: fix a regression in ioctl

[Sergei Shtylyov]

Hello.

On 29-06-2013 6:24, Cong Wang wrote:

> From: Cong Wang <amwang@redhat.com>

> When testing GRE tunnel, I got:

>   # ip tunnel show
>   get tunnel gre0 failed: Invalid argument
>   get tunnel gre1 failed: Invalid argument

> This is a regression introduced by commit c54419321455631079c7d
> ("GRE: Refactor GRE tunneling code.") because previously we
> only check the parameters for SIOCADDTUNNEL and SIOCCHGTUNNEL,
> after that commit, the check is moved for all commands.

> So, just move it back inside SIOCADDTUNNEL and SIOCCHGTUNNEL.

> After this patch I got:

>   # ip tunnel show
>   gre0: gre/ip  remote any  local any  ttl inherit  nopmtudisc
>   gre1: gre/ip  remote 192.168.122.101  local 192.168.122.45  ttl inherit

> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
> v2: check TUNNEL_* flags

[...]
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 394cebc..dc7d7ac 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -712,6 +712,11 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
>
>   	case SIOCADDTUNNEL:
>   	case SIOCCHGTUNNEL:
> +		if (p->iph.version != 4 || p->iph.protocol != IPPROTO_GRE ||
> +		    p->iph.ihl != 5 || (p->iph.frag_off&htons(~IP_DF)) ||
> +		    ((p->i_flags|p->o_flags)&(TUNNEL_VERSION|TUNNEL_ROUTING)))

    Maybe it's time to insert spaces around & to make the code formatted 
consistently?

WBR, Sergei

Re: [Patch net-next v2] gre: fix a regression in ioctl

[Cong Wang]

On Sat, 2013-06-29 at 19:52 +0400, Sergei Shtylyov wrote:
> 
>     Maybe it's time to insert spaces around & to make the code formatted 
> consistently?

Hi, Sergei

You are always welcome to send a _pure_ coding-style fix, not just for
these few lines. ;)