[net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Alexander Duyck]

This change makes it so that we clear the skb->mark field when we pass
through the receive path of the IPv4 or IPv6 virtual tunnel interface.  The
reason for clearing these fields is to resolve an apparent regression for
the behavior before skb_scrub_packet was modified.  Without this patch I
have to set disable_policy for the vti tunnel endpoint in order to be able
to receive traffic.

Fixes: 213dd74aee76 ("skbuff: Do not scrub skb mark within the same name space")
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---

I have only tested the ipv4 side of this patch as I have yet to be able to
get a message to successfully pass between to ipv6 vti endpoints.

 net/ipv4/ip_vti.c  |    1 +
 net/ipv6/ip6_vti.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index ee479495f5a3..d853e78742d3 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -112,6 +112,7 @@ static int vti_rcv_cb(struct sk_buff *skb, int err)
 
 	skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(skb->dev)));
 	skb->dev = dev;
+	skb->mark = 0;
 
 	tstats = this_cpu_ptr(dev->tstats);
 
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index ed9d681207fa..c245fb8298e5 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -363,6 +363,7 @@ static int vti6_rcv_cb(struct sk_buff *skb, int err)
 
 	skb_scrub_packet(skb, !net_eq(t->net, dev_net(skb->dev)));
 	skb->dev = dev;
+	skb->mark = 0;
 
 	tstats = this_cpu_ptr(dev->tstats);
 	u64_stats_update_begin(&tstats->syncp);

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Wed, May 13, 2015 at 07:04:28PM -0700, Alexander Duyck wrote:
> This change makes it so that we clear the skb->mark field when we pass
> through the receive path of the IPv4 or IPv6 virtual tunnel interface.  The
> reason for clearing these fields is to resolve an apparent regression for
> the behavior before skb_scrub_packet was modified.  Without this patch I
> have to set disable_policy for the vti tunnel endpoint in order to be able
> to receive traffic.
> 
> Fixes: 213dd74aee76 ("skbuff: Do not scrub skb mark within the same name space")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>

This patch makes no sense.  Please explain your problem more
clearly and tell us why the mark changes the way your packet
is dealt with and why this isn't a policy decision that should
be made in user-space.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Alexander Duyck]

On 05/13/2015 08:28 PM, Herbert Xu wrote:
> On Wed, May 13, 2015 at 07:04:28PM -0700, Alexander Duyck wrote:
>> This change makes it so that we clear the skb->mark field when we pass
>> through the receive path of the IPv4 or IPv6 virtual tunnel interface.  The
>> reason for clearing these fields is to resolve an apparent regression for
>> the behavior before skb_scrub_packet was modified.  Without this patch I
>> have to set disable_policy for the vti tunnel endpoint in order to be able
>> to receive traffic.
>>
>> Fixes: 213dd74aee76 ("skbuff: Do not scrub skb mark within the same name space")
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> This patch makes no sense.  Please explain your problem more
> clearly and tell us why the mark changes the way your packet
> is dealt with and why this isn't a policy decision that should
> be made in user-space.
>
> Cheers,

The problem is if I set up a ipsec tunnel with vti endpoints on the 
current net-next I am unable to ping the other side.  What it looks like 
is the packet makes it past the endpoint (I can see the ICMP request in 
tcpdump), but the stack appears to be dropping the frame due to a policy 
check.  With my patch applied this issue is resolved, same thing for if 
I revert the "Fixes" patch, or if I set disable_policy for the vti endpoint.

