Message ID | 1372472672-1900-1-git-send-email-amwang@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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
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
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 --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;