[PATCH -next 0/2] net: allow setting ecn via routing table

[PATCH -next 0/2] net: allow setting ecn via routing table

[Florian Westphal]

These two patches allow turing on explicit congestion notification
based on the destination network.

For example, assuming the default tcp_ecn sysctl '2', the following will
enable ecn (tcp_ecn=1 behaviour, i.e. request ecn to be enabled for a
tcp connection) for all connections to hosts inside the 192.168.2/24 network:

ip route change 192.168.2.0/24 dev eth0 features ecn

Having a more fine-grained per-route setting can be beneficial for
various reasons, for example 1) within data centers, or 2) local ISPs
may deploy ECN support for their own video/streaming services [1], etc.

Joint work with Daniel Borkmann, feature suggested by Hannes Frederic Sowa.

The patch to enable this in iproute2 will be posted shortly, it is currently
also available here:
http://git.breakpoint.cc/cgit/fw/iproute2.git/commit/?h=iproute_features&id=8843d2d8973fb81c78a7efe6d42e3a17d739003e

[1] http://www.ietf.org/proceedings/89/slides/slides-89-tsvarea-1.pdf, p.15

[PATCH -next 1/2] syncookies: remove ecn_ok validation when decoding option timestamp

[Florian Westphal]

Won't work anymore when tcp_ecn=0 and RTAX_FEATURES route attribute did
allow ecn.

While its possible to extend the test to also perform route lookup and
check RTAX_FEATURES, it doesn't seem worth it.