The fact is I am not all that familiar with the vti code and just 
started crawling through it a few days ago, but it seems like it is 
overwriting the skb->mark value with the i_key to determine which policy 
to use.  The code prior to commit df3893c176e9 ("vti: Update the ipv4 
side to use it's own receive hook.") was saving the old skb->mark, 
overwriting it, and then restoring it after a call to 
xfrm4_policy_check.  After that commit it was letting skb_scrub_packet 
in vti_rcv_cb clear the mark and it was just dropped.

I suppose if we are wanting to get back to the behavior before the 
receive hook change we could look at maybe storing the previous mark in 
the skb->cb assuming there is any room there for it, and then we could 
restore it in vti_rcv_cb.

- Alex

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Wed, May 13, 2015 at 11:14:39PM -0700, Alexander Duyck wrote:
> 
> The fact is I am not all that familiar with the vti code and just
> started crawling through it a few days ago, but it seems like it is
> overwriting the skb->mark value with the i_key to determine which
> policy to use.  The code prior to commit df3893c176e9 ("vti: Update
> the ipv4 side to use it's own receive hook.") was saving the old
> skb->mark, overwriting it, and then restoring it after a call to
> xfrm4_policy_check.  After that commit it was letting
> skb_scrub_packet in vti_rcv_cb clear the mark and it was just
> dropped.

Steffen, why is vti touching skb->mark at all? This is supposed
to be a field used by user-space to control a packet as it moves
inside the kernel.  Seconding it for other purposes looks very
wrong.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 14 May 2015 14:26:14 +0800

> On Wed, May 13, 2015 at 11:14:39PM -0700, Alexander Duyck wrote:
>> 
>> The fact is I am not all that familiar with the vti code and just
>> started crawling through it a few days ago, but it seems like it is
>> overwriting the skb->mark value with the i_key to determine which
>> policy to use.  The code prior to commit df3893c176e9 ("vti: Update
>> the ipv4 side to use it's own receive hook.") was saving the old
>> skb->mark, overwriting it, and then restoring it after a call to
>> xfrm4_policy_check.  After that commit it was letting
>> skb_scrub_packet in vti_rcv_cb clear the mark and it was just
>> dropped.
> 
> Steffen, why is vti touching skb->mark at all? This is supposed
> to be a field used by user-space to control a packet as it moves
> inside the kernel.  Seconding it for other purposes looks very
> wrong.

If anything, the skb_scrub_packet() call right above the skb->mark
clears should be taking care of this.

The only case where mark should be cleared is if we are changing
namespaces, and that's exactly the policy implemented by
skb_scrub_packet() currently.

Yeah, this mark handling via tunnel->parms.o_key looks not so good.

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Alexander Duyck]

On 05/15/2015 09:37 AM, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 14 May 2015 14:26:14 +0800
>
>> On Wed, May 13, 2015 at 11:14:39PM -0700, Alexander Duyck wrote:
>>> The fact is I am not all that familiar with the vti code and just
>>> started crawling through it a few days ago, but it seems like it is
>>> overwriting the skb->mark value with the i_key to determine which
>>> policy to use.  The code prior to commit df3893c176e9 ("vti: Update
>>> the ipv4 side to use it's own receive hook.") was saving the old
>>> skb->mark, overwriting it, and then restoring it after a call to
>>> xfrm4_policy_check.  After that commit it was letting
>>> skb_scrub_packet in vti_rcv_cb clear the mark and it was just
>>> dropped.
>> Steffen, why is vti touching skb->mark at all? This is supposed
>> to be a field used by user-space to control a packet as it moves
>> inside the kernel.  Seconding it for other purposes looks very
>> wrong.
> If anything, the skb_scrub_packet() call right above the skb->mark
> clears should be taking care of this.

That only applies if you are crossing namespaces which we are not in 
this case.

> The only case where mark should be cleared is if we are changing
> namespaces, and that's exactly the policy implemented by
> skb_scrub_packet() currently.

Right.  The problem is it looks like vti and vti6 are using the mark to 
signal to the policy that is meant to be used for either end of the 
tunnel.  From what I can tell at some point there was a pre-routing hook 
that was used but later it was replaced with the i_key for input, and 
o_key for output.

> Yeah, this mark handling via tunnel->parms.o_key looks not so good.

So is there any recommendations for an alternative to make it so that 
the ipsec endpoint is identified as needing to be encrypted or 
decrypted?  If needed I could probably take a day or two to try and 
address it as I still have a few other minor things I want to try and 
fix such as the MTU configuration for vti/vti6.

- Alex

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Fri, May 15, 2015 at 12:14:43PM -0700, Alexander Duyck wrote:
>
> >Yeah, this mark handling via tunnel->parms.o_key looks not so good.
> 
> So is there any recommendations for an alternative to make it so
> that the ipsec endpoint is identified as needing to be encrypted or
> decrypted?  If needed I could probably take a day or two to try and
> address it as I still have a few other minor things I want to try
> and fix such as the MTU configuration for vti/vti6.

I'd like to hear from Steffen as to whether there is anything
in userspace that relies on the mark being used in this way by
vti.  If not it should be easy to get rid of it and use some
field that's not exposed to user-space.  If there is then this
would be tricky to resolve.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 16 May 2015 20:34:58 +0800

> On Fri, May 15, 2015 at 12:14:43PM -0700, Alexander Duyck wrote:
>>
>> >Yeah, this mark handling via tunnel->parms.o_key looks not so good.
>> 
>> So is there any recommendations for an alternative to make it so
>> that the ipsec endpoint is identified as needing to be encrypted or
>> decrypted?  If needed I could probably take a day or two to try and
>> address it as I still have a few other minor things I want to try
>> and fix such as the MTU configuration for vti/vti6.
> 
> I'd like to hear from Steffen as to whether there is anything
> in userspace that relies on the mark being used in this way by
> vti.  If not it should be easy to get rid of it and use some
> field that's not exposed to user-space.  If there is then this
> would be tricky to resolve.

The mark stuff comes from the commit below, and at the time I remember
pushing back a few times because I was uneasy about applying this
change:

====================
commit 7263a5187f9e9de45fcb51349cf0e031142c19a1
Author: Christophe Gouault <christophe.gouault@6wind.com>
Date:   Tue Oct 8 17:21:22 2013 +0200

    vti: get rid of nf mark rule in prerouting
    
    This patch fixes and improves the use of vti interfaces (while
    lightly changing the way of configuring them).
    
    Currently:
    
    - it is necessary to identify and mark inbound IPsec
      packets destined to each vti interface, via netfilter rules in
      the mangle table at prerouting hook.
    
    - the vti module cannot retrieve the right tunnel in input since
      commit b9959fd3: vti tunnels all have an i_key, but the tunnel lookup
      is done with flag TUNNEL_NO_KEY, so there no chance to retrieve them.
    
    - the i_key is used by the outbound processing as a mark to lookup
      for the right SP and SA bundle.
    
    This patch uses the o_key to store the vti mark (instead of i_key) and
    enables:
    
    - to avoid the need for previously marking the inbound skbuffs via a
      netfilter rule.
    - to properly retrieve the right tunnel in input, only based on the IPsec
      packet outer addresses.
    - to properly perform an inbound policy check (using the tunnel o_key
      as a mark).
    - to properly perform an outbound SPD and SAD lookup (using the tunnel
      o_key as a mark).
    - to keep the current mark of the skbuff. The skbuff mark is neither
      used nor changed by the vti interface. Only the vti interface o_key
      is used.
    
    SAs have a wildcard mark.
    SPs have a mark equal to the vti interface o_key.
    
    The vti interface must be created as follows (i_key = 0, o_key = mark):
    
       ip link add vti1 mode vti local 1.1.1.1 remote 2.2.2.2 okey 1
    
    The SPs attached to vti1 must be created as follows (mark = vti1 o_key):
    
       ip xfrm policy add dir out mark 1 tmpl src 1.1.1.1 dst 2.2.2.2 \
          proto esp mode tunnel
       ip xfrm policy add dir in  mark 1 tmpl src 2.2.2.2 dst 1.1.1.1 \
          proto esp mode tunnel
    
    The SAs are created with the default wildcard mark. There is no
    distinction between global vs. vti SAs. Just their addresses will
    possibly link them to a vti interface:
    
       ip xfrm state add src 1.1.1.1 dst 2.2.2.2 proto esp spi 1000 mode tunnel \
                     enc "cbc(aes)" "azertyuiopqsdfgh"
    
       ip xfrm state add src 2.2.2.2 dst 1.1.1.1 proto esp spi 2000 mode tunnel \
                     enc "cbc(aes)" "sqbdhgqsdjqjsdfh"
    
    To avoid matching "global" (not vti) SPs in vti interfaces, global SPs
    should no use the default wildcard mark, but explicitly match mark 0.
    
    To avoid a double SPD lookup in input and output (in global and vti SPDs),
    the NOPOLICY and NOXFRM options should be set on the vti interfaces:
    
       echo 1 > /proc/sys/net/ipv4/conf/vti1/disable_policy
       echo 1 > /proc/sys/net/ipv4/conf/vti1/disable_xfrm
    
    The outgoing traffic is steered to vti1 by a route via the vti interface:
    
       ip route add 192.168.0.0/16 dev vti1
    
    The incoming IPsec traffic is steered to vti1 because its outer addresses
    match the vti1 tunnel configuration.
    
    Signed-off-by: Christophe Gouault <christophe.gouault@6wind.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index e805e7b..6e87f85 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -125,8 +125,17 @@ static int vti_rcv(struct sk_buff *skb)
 				  iph->saddr, iph->daddr, 0);
 	if (tunnel != NULL) {
 		struct pcpu_tstats *tstats;
+		u32 oldmark = skb->mark;
+		int ret;
 
-		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
+
+		/* 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;
+		if (!ret)
 			return -1;
 
 		tstats = this_cpu_ptr(tunnel->dev->tstats);
@@ -135,7 +144,6 @@ static int vti_rcv(struct sk_buff *skb)
 		tstats->rx_bytes += skb->len;
 		u64_stats_update_end(&tstats->syncp);
 
-		skb->mark = 0;
 		secpath_reset(skb);
 		skb->dev = tunnel->dev;
 		return 1;
@@ -167,7 +175,7 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	memset(&fl4, 0, sizeof(fl4));
 	flowi4_init_output(&fl4, tunnel->parms.link,
-			   be32_to_cpu(tunnel->parms.i_key), RT_TOS(tos),
+			   be32_to_cpu(tunnel->parms.o_key), RT_TOS(tos),
 			   RT_SCOPE_UNIVERSE,
 			   IPPROTO_IPIP, 0,
 			   dst, tiph->saddr, 0, 0);

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Steffen Klassert]

On Sat, May 16, 2015 at 05:13:28PM -0400, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sat, 16 May 2015 20:34:58 +0800
> 
> > On Fri, May 15, 2015 at 12:14:43PM -0700, Alexander Duyck wrote:
> >>
> >> >Yeah, this mark handling via tunnel->parms.o_key looks not so good.
> >> 
> >> So is there any recommendations for an alternative to make it so
> >> that the ipsec endpoint is identified as needing to be encrypted or
> >> decrypted?  If needed I could probably take a day or two to try and
> >> address it as I still have a few other minor things I want to try
> >> and fix such as the MTU configuration for vti/vti6.
> > 
> > I'd like to hear from Steffen as to whether there is anything
> > in userspace that relies on the mark being used in this way by
> > vti.  If not it should be easy to get rid of it and use some
> > field that's not exposed to user-space.  If there is then this
> > would be tricky to resolve.

The policy and state mark must match the tunnels i_key/o_key
on lookup. It is not required to set skb->mark, but we have
to make sure to use the tunnels gre keys as the mark when we
do a policy or state lookup.

> 
> The mark stuff comes from the commit below, and at the time I remember
> pushing back a few times because I was uneasy about applying this
> change:
> 
> ====================
> commit 7263a5187f9e9de45fcb51349cf0e031142c19a1
> Author: Christophe Gouault <christophe.gouault@6wind.com>
> Date:   Tue Oct 8 17:21:22 2013 +0200
> 
>     vti: get rid of nf mark rule in prerouting

This patch added the mark to the skb, but the matching
of the gre keys and the mark was already in the initial
implementation of vti. It used the i_key (instead of
skb->mark) to do the output route lookup.

I never liked this gre key/mark lookup. This should have been
done with a separate lookup key, but I fear we can't change
it without breaking backwards compatibility.

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Mon, May 18, 2015 at 09:04:27AM +0200, Steffen Klassert wrote:
>
> The policy and state mark must match the tunnels i_key/o_key
> on lookup. It is not required to set skb->mark, but we have
> to make sure to use the tunnels gre keys as the mark when we
> do a policy or state lookup.

So is it possible to restore the original mark after the lookups?
At least that way it is still possible to use the mark before and
after the encapsulation.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Steffen Klassert]

On Mon, May 18, 2015 at 04:31:21PM +0800, Herbert Xu wrote:
> On Mon, May 18, 2015 at 09:04:27AM +0200, Steffen Klassert wrote:
> >
> > The policy and state mark must match the tunnels i_key/o_key
> > on lookup. It is not required to set skb->mark, but we have
> > to make sure to use the tunnels gre keys as the mark when we
> > do a policy or state lookup.
> 
> So is it possible to restore the original mark after the lookups?

Yes, that's possible. We just did not restore it because it 
was cleared with skb_scrub_packet() right after the lookup.

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Mon, May 18, 2015 at 10:38:44AM +0200, Steffen Klassert wrote:
> 
> Yes, that's possible. We just did not restore it because it 
> was cleared with skb_scrub_packet() right after the lookup.

OK I think we can still salvage this if we do the following:

1) Restore the mark after the lookup so the effect is minimised.
2) Create new flow lookup field for this.
3) Create tunnel flag that indicates new field should be used.
4) Deprecate the use of skb->mark by vti.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Steffen Klassert]

On Mon, May 18, 2015 at 04:59:08PM +0800, Herbert Xu wrote:
> On Mon, May 18, 2015 at 10:38:44AM +0200, Steffen Klassert wrote:
> > 
> > Yes, that's possible. We just did not restore it because it 
> > was cleared with skb_scrub_packet() right after the lookup.
> 
> OK I think we can still salvage this if we do the following:
> 
> 1) Restore the mark after the lookup so the effect is minimised.

Yes, we need this to fix it for now.

> 2) Create new flow lookup field for this.
> 3) Create tunnel flag that indicates new field should be used.

Or, maybe we can create new vti tunnel keys and some key
field in the xfrm_policy and xfrm_state. If the vti keys
are present at the tunnel, use the new flow lookup field
and match with the key field of the policy and state.
Otherwise, fallback to use gre keys and mark.

Re: [net PATCH] ip_vti/ip6_vti: Clear skb->mark when resetting skb->dev in receive path

[Herbert Xu]

On Mon, May 18, 2015 at 12:30:47PM +0200, Steffen Klassert wrote:
>
> Or, maybe we can create new vti tunnel keys and some key
> field in the xfrm_policy and xfrm_state. If the vti keys
> are present at the tunnel, use the new flow lookup field
> and match with the key field of the policy and state.
> Otherwise, fallback to use gre keys and mark.

Yes that could work.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt