ip_tunnel: Remove gratuitous skb scrubbing

ip_tunnel: Remove gratuitous skb scrubbing

[Herbert Xu]

The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
harmonize cleanup done on skb on rx path") broke anyone trying to
use netfilter marking across IPv4 tunnels.  As the commit message
did not give any justification for this (in fact it shouldn't
even be touching the tx path), I can only assume that it was a typo.

This patch reverts that change.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 88c386c..709e711 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -54,7 +54,8 @@ int iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 	struct iphdr *iph;
 	int err;
 
-	skb_scrub_packet(skb, xnet);
+	if (xnet)
+		skb_scrub_packet(skb, true);
 
 	skb_clear_hash(skb);
 	skb_dst_set(skb, &rt->dst);
-- 
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: ip_tunnel: Remove gratuitous skb scrubbing

[Herbert Xu]

On Wed, Apr 15, 2015 at 06:01:07PM +0800, Herbert Xu wrote:
> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> harmonize cleanup done on skb on rx path") broke anyone trying to
> use netfilter marking across IPv4 tunnels.  As the commit message
> did not give any justification for this (in fact it shouldn't
> even be touching the tx path), I can only assume that it was a typo.
> 
> This patch reverts that change.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Scratch this, I guess most of the scurbbing makes sense.  I'll
spin a patch to just preserves the mark.

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: ip_tunnel: Remove gratuitous skb scrubbing

[Nicolas Dichtel]

Le 15/04/2015 12:01, Herbert Xu a écrit :
> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> harmonize cleanup done on skb on rx path") broke anyone trying to
> use netfilter marking across IPv4 tunnels.  As the commit message
> did not give any justification for this (in fact it shouldn't
> even be touching the tx path), I can only assume that it was a typo.
If I remember well, this was discussed on netdev (CC Eric). The goal of this
patch was, like the title said, to hamonize packets processing in tunnels.
I'm not against to keep the mark, but I think patching skb_scrub_packet is
better. With your patch, ip6tnl, gre6, etc. still drops the mark. And at the
end, it's not consistant.

