[PATCH] vti: remove GRE_KEY flag for vti tunnel

[PATCH] vti: remove GRE_KEY flag for vti tunnel

[Hangbin Liu]

vti tunnel use IPPROTO_IPIP instead of IPPROTO_GRE, and keys are not allowed
with ipip tunnel. So there is no reason to set GRE_KEY flag for vti.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/ip_vti.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 52b802a..58c4e6a 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -185,10 +185,8 @@ vti_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	if (err)
 		return err;
 
-	if (cmd != SIOCDELTUNNEL) {
-		p.i_flags |= GRE_KEY | VTI_ISVTI;
-		p.o_flags |= GRE_KEY;
-	}
+	if (cmd != SIOCDELTUNNEL)
+		p.i_flags |= VTI_ISVTI;
 
 	if (copy_to_user(ifr->ifr_ifru.ifru_data, &p, sizeof(p)))
 		return -EFAULT;
-- 
1.8.1.4

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Christophe Gouault]

Hello Hangbin,

vti interfaces precisely need an o_key to be configured (it must be set
to the mark of ipsec policies attached to this interface). Consequently,
this flag must not be removed.

Best Regards,
Christophe

On 12/04/2013 09:48 AM, Hangbin Liu wrote:
> vti tunnel use IPPROTO_IPIP instead of IPPROTO_GRE, and keys are not allowed
> with ipip tunnel. So there is no reason to set GRE_KEY flag for vti.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   net/ipv4/ip_vti.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 52b802a..58c4e6a 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -185,10 +185,8 @@ vti_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>   	if (err)
>   		return err;
>
> -	if (cmd != SIOCDELTUNNEL) {
> -		p.i_flags |= GRE_KEY | VTI_ISVTI;
> -		p.o_flags |= GRE_KEY;
> -	}
> +	if (cmd != SIOCDELTUNNEL)
> +		p.i_flags |= VTI_ISVTI;
>
>   	if (copy_to_user(ifr->ifr_ifru.ifru_data, &p, sizeof(p)))
>   		return -EFAULT;
>

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Steffen Klassert]

On Wed, Dec 04, 2013 at 01:46:40PM +0100, Christophe Gouault wrote:
> Hello Hangbin,
> 
> vti interfaces precisely need an o_key to be configured (it must be set
> to the mark of ipsec policies attached to this interface). Consequently,
> this flag must not be removed.
> 

Why do we need to set these flags in vti_tunnel_ioctl()? All we do
here, is to cheat userspace. The userspace should set these flags
to configure the tunnel. Please note that we completely ignore
the flags from userspace, instead we set our own flags.

I think the only reason why we set these flags here, is to make iproute2
to print the i_key/o_key.

I've already metntioned that I'am a bit disappointed on how the gre
keys and flags are used with vti. But I fear we need to keep it
as it is for now, because it would at least change the behaviour
of iproute2 if we remove the flags.

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Hangbin Liu]

On Thu, Dec 05, 2013 at 09:47:27AM +0100, Steffen Klassert wrote:
> On Wed, Dec 04, 2013 at 01:46:40PM +0100, Christophe Gouault wrote:
> > Hello Hangbin,
> > 
> > vti interfaces precisely need an o_key to be configured (it must be set
> > to the mark of ipsec policies attached to this interface). Consequently,
> > this flag must not be removed.
> > 
> 
> Why do we need to set these flags in vti_tunnel_ioctl()? All we do
> here, is to cheat userspace. The userspace should set these flags
> to configure the tunnel. Please note that we completely ignore
> the flags from userspace, instead we set our own flags.
> 
> I think the only reason why we set these flags here, is to make iproute2
> to print the i_key/o_key.
> 
> I've already metntioned that I'am a bit disappointed on how the gre
> keys and flags are used with vti. But I fear we need to keep it
> as it is for now, because it would at least change the behaviour
> of iproute2 if we remove the flags.

I submit this patch just beacuse iproute2 will return fail when we modify vti
parameters. Code here

ip/iptunnel.c#L226:
        if (p->iph.protocol == IPPROTO_IPIP || p->iph.protocol == IPPROTO_IPV6) {
                if ((p->i_flags & GRE_KEY) || (p->o_flags & GRE_KEY)) {
                        fprintf(stderr, "Keys are not allowed with ipip and sit tunnels\n");
                        return -1;
                }
        }

So should we modify iproute2 or kernel code?
-- 

Thanks & Best Regards
Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Hangbin Liu]

On Wed, Dec 04, 2013 at 01:46:40PM +0100, Christophe Gouault wrote:
> Hello Hangbin,
> 
> vti interfaces precisely need an o_key to be configured (it must be set
> to the mark of ipsec policies attached to this interface). Consequently,
> this flag must not be removed.

I saw the o_key was used here, do you mean this? I'm not clearly understand
xfrm4_policy_check(), does it really need GRE_KEY? or any value is ok?

static int vti_rcv(struct sk_buff *skb)
{
        struct ip_tunnel *tunnel;
        const struct iphdr *iph = ip_hdr(skb);
        struct net *net = dev_net(skb->dev);
        struct ip_tunnel_net *itn = net_generic(net, vti_net_id);

        tunnel = ip_tunnel_lookup(itn, skb->dev->ifindex, TUNNEL_NO_KEY,
                                  iph->saddr, iph->daddr, 0);
        if (tunnel != NULL) {
                struct pcpu_tstats *tstats;
                u32 oldmark = skb->mark;
                int ret;


                /* temporarily mark the skb with the tunnel o_key, to
                 * only match policies with this mark.
                 */
                skb->mark = be32_to_cpu(tunnel->parms.o_key);
                ret = xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb);
                skb->mark = oldmark;


> 
> Best Regards,
> Christophe
> 
> On 12/04/2013 09:48 AM, Hangbin Liu wrote:
> >vti tunnel use IPPROTO_IPIP instead of IPPROTO_GRE, and keys are not allowed
> >with ipip tunnel. So there is no reason to set GRE_KEY flag for vti.
> >
> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> >---
> >  net/ipv4/ip_vti.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> >index 52b802a..58c4e6a 100644
> >--- a/net/ipv4/ip_vti.c
> >+++ b/net/ipv4/ip_vti.c
> >@@ -185,10 +185,8 @@ vti_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> >  	if (err)
> >  		return err;
> >
> >-	if (cmd != SIOCDELTUNNEL) {
> >-		p.i_flags |= GRE_KEY | VTI_ISVTI;
> >-		p.o_flags |= GRE_KEY;
> >-	}
> >+	if (cmd != SIOCDELTUNNEL)
> >+		p.i_flags |= VTI_ISVTI;
> >
> >  	if (copy_to_user(ifr->ifr_ifru.ifru_data, &p, sizeof(p)))
> >  		return -EFAULT;
> >

-- 

Thanks & Best Regards
Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Steffen Klassert]

On Thu, Dec 05, 2013 at 05:47:08PM +0800, Hangbin Liu wrote:
> On Thu, Dec 05, 2013 at 09:47:27AM +0100, Steffen Klassert wrote:
> > 
> > I've already metntioned that I'am a bit disappointed on how the gre
> > keys and flags are used with vti. But I fear we need to keep it
> > as it is for now, because it would at least change the behaviour
> > of iproute2 if we remove the flags.
> 
> I submit this patch just beacuse iproute2 will return fail when we modify vti
> parameters. Code here
> 
> ip/iptunnel.c#L226:
>         if (p->iph.protocol == IPPROTO_IPIP || p->iph.protocol == IPPROTO_IPV6) {
>                 if ((p->i_flags & GRE_KEY) || (p->o_flags & GRE_KEY)) {
>                         fprintf(stderr, "Keys are not allowed with ipip and sit tunnels\n");
>                         return -1;
>                 }
>         }
> 
> So should we modify iproute2 or kernel code?

Update iproute2 to allow GRE_KEY if the VTI_ISVTI flag is set.

The handling of vti keys inside the kernel needs to be redesigned too,
but this requires a concept such that the existing userspace API still
works.

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Steffen Klassert]

On Thu, Dec 05, 2013 at 05:47:41PM +0800, Hangbin Liu wrote:
> On Wed, Dec 04, 2013 at 01:46:40PM +0100, Christophe Gouault wrote:
> > Hello Hangbin,
> > 
> > vti interfaces precisely need an o_key to be configured (it must be set
> > to the mark of ipsec policies attached to this interface). Consequently,
> > this flag must not be removed.
> 
> I saw the o_key was used here, do you mean this? I'm not clearly understand
> xfrm4_policy_check(), does it really need GRE_KEY? or any value is ok?

It does not need GRE_KEY at all, this flag is not even set on vti
tunnels. The vti key is just a mark that will be set on the skb.
It is used to match the right policy for that tunnel, so the policy
that should match must be configured to have the same mark.

Re: [PATCH] vti: remove GRE_KEY flag for vti tunnel

[Hangbin Liu]

On Thu, Dec 05, 2013 at 11:51:43AM +0100, Steffen Klassert wrote:
> On Thu, Dec 05, 2013 at 05:47:08PM +0800, Hangbin Liu wrote:
> > On Thu, Dec 05, 2013 at 09:47:27AM +0100, Steffen Klassert wrote:
> > > 
> > > I've already metntioned that I'am a bit disappointed on how the gre
> > > keys and flags are used with vti. But I fear we need to keep it
> > > as it is for now, because it would at least change the behaviour
> > > of iproute2 if we remove the flags.
> > 
> > I submit this patch just beacuse iproute2 will return fail when we modify vti
> > parameters. Code here
> > 
> > ip/iptunnel.c#L226:
> >         if (p->iph.protocol == IPPROTO_IPIP || p->iph.protocol == IPPROTO_IPV6) {
> >                 if ((p->i_flags & GRE_KEY) || (p->o_flags & GRE_KEY)) {
> >                         fprintf(stderr, "Keys are not allowed with ipip and sit tunnels\n");
> >                         return -1;
> >                 }
> >         }
> > 
> > So should we modify iproute2 or kernel code?
> 
> Update iproute2 to allow GRE_KEY if the VTI_ISVTI flag is set.

OK, I will resubmit a patch for iproute2

> 
> The handling of vti keys inside the kernel needs to be redesigned too,
> but this requires a concept such that the existing userspace API still
> works.

-- 

Thanks & Best Regards
Hangbin Liu <liuhangbin@gmail.com>