Thus, just turn on ecn if the client ts indicates so.
This means that while syn cookies are in use clients can turn on ecn
even if it is off.  However, there seems to be no harm in permitting
this.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/tcp.h     | 3 +--
 net/ipv4/syncookies.c | 6 ++----
 net/ipv6/syncookies.c | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index c73fc14..7c85167 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -495,8 +495,7 @@ __u32 cookie_v4_init_sequence(struct sock *sk, const struct sk_buff *skb,
 #endif
 
 __u32 cookie_init_timestamp(struct request_sock *req);
-bool cookie_check_timestamp(struct tcp_options_received *opt, struct net *net,
-			    bool *ecn_ok);
+bool cookie_check_timestamp(struct tcp_options_received *opt, bool *ecn_ok);
 
 /* From net/ipv6/syncookies.c */
 int __cookie_v6_check(const struct ipv6hdr *iph, const struct tcphdr *th,
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 32b98d0..b84cc12 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -225,7 +225,7 @@ static inline struct sock *get_cookie_sock(struct sock *sk, struct sk_buff *skb,
  * return false if we decode an option that should not be.
  */
 bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
-			struct net *net, bool *ecn_ok)
+			    bool *ecn_ok)
 {
 	/* echoed timestamp, lowest bits contain options */
 	u32 options = tcp_opt->rcv_tsecr & TSMASK;
@@ -240,8 +240,6 @@ bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
 
 	tcp_opt->sack_ok = (options & (1 << 4)) ? TCP_SACK_SEEN : 0;
 	*ecn_ok = (options >> 5) & 1;
-	if (*ecn_ok && !net->ipv4.sysctl_tcp_ecn)
-		return false;
 
 	if (tcp_opt->sack_ok && !sysctl_tcp_sack)
 		return false;
@@ -287,7 +285,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	memset(&tcp_opt, 0, sizeof(tcp_opt));
 	tcp_parse_options(skb, &tcp_opt, 0, NULL);
 
-	if (!cookie_check_timestamp(&tcp_opt, sock_net(sk), &ecn_ok))
+	if (!cookie_check_timestamp(&tcp_opt, &ecn_ok))
 		goto out;
 
 	ret = NULL;
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 0e26e79..4df0258 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -183,7 +183,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	memset(&tcp_opt, 0, sizeof(tcp_opt));
 	tcp_parse_options(skb, &tcp_opt, 0, NULL);
 
-	if (!cookie_check_timestamp(&tcp_opt, sock_net(sk), &ecn_ok))
+	if (!cookie_check_timestamp(&tcp_opt, &ecn_ok))
 		goto out;
 
 	ret = NULL;
-- 
2.0.4

[PATCH -next 2/2] net: allow setting ecn via routing table

[Florian Westphal]

Allows to set ECN on a per-route basis in case the sysctl tcp_ecn is
not 1.  IOW, when ECN is set for specific routes, it provides a
tcp_ecn=1 behaviour for that route while the rest of the stack acts
according to the global settings.

One can use 'ip route change dev $dev $net features ecn' to toggle this.

Joint work with Daniel Borkmann.

Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/ipv4/tcp_input.c  | 25 +++++++++++++++----------
 net/ipv4/tcp_output.c | 12 ++++++++++--
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a12b455..72d7510 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5875,20 +5875,22 @@ static inline void pr_drop_req(struct request_sock *req, __u16 port, int family)
  */
 static void tcp_ecn_create_request(struct request_sock *req,
 				   const struct sk_buff *skb,
-				   const struct sock *listen_sk)
+				   const struct sock *listen_sk,
+				   struct dst_entry *dst)
 {
 	const struct tcphdr *th = tcp_hdr(skb);
 	const struct net *net = sock_net(listen_sk);
 	bool th_ecn = th->ece && th->cwr;
-	bool ect, need_ecn;
+	bool ect, need_ecn, ecn_ok;
 
 	if (!th_ecn)
 		return;
 
 	ect = !INET_ECN_is_not_ect(TCP_SKB_CB(skb)->ip_dsfield);
 	need_ecn = tcp_ca_needs_ecn(listen_sk);
+	ecn_ok = net->ipv4.sysctl_tcp_ecn || dst_feature(dst, RTAX_FEATURE_ECN);
 
-	if (!ect && !need_ecn && net->ipv4.sysctl_tcp_ecn)
+	if (!ect && !need_ecn && ecn_ok)
 		inet_rsk(req)->ecn_ok = 1;
 	else if (ect && need_ecn)
 		inet_rsk(req)->ecn_ok = 1;
@@ -5953,13 +5955,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	if (!want_cookie || tmp_opt.tstamp_ok)
-		tcp_ecn_create_request(req, skb, sk);
-
-	if (want_cookie) {
-		isn = cookie_init_sequence(af_ops, sk, skb, &req->mss);
-		req->cookie_ts = tmp_opt.tstamp_ok;
-	} else if (!isn) {
+	if (!want_cookie && !isn) {
 		/* VJ's idea. We save last timestamp seen
 		 * from the destination in peer table, when entering
 		 * state TIME-WAIT, and check against it before
@@ -6007,6 +6003,15 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 			goto drop_and_free;
 	}
 
+	tcp_ecn_create_request(req, skb, sk, dst);
+
+	if (want_cookie) {
+		isn = cookie_init_sequence(af_ops, sk, skb, &req->mss);
+		req->cookie_ts = tmp_opt.tstamp_ok;
+		if (!tmp_opt.tstamp_ok)
+			inet_rsk(req)->ecn_ok = 0;
+	}
+
 	tcp_rsk(req)->snt_isn = isn;
 	tcp_openreq_init_rwin(req, sk, dst);
 	fastopen = !want_cookie &&
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 3af2129..b1c6296 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -333,10 +333,18 @@ static void tcp_ecn_send_synack(struct sock *sk, struct sk_buff *skb)
 static void tcp_ecn_send_syn(struct sock *sk, struct sk_buff *skb)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	bool use_ecn = sock_net(sk)->ipv4.sysctl_tcp_ecn == 1 ||
+		       tcp_ca_needs_ecn(sk);
+
+	if (!use_ecn) {
+		const struct dst_entry *dst = __sk_dst_get(sk);
+		if (dst && dst_feature(dst, RTAX_FEATURE_ECN))
+			use_ecn = true;
+	}
 
 	tp->ecn_flags = 0;
-	if (sock_net(sk)->ipv4.sysctl_tcp_ecn == 1 ||
-	    tcp_ca_needs_ecn(sk)) {
+
+	if (use_ecn) {
 		TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_ECE | TCPHDR_CWR;
 		tp->ecn_flags = TCP_ECN_OK;
 		if (tcp_ca_needs_ecn(sk))
-- 
2.0.4

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Sun, 26 Oct 2014 00:38:47 +0200

> These two patches allow turing on explicit congestion notification
> based on the destination network.
> 
> For example, assuming the default tcp_ecn sysctl '2', the following will
> enable ecn (tcp_ecn=1 behaviour, i.e. request ecn to be enabled for a
> tcp connection) for all connections to hosts inside the 192.168.2/24 network:
> 
> ip route change 192.168.2.0/24 dev eth0 features ecn
> 
> Having a more fine-grained per-route setting can be beneficial for
> various reasons, for example 1) within data centers, or 2) local ISPs
> may deploy ECN support for their own video/streaming services [1], etc.
> 
> Joint work with Daniel Borkmann, feature suggested by Hannes Frederic Sowa.
> 
> The patch to enable this in iproute2 will be posted shortly, it is currently
> also available here:
> http://git.breakpoint.cc/cgit/fw/iproute2.git/commit/?h=iproute_features&id=8843d2d8973fb81c78a7efe6d42e3a17d739003e
> 
> [1] http://www.ietf.org/proceedings/89/slides/slides-89-tsvarea-1.pdf, p.15

I don't like how the route metric gives less control than the sysctl.

If the tcp_ecn cases of '1' and '2' make sense for the sysctl, I do not
see why they wouldn't make sense for the per-route knob to.

Implement the following policy, if per-route metric is non-zero use it
instead of the sysctl setting.

Then you have a helper:

static int tcp_ecn_enabled(struct net *net, struct dst_entry *dst)
{
	u32 val = dst_metric(dst, RTAX_ECN);

	if (val)
		return val;
	return net->ipv4.sysctl_tcp_ecn;
}

Then there is no other change to make other than an absolute
strict substitution of sysctl_tcp_ecn with tcp_ecn_enabled().

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Florian Westphal]

David Miller <davem@davemloft.net> wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Sun, 26 Oct 2014 00:38:47 +0200
> 
> > These two patches allow turing on explicit congestion notification
> > based on the destination network.
> > 
> > For example, assuming the default tcp_ecn sysctl '2', the following will
> > enable ecn (tcp_ecn=1 behaviour, i.e. request ecn to be enabled for a
> > tcp connection) for all connections to hosts inside the 192.168.2/24 network:
> > 
> > ip route change 192.168.2.0/24 dev eth0 features ecn
> > 
> > Having a more fine-grained per-route setting can be beneficial for
> > various reasons, for example 1) within data centers, or 2) local ISPs
> > may deploy ECN support for their own video/streaming services [1], etc.
> > 
> > Joint work with Daniel Borkmann, feature suggested by Hannes Frederic Sowa.
> > 
> > The patch to enable this in iproute2 will be posted shortly, it is currently
> > also available here:
> > http://git.breakpoint.cc/cgit/fw/iproute2.git/commit/?h=iproute_features&id=8843d2d8973fb81c78a7efe6d42e3a17d739003e
> > 
> > [1] http://www.ietf.org/proceedings/89/slides/slides-89-tsvarea-1.pdf, p.15
> 
> I don't like how the route metric gives less control than the sysctl.
> 
> If the tcp_ecn cases of '1' and '2' make sense for the sysctl, I do not
> see why they wouldn't make sense for the per-route knob to.
>
> Implement the following policy, if per-route metric is non-zero use it
> instead of the sysctl setting.

I think that if we add a u32 route attr for a new ecn switch we might as well
support full override of the sysctl.

I had a discussion with Daniel Borkmann, and we came up with this
proposal:

- add RTAX_ECN
- if RTAX_ECN attribute present, set RTAX_FEATURE_ECN in RTAX_FEATURES
- in kernel, when RTAX_FEATURE_ECN set, use dst_metric(dst, RTAX_ECN).
- else, if RTAX_FEATURE_ECN unset, use the sysctl as fallback.

It would allow things like sysctl_tcp_ecn=1 and
ip route change some_blackhole dev eth0 ecn 0
ip route change some_network dev eth0 ecn 2
ip route change other dev eth0 ecn 1

We could do that, if you prefer.

I tried to come up with a scenario though, where sysctl_tcp_ecn=0, and
then we want to enable 'passive' ecn for incoming connections only on
a particular route without announcing ecn to the peer. I haven't been
able to find any -- I think if you deem 'route to x' safe for ecn it
might as well be enabled for both initiator and responder.  The original
patch would be sufficient for that.

IOW, is 'ecn from a to b but not b to a' a sensible requirement?

sysctl_tcp_ecn=2 seems just to be a convenience solution/intermediate
step to make the stack ecn-aware by default without too much breakage
risk for users (i.e. instead of having sysctl_tcp_ecn=1 as default).

Unrelated to this patch, but I'd like to see sysctl_tcp_ecn=1 as a
default at one point (almost no routers set CE bit at this time, perhaps
that would change if ecn usage is more widespread).

Thanks!

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Wed, 29 Oct 2014 13:23:07 +0100

> We could do that, if you prefer.
> 
> I tried to come up with a scenario though, where sysctl_tcp_ecn=0, and
> then we want to enable 'passive' ecn for incoming connections only on
> a particular route without announcing ecn to the peer. I haven't been
> able to find any -- I think if you deem 'route to x' safe for ecn it
> might as well be enabled for both initiator and responder.  The original
> patch would be sufficient for that.
> 
> IOW, is 'ecn from a to b but not b to a' a sensible requirement?

I think you have to apply the same logic for the sysctl (there's a
reason to only support ECN passively) as you do for the route feature
because you can logically look at the sysctl as applying to the
default route.

> Unrelated to this patch, but I'd like to see sysctl_tcp_ecn=1 as a
> default at one point (almost no routers set CE bit at this time, perhaps
> that would change if ecn usage is more widespread).

Now you're talking.

So, either passive ECN support makes sense or it does not.  To me, no
matter what the argument, it doesn't matter what realm (whole system,
specific routes) you apply that argument to.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Florian Westphal]

David Miller <davem@davemloft.net> wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Wed, 29 Oct 2014 13:23:07 +0100
> 
> > We could do that, if you prefer.
> > 
> > I tried to come up with a scenario though, where sysctl_tcp_ecn=0, and
> > then we want to enable 'passive' ecn for incoming connections only on
> > a particular route without announcing ecn to the peer. I haven't been
> > able to find any -- I think if you deem 'route to x' safe for ecn it
> > might as well be enabled for both initiator and responder.  The original
> > patch would be sufficient for that.
> > 
> > IOW, is 'ecn from a to b but not b to a' a sensible requirement?
> 
> I think you have to apply the same logic for the sysctl (there's a
> reason to only support ECN passively) as you do for the route feature
> because you can logically look at the sysctl as applying to the
> default route.

Agreed, sysctl is comparable to default route.
And I think 'passive ecn' makes perfect sense for a default route.

But for a specific host/network?

Either I know that path to $x is ecn-safe (i.e. turn it on for both ends)
or I don't, in which case the global 'passive' default ("if peer requests
it they probably know what they're doing") is fine.

> > default at one point (almost no routers set CE bit at this time, perhaps
> > that would change if ecn usage is more widespread).
> 
> Now you're talking.
> 
> So, either passive ECN support makes sense or it does not.  To me, no
> matter what the argument, it doesn't matter what realm (whole system,
> specific routes) you apply that argument to.

The passive mode was added 5 years ago via

commit 255cac91c3c9ce7dca7713b93ab03c75b7902e0e
(tcp: extend ECN sysctl to allow server-side only ECN), and I think the
commit log rationale makes sense.

So, what about changing the default to 1 in net-next?

We could add automatic 'no-ecn' to retransmitted syns to avoid
ecn blackholes (Daniel Borkmann has a patch for this), and, in case
ecn=1 causes too much breakage we can always revert (and re-consider ecn
per route settings as an intermediate step).

What do you think?

Thanks,
Florian

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Eric Dumazet]

On Thu, 2014-10-30 at 21:52 +0100, Florian Westphal wrote:

> So, what about changing the default to 1 in net-next?
> 
> We could add automatic 'no-ecn' to retransmitted syns to avoid
> ecn blackholes (Daniel Borkmann has a patch for this), and, in case
> ecn=1 causes too much breakage we can always revert (and re-consider ecn
> per route settings as an intermediate step).
> 
> What do you think?

I think this is way too dangerous.

I played a lot with ECN in the past (and fixed number of bugs in linux)
and discovered many times I had to disable it to be able to surf the
Internet.

I am kind of an expert so always could figure out what was the problem,
but average user wont have the choice.

Reverting might take a long long time, it wont help people stuck behind
buggy equipment.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-10-30 at 21:52 +0100, Florian Westphal wrote:
> 
> > So, what about changing the default to 1 in net-next?
> > 
> > We could add automatic 'no-ecn' to retransmitted syns to avoid
> > ecn blackholes (Daniel Borkmann has a patch for this), and, in case
> > ecn=1 causes too much breakage we can always revert (and re-consider ecn
> > per route settings as an intermediate step).
> > 
> > What do you think?
> 
> I think this is way too dangerous.
> 
> I played a lot with ECN in the past (and fixed number of bugs in linux)
> and discovered many times I had to disable it to be able to surf the
> Internet.
[..]
> Reverting might take a long long time, it wont help people stuck behind
> buggy equipment.

Do you think a fallback to non-ecn for retransmitted syns would help?
If not, do you think having ecn tunable available via route is helpful?

Thanks!

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Eric Dumazet]

On Thu, 2014-10-30 at 23:15 +0100, Florian Westphal wrote:

> Do you think a fallback to non-ecn for retransmitted syns would help?
> If not, do you think having ecn tunable available via route is helpful?

Unfortunately some firewalls are buggy and accept a single SYN per flow.

You would need to blacklist ecn at first sign of a possible ecn problem,
for all following connections attempts.

Note that ECN problems are not only contained in SYN packets being
eventually dropped. You might have a success to establish a flow, and
later get CE marks for all packets and cwnd converges to 1.

This is really a lot of work to get all sorted out.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-10-30 at 23:15 +0100, Florian Westphal wrote:
> 
> > Do you think a fallback to non-ecn for retransmitted syns would help?
> 
> Unfortunately some firewalls are buggy and accept a single SYN per flow.
[..]
> Note that ECN problems are not only contained in SYN packets being
> eventually dropped. You might have a success to establish a flow, and
> later get CE marks for all packets and cwnd converges to 1.

I see. So that makes ecn=1 default a pure fantasy.

Thanks for explaining this.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Eric Dumazet]

On Fri, 2014-10-31 at 00:16 +0100, Florian Westphal wrote:

> I see. So that makes ecn=1 default a pure fantasy.

Well, I had this dream about 2 or 3 years ago, but I eventually came to
this (sad) conclusion.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 30 Oct 2014 16:30:01 -0700

> On Fri, 2014-10-31 at 00:16 +0100, Florian Westphal wrote:
> 
>> I see. So that makes ecn=1 default a pure fantasy.
> 
> Well, I had this dream about 2 or 3 years ago, but I eventually came to
> this (sad) conclusion.

We really should have done this from the beginning, but I guess we
lacked the courage to do so.

Some security nuts will say that the ECN bits are some covert channel
of communication and block connections based upon that, instead of,
you know, just clearing the bits.

Ok ok, Florian please resubmit your original patches.  We'll go with a
boolean per-route override.  Thanks for taking the time to discuss
this with us.

Re: [PATCH -next 0/2] net: allow setting ecn via routing table

[Daniel Borkmann]

On 10/31/2014 12:05 AM, Eric Dumazet wrote:
> On Thu, 2014-10-30 at 23:15 +0100, Florian Westphal wrote:
>
>> Do you think a fallback to non-ecn for retransmitted syns would help?
>> If not, do you think having ecn tunable available via route is helpful?
>
> Unfortunately some firewalls are buggy and accept a single SYN per flow.
>
> You would need to blacklist ecn at first sign of a possible ecn problem,
> for all following connections attempts.
>
> Note that ECN problems are not only contained in SYN packets being
> eventually dropped. You might have a success to establish a flow, and
> later get CE marks for all packets and cwnd converges to 1.

Wow, that is buggy! Btw, fwiw, there was a recent study [1] (paper
not public yet) which scanned the Alexa's publicly available top
million websites list from a vantage point in US, Europe and Asia:

Half of the Alexa list will now happily use ECN (tcp_ecn=2, most
likely blamed to commit 255cac91c3c9 ;)), the break in connectivity
on-path was found is about 1 in 10,000 cases. Timeouts rather than
receiving back RSTs were much more common in the negotiation phase
(and mostly seen in the Alexa middle band, ranks around 50k-150k):
from 12-thousand hosts on which there _may_ be ECN-linked connection
failures, only 79 failed with RST when _not_ failing with RST when
ECN is not requested.

It's unclear though, how much equipment actually marks the CE, and
as you mention above, marks it correctly ...

> This is really a lot of work to get all sorted out.

Yep, you are right.

  [1] http://ecn.ethz.ch/