diff mbox

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

Message ID 1372472672-1900-1-git-send-email-amwang@redhat.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Amerigo Wang June 29, 2013, 2:24 a.m. UTC 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pravin B Shelar June 29, 2013, 3:28 a.m. UTC | #1 
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;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang June 29, 2013, 3:33 a.m. UTC | #2 
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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang June 29, 2013, 3:55 a.m. UTC | #3 
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?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov June 29, 2013, 3:52 p.m. UTC | #4 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang July 1, 2013, 2:08 a.m. UTC | #5 
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. ;)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;