From: ebiederm@xmission•com (Eric W. Biederman)
To: David Miller <davem@davemloft•net>
Cc: nicolas.dichtel@6wind•com, netdev@vger•kernel.org,
bcrl@kvack•org, ravi.mlists@gmail•com, bhutchings@solarflare•com
Subject: Re: [PATCH v2 net-next 2/2] sit: add support of x-netns
Date: Wed, 26 Jun 2013 03:03:22 -0700 [thread overview]
Message-ID: <87ppv942ad.fsf@xmission.com> (raw)
In-Reply-To: <20130625.224810.293452557691677283.davem@davemloft.net> (David Miller's message of "Tue, 25 Jun 2013 22:48:10 -0700 (PDT)")
David Miller <davem@davemloft•net> writes:
> From: ebiederm@xmission•com (Eric W. Biederman)
> Date: Tue, 25 Jun 2013 18:35:30 -0700
>
>> David Miller <davem@davemloft•net> writes:
>>
>>> From: Nicolas Dichtel <nicolas.dichtel@6wind•com>
>>> Date: Tue, 25 Jun 2013 16:24:55 +0200
>>>
>>>> @@ -453,6 +454,8 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>>>> tstats->rx_bytes += skb->len;
>>>> u64_stats_update_end(&tstats->syncp);
>>>>
>>>> + skb_scrub_packet(skb);
>>>> +
>>>> if (tunnel->dev->type == ARPHRD_ETHER) {
>>>> skb->protocol = eth_type_trans(skb, tunnel->dev);
>>>> skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>>>
>>> I can't see how this can be ok.
>>>
>>> If something in netfilter depends upon the state you are clearing out
>>> here, someone's packet filtering setup is going to break.
>>>
>>> I'm not applying these patches, sorry.
>>
>> How can netfilter depend on the state of a packet inside of a tunnel?
>>
>> How can it even make sense?
>>
>> Or is your concern that we unintentionally allowed this in the past so
>> to avoid breaking binary compatibility we should continue in case
>> someone somewhere cares?
>>
>> I really can't see how this could possibly be an intentional feature.
>
> You can make all of these issues go away by only clearing the SKB
> meta state when namespaces are actually changing as we go through
> the tunnel.
I have spent some time thinking about the cases where I have had an
opportunity to use the marks on packets and it turns out that if I had
been using a tunnel with any of those configurations leaving the marks
on would have either broken my configuration or at the very least have
required me to make certain I changed those marks.
So I really think this is a bug fix, for a long standing bug in a rare
corner case of kernel behavior that people just haven't noticed. Which
is why I suggested to Nicolas Ditchtel that he remove the test to see if
we were changing network namespaces before scrubbing the packet.
That said I won't object if Nocolas Ditchel resends his patches with
that test put back in. I just think it is silly and when someone
finally gets bit by the bug and complains we will have to go through and
remove the test.
Eric
next prev parent reply other threads:[~2013-06-26 10:03 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-23 17:49 switching network namespace midway rsa
2012-10-24 21:11 ` Eric W. Biederman
2012-10-24 21:21 ` Benjamin LaHaise
2012-10-25 1:37 ` Eric W. Biederman
2012-10-25 14:38 ` Benjamin LaHaise
2012-10-25 16:21 ` Stephen Hemminger
2012-10-28 5:43 ` Eric W. Biederman
2012-10-29 14:23 ` Stephen Hemminger
2012-10-30 0:21 ` Eric W. Biederman
2012-10-30 8:55 ` James Chapman
2012-10-25 15:12 ` rsa
2012-10-25 15:29 ` rsa
2012-10-25 15:59 ` Benjamin LaHaise
2012-10-25 16:15 ` Eric W. Biederman
2012-11-02 2:25 ` Benjamin LaHaise
2012-11-02 6:18 ` Eric W. Biederman
2012-11-02 14:03 ` Benjamin LaHaise
2012-11-02 20:45 ` Eric W. Biederman
2013-06-24 14:13 ` [RFC PATCH net-next 0/2] sit: allow to switch netns during encap/decap Nicolas Dichtel
2013-06-24 14:13 ` [RFC PATCH net-next 1/2] dev: introduce dev_cleanup_skb() Nicolas Dichtel
2013-06-24 18:13 ` Ben Hutchings
2013-06-24 19:05 ` Eric W. Biederman
2013-06-24 14:13 ` [RFC PATCH net-next 2/2] sit: add support of x-netns Nicolas Dichtel
2013-06-24 19:28 ` Eric W. Biederman
2013-06-24 21:11 ` Nicolas Dichtel
2013-06-24 22:42 ` Eric W. Biederman
2013-06-25 14:10 ` Nicolas Dichtel
2013-06-25 14:24 ` [PATCH v2 net-next 0/2] sit: allow to switch netns during encap/decap Nicolas Dichtel
2013-06-25 14:24 ` [PATCH v2 net-next 1/2] dev: introduce skb_scrub_packet() Nicolas Dichtel
2013-06-25 14:24 ` [PATCH v2 net-next 2/2] sit: add support of x-netns Nicolas Dichtel
2013-06-25 23:56 ` David Miller
2013-06-26 1:35 ` Eric W. Biederman
2013-06-26 5:48 ` David Miller
2013-06-26 10:03 ` Eric W. Biederman [this message]
2013-06-26 10:22 ` Eric Dumazet
2013-06-26 12:15 ` Nicolas Dichtel
2013-06-26 14:11 ` [PATCH v3 net-next 0/2] sit: allow to switch netns during encap/decap Nicolas Dichtel
2013-06-26 14:11 ` [PATCH v3 net-next 1/2] dev: introduce skb_scrub_packet() Nicolas Dichtel
2013-06-26 14:11 ` [PATCH v3 net-next 2/2] sit: add support of x-netns Nicolas Dichtel
2013-06-28 5:36 ` [PATCH v3 net-next 0/2] sit: allow to switch netns during encap/decap David Miller
2013-07-03 15:00 ` [PATCH net-next 0/3] ipip/ip6tnl: " Nicolas Dichtel
2013-07-03 15:00 ` [PATCH net-next 1/3] sit: fix tunnel update via netlink Nicolas Dichtel
2013-07-03 15:00 ` [PATCH net-next 2/3] ipip: add x-netns support Nicolas Dichtel
2013-07-03 15:00 ` [PATCH net-next 3/3] ip6tnl: " Nicolas Dichtel
2013-07-04 21:56 ` [PATCH net-next 0/3] ipip/ip6tnl: allow to switch netns during encap/decap David Miller
2013-08-13 15:51 ` [PATCH net-next v2 0/4] " Nicolas Dichtel
2013-08-13 15:51 ` [PATCH net-next v2 1/4] dev: move skb_scrub_packet() after eth_type_trans() Nicolas Dichtel
2013-08-13 15:51 ` [PATCH net-next v2 2/4] ipv4 tunnels: use net_eq() helper to check netns Nicolas Dichtel
2013-08-13 15:51 ` [PATCH net-next v2 3/4] ipip: add x-netns support Nicolas Dichtel
2013-08-13 15:51 ` [PATCH net-next v2 4/4] ip6tnl: " Nicolas Dichtel
2013-08-15 8:01 ` [PATCH net-next v2 0/4] ipip/ip6tnl: allow to switch netns during encap/decap David Miller
2013-06-26 13:49 ` [PATCH v2 net-next 2/2] sit: add support of x-netns Nicolas Dichtel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ppv942ad.fsf@xmission.com \
--to=ebiederm@xmission$(echo .)com \
--cc=bcrl@kvack$(echo .)org \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.dichtel@6wind$(echo .)com \
--cc=ravi.mlists@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox