diff mbox

ip_tunnel: Remove gratuitous skb scrubbing

Message ID 552E3B7A.2040701@6wind.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Nicolas Dichtel April 15, 2015, 10:20 a.m. UTC
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:

--
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

Herbert Xu April 15, 2015, 10:22 a.m. UTC | #1
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,
Nicolas Dichtel April 15, 2015, 10:28 a.m. UTC | #2
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
--
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
Herbert Xu April 15, 2015, 10:32 a.m. UTC | #3
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,
diff mbox

Patch

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);