What about something like this:

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3b6e5830256e..1d5f6bd0e383 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4124,14 +4124,15 @@ EXPORT_SYMBOL(skb_try_coalesce);
   */
  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
  {
-	if (xnet)
+	if (xnet) {
  		skb_orphan(skb);
+		skb->mark = 0;
+	}
  	skb->tstamp.tv64 = 0;
  	skb->pkt_type = PACKET_HOST;
  	skb->skb_iif = 0;
  	skb->ignore_df = 0;
  	skb_dst_drop(skb);
-	skb->mark = 0;
  	skb_sender_cpu_clear(skb);
  	skb_init_secmark(skb);
  	secpath_reset(skb);

Re: ip_tunnel: Remove gratuitous skb scrubbing

[Herbert Xu]

On Wed, Apr 15, 2015 at 12:20:42PM +0200, Nicolas Dichtel wrote:
> Le 15/04/2015 12:01, Herbert Xu a écrit :
> >The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> >harmonize cleanup done on skb on rx path") broke anyone trying to
> >use netfilter marking across IPv4 tunnels.  As the commit message
> >did not give any justification for this (in fact it shouldn't
> >even be touching the tx path), I can only assume that it was a typo.
> If I remember well, this was discussed on netdev (CC Eric). The goal of this
> patch was, like the title said, to hamonize packets processing in tunnels.
> I'm not against to keep the mark, but I think patching skb_scrub_packet is
> better. With your patch, ip6tnl, gre6, etc. still drops the mark. And at the
> end, it's not consistant.
> 
> What about something like this:

Yes this is better.  I'm currently auditing all the other bits
that are cleared to see if there is anything else that we should
preserve for tunneling.

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: ip_tunnel: Remove gratuitous skb scrubbing

[Nicolas Dichtel]

Le 15/04/2015 12:22, Herbert Xu a écrit :
> On Wed, Apr 15, 2015 at 12:20:42PM +0200, Nicolas Dichtel wrote:
>> Le 15/04/2015 12:01, Herbert Xu a écrit :
>>> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
>>> harmonize cleanup done on skb on rx path") broke anyone trying to
>>> use netfilter marking across IPv4 tunnels.  As the commit message
>>> did not give any justification for this (in fact it shouldn't
>>> even be touching the tx path), I can only assume that it was a typo.
>> If I remember well, this was discussed on netdev (CC Eric). The goal of this
>> patch was, like the title said, to hamonize packets processing in tunnels.
>> I'm not against to keep the mark, but I think patching skb_scrub_packet is
>> better. With your patch, ip6tnl, gre6, etc. still drops the mark. And at the
>> end, it's not consistant.
>>
>> What about something like this:
>
> Yes this is better.  I'm currently auditing all the other bits
> that are cleared to see if there is anything else that we should
> preserve for tunneling.
Here is the thread about the mark:
http://thread.gmane.org/gmane.linux.network/246876/focus=274528

Re: ip_tunnel: Remove gratuitous skb scrubbing

[Herbert Xu]

On Wed, Apr 15, 2015 at 12:28:46PM +0200, Nicolas Dichtel wrote:
>
> Here is the thread about the mark:
> http://thread.gmane.org/gmane.linux.network/246876/focus=274528

Thanks but I don't see any justification for breaking the mark
feature.

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: ip_tunnel: Remove gratuitous skb scrubbing

[Herbert Xu]

On Wed, Apr 15, 2015 at 06:22:29PM +0800, Herbert Xu wrote:
>
> Yes this is better.  I'm currently auditing all the other bits
> that are cleared to see if there is anything else that we should
> preserve for tunneling.

OK the only other thing that we may wish to preserve is secmark.
James, can you confirm whether secmark should be preserved or
cleared for tunnels within the same name space? Up until December
2014 it was preserved but since then it has been cleared.

For the mark here is my final tested patch.

---8<---
Subject: skbuff: Do not scrub skb mark within the same name space

The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
harmonize cleanup done on skb on rx path") broke anyone trying to
use netfilter marking across IPv4 tunnels.  While most of the
fields that are cleared by skb_scrub_packet don't matter, the
netfilter mark must be preserved.

This patch rearranges skb_scurb_packet to preserve the mark field.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3b6e583..a185427 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4124,19 +4124,22 @@ EXPORT_SYMBOL(skb_try_coalesce);
  */
 void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 {
-	if (xnet)
-		skb_orphan(skb);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->skb_iif = 0;
 	skb->ignore_df = 0;
 	skb_dst_drop(skb);
-	skb->mark = 0;
 	skb_sender_cpu_clear(skb);
 	skb_init_secmark(skb);
 	secpath_reset(skb);
 	nf_reset(skb);
 	nf_reset_trace(skb);
+
+	if (!xnet)
+		return;
+
+	skb_orphan(skb);
+	skb->mark = 0;
 }
 EXPORT_SYMBOL_GPL(skb_scrub_packet);
 
-- 
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: ip_tunnel: Remove gratuitous skb scrubbing

[Nicolas Dichtel]

Le 15/04/2015 15:57, Herbert Xu a écrit :
> On Wed, Apr 15, 2015 at 06:22:29PM +0800, Herbert Xu wrote:
[snip]
> Subject: skbuff: Do not scrub skb mark within the same name space
>
> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
Maybe add a Fixes tag?
Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")

> harmonize cleanup done on skb on rx path") broke anyone trying to
> use netfilter marking across IPv4 tunnels.  While most of the
> fields that are cleared by skb_scrub_packet don't matter, the
> netfilter mark must be preserved.
>
> This patch rearranges skb_scurb_packet to preserve the mark field.
nit: s/scurb/scrub

Else it's fine for me.

[v3] skbuff: Do not scrub skb mark within the same name space

[Herbert Xu]

On Wed, Apr 15, 2015 at 05:41:26PM +0200, Nicolas Dichtel wrote:
> Le 15/04/2015 15:57, Herbert Xu a écrit :
> >On Wed, Apr 15, 2015 at 06:22:29PM +0800, Herbert Xu wrote:
> [snip]
> >Subject: skbuff: Do not scrub skb mark within the same name space
> >
> >The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> Maybe add a Fixes tag?
> Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")
> 
> >harmonize cleanup done on skb on rx path") broke anyone trying to
> >use netfilter marking across IPv4 tunnels.  While most of the
> >fields that are cleared by skb_scrub_packet don't matter, the
> >netfilter mark must be preserved.
> >
> >This patch rearranges skb_scurb_packet to preserve the mark field.
> nit: s/scurb/scrub
> 
> Else it's fine for me.

Sure.

PS I used the wrong email for James the first time around.  So
let me repeat the question here.  Should secmark be preserved
or cleared across tunnels within the same name space? In fact,
do our security models even support name spaces?

---8<---
The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
harmonize cleanup done on skb on rx path") broke anyone trying to
use netfilter marking across IPv4 tunnels.  While most of the
fields that are cleared by skb_scrub_packet don't matter, the
netfilter mark must be preserved.

This patch rearranges skb_scrub_packet to preserve the mark field.

Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3b6e583..a185427 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4124,19 +4124,22 @@ EXPORT_SYMBOL(skb_try_coalesce);
  */
 void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 {
-	if (xnet)
-		skb_orphan(skb);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->skb_iif = 0;
 	skb->ignore_df = 0;
 	skb_dst_drop(skb);
-	skb->mark = 0;
 	skb_sender_cpu_clear(skb);
 	skb_init_secmark(skb);
 	secpath_reset(skb);
 	nf_reset(skb);
 	nf_reset_trace(skb);
+
+	if (!xnet)
+		return;
+
+	skb_orphan(skb);
+	skb->mark = 0;
 }
 EXPORT_SYMBOL_GPL(skb_scrub_packet);
 
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3] skbuff: Do not scrub skb mark within the same name space

[James Morris]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1304 bytes --]

On Thu, 16 Apr 2015, Herbert Xu wrote:

> On Wed, Apr 15, 2015 at 05:41:26PM +0200, Nicolas Dichtel wrote:
> > Le 15/04/2015 15:57, Herbert Xu a écrit :
> > >On Wed, Apr 15, 2015 at 06:22:29PM +0800, Herbert Xu wrote:
> > [snip]
> > >Subject: skbuff: Do not scrub skb mark within the same name space
> > >
> > >The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> > Maybe add a Fixes tag?
> > Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")
> > 
> > >harmonize cleanup done on skb on rx path") broke anyone trying to
> > >use netfilter marking across IPv4 tunnels.  While most of the
> > >fields that are cleared by skb_scrub_packet don't matter, the
> > >netfilter mark must be preserved.
> > >
> > >This patch rearranges skb_scurb_packet to preserve the mark field.
> > nit: s/scurb/scrub
> > 
> > Else it's fine for me.
> 
> Sure.
> 
> PS I used the wrong email for James the first time around.  So
> let me repeat the question here.  Should secmark be preserved
> or cleared across tunnels within the same name space? In fact,
> do our security models even support name spaces?

They don't support namespaces, and maintaining the label is critical for 
SELinux, at least, which mediates security for the system as a whole.


-- 
James Morris
<jmorris@namei.org>

Re: [v3] skbuff: Do not scrub skb mark within the same name space

[Nicolas Dichtel]

Le 16/04/2015 09:02, James Morris a écrit :
> On Thu, 16 Apr 2015, Herbert Xu wrote:
[snip]
>> PS I used the wrong email for James the first time around.  So
>> let me repeat the question here.  Should secmark be preserved
>> or cleared across tunnels within the same name space? In fact,
>> do our security models even support name spaces?
>
> They don't support namespaces, and maintaining the label is critical for
> SELinux, at least, which mediates security for the system as a whole.
Herbert, could you send a v4 of your patch with the secmark included?

Re: [v3] skbuff: Do not scrub skb mark within the same name space

[Herbert Xu]

On Thu, Apr 16, 2015 at 09:35:31AM +0200, Nicolas Dichtel wrote:
> Herbert, could you send a v4 of your patch with the secmark included?

Actually this should go into a separate patch because it's a
straight revert and also that unlike the mark this should never
be cleared, even when you cross a namespace boundary.

But I will send a patch.

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

Revert "net: Reset secmark when scrubbing packet"

[Herbert Xu]

On Thu, Apr 16, 2015 at 05:02:15PM +1000, James Morris wrote:
> 
> They don't support namespaces, and maintaining the label is critical for 
> SELinux, at least, which mediates security for the system as a whole.

Thanks for the confirmation James, I thought this looked a bit
dodgy :)

---8<---
This patch reverts commit b8fb4e0648a2ab3734140342002f68fb0c7d1602
because the secmark must be preserved even when a packet crosses
namespace boundaries.  The reason is that security labels apply to
the system as a whole and is not per-namespace.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a185427..d1967da 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4130,7 +4130,6 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 	skb->ignore_df = 0;
 	skb_dst_drop(skb);
 	skb_sender_cpu_clear(skb);
-	skb_init_secmark(skb);
 	secpath_reset(skb);
 	nf_reset(skb);
 	nf_reset_trace(skb);
-- 
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: Revert "net: Reset secmark when scrubbing packet"

[Thomas Graf]

On 04/16/15 at 04:12pm, Herbert Xu wrote:
> On Thu, Apr 16, 2015 at 05:02:15PM +1000, James Morris wrote:
> > 
> > They don't support namespaces, and maintaining the label is critical for 
> > SELinux, at least, which mediates security for the system as a whole.
> 
> Thanks for the confirmation James, I thought this looked a bit
> dodgy :)
> 
> ---8<---
> This patch reverts commit b8fb4e0648a2ab3734140342002f68fb0c7d1602
> because the secmark must be preserved even when a packet crosses
> namespace boundaries.  The reason is that security labels apply to
> the system as a whole and is not per-namespace.

No objection to reverting, _BUT_ just because security labels
apply to the system as a whole does not mean that both the packet
in the underlay and overlay belong to the same context.

The point here was to not blindly inherit the security context of a
packet based on the outer or inner header. Someone tagging all
packets addressed to the host itself with a SElinux context may not
expect that SELinux context to be preserved into a namespaced tenant.

Re: [v3] skbuff: Do not scrub skb mark within the same name space

[Thomas Graf]

On 04/16/15 at 09:03am, Herbert Xu wrote:
> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
> harmonize cleanup done on skb on rx path") broke anyone trying to
> use netfilter marking across IPv4 tunnels.  While most of the
> fields that are cleared by skb_scrub_packet don't matter, the
> netfilter mark must be preserved.
> 
> This patch rearranges skb_scrub_packet to preserve the mark field.
> 
> Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Thomas Graf <tgraf@suug.ch>

We should also add a flag to veth which expclitly allows to preserve
the mark into the namespace.

Re: [v3] skbuff: Do not scrub skb mark within the same name space

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 16 Apr 2015 09:33:35 +0100

> On 04/16/15 at 09:03am, Herbert Xu wrote:
>> The commit ea23192e8e577dfc51e0f4fc5ca113af334edff9 ("tunnels:
>> harmonize cleanup done on skb on rx path") broke anyone trying to
>> use netfilter marking across IPv4 tunnels.  While most of the
>> fields that are cleared by skb_scrub_packet don't matter, the
>> netfilter mark must be preserved.
>> 
>> This patch rearranges skb_scrub_packet to preserve the mark field.
>> 
>> Fixes: ea23192e8e57 ("tunnels: harmonize cleanup done on skb on rx path")
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: Thomas Graf <tgraf@suug.ch>
> 
> We should also add a flag to veth which expclitly allows to preserve
> the mark into the namespace.

Applied and queued up for -stable, thanks.

Re: Revert "net: Reset secmark when scrubbing packet"

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 16 Apr 2015 16:12:53 +0800

> On Thu, Apr 16, 2015 at 05:02:15PM +1000, James Morris wrote:
>> 
>> They don't support namespaces, and maintaining the label is critical for 
>> SELinux, at least, which mediates security for the system as a whole.
> 
> Thanks for the confirmation James, I thought this looked a bit
> dodgy :)
> 
> ---8<---
> This patch reverts commit b8fb4e0648a2ab3734140342002f68fb0c7d1602
> because the secmark must be preserved even when a packet crosses
> namespace boundaries.  The reason is that security labels apply to
> the system as a whole and is not per-namespace.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied and queued up for -stable.