Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

Florian Westphal <fw(a)strlen.de> wrote:
> Peter Krystad <peter.krystad(a)linux.intel.com> wrote:
> > 
> > Hi Florian -
> > 
> > My thinking here was that code in mptcp_parse_options() just parses the
> > options without having knowledge about configuration. There is existing logic
> > for peer requesting checksums in subflow_v4_init_req().
> 
> Uh, I don't understand this then.  Can you fix this?
> 
> mptcp-next can't talk to mptcp kernel unless one disables checksums on
> the mptcp kernel.
> 
> mptcp (out of tree) excepts checksums in default settings and will reset
> the connection quickly.  I don't see how to fallback to tcp in that case
> except with the patch I sent (not setting mp capable).

I hacked this thing together but I can't say I like it, but I found no
other solution to transport the needed information (csum requested)
to the spots that need to withdraw the mp_capable=1 decision.

 net/mptcp/options.c |  3 +++
 net/mptcp/subflow.c | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 9f892478d336..2bfba0cee72c 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -320,6 +320,9 @@ void mptcp_rcv_synsent(struct sock *sk)
 	struct subflow_context *subflow = subflow_ctx(sk);
 
 	if (subflow->request_mptcp && tp->rx_opt.mptcp.mp_capable) {
+		if (tp->rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
+			subflow->use_checksum = 1;
+
 		subflow->mp_capable = 1;
 		subflow->remote_key = tp->rx_opt.mptcp.sndr_key;
 		pr_debug("subflow=%p, remote_key=%llu", subflow,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c9dcb601dce3..ec1e1ada657b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -74,6 +74,22 @@ static void subflow_v4_init_req(struct request_sock *req,
 	subflow_req->mp_capable = 0;
 	subflow_req->mp_join = 0;
 
+	/* RFC 6824, Section 3.1:
+	 * "For the Checksum Required bit (labeled "A"), if either
+	 * host requires the use of checksums, checksums MUST be used.
+	 * In other words, the only way for checksums not to be used
+	 * is if both hosts in their SYNs set A=0."
+	 *
+	 * Section 3.3.0:
+	 * "If a checksum is not present when its use has been
+	 * negotiated, the receiver MUST close the subflow with a RST as it is
+	 * considered broken."
+	 *
+	 * We don't implement DSS checksum - fall back to TCP.
+	 */
+	if (rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
+		return;
+
 	if (rx_opt.mptcp.mp_capable && rx_opt.mptcp.mp_join)
 		return;
 
@@ -148,6 +164,11 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 	if (subflow->mp_capable && !subflow->conn_finished) {
 		pr_debug("subflow=%p, remote_key=%llu", subflow_ctx(sk),
 			 subflow->remote_key);
+
+		/* We don't support DSS checksums, no choice but to disable mptcp */
+		if (subflow->use_checksum)
+			subflow->mp_capable = 0;
+
 		mptcp_finish_connect(subflow->conn, subflow->mp_capable);
 		subflow->conn_finished = 1;
 
-- 
2.21.0

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

Hi Florian, Peter, Christoph,

On 27/08/2019 19:57, Peter Krystad wrote:
> 
> Hi Florian - 
> 
> Yeah, this isn't great, I am OK with your original patch. I'll take a to-do to
> remove the 'request_cksum' logic, since it doesn't have any purpose until
> checksums are implemented.

Thank you for the patch and the reviews!

Just applied in the repo:
- 87fd3fd5c2fc: "squashed" in "mptcp: Handle MPTCP TCP options"
- 16c36ea3d8e9: signed-off
- 0a3de10b90d9..ca4c26db67e0: result

Tests are still OK!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Peter Krystad]

[-- Attachment #1: Type: text/plain, Size: 3580 bytes --]


Hi Florian - 

Yeah, this isn't great, I am OK with your original patch. I'll take a to-do to
remove the 'request_cksum' logic, since it doesn't have any purpose until
checksums are implemented.

Begs the question then, where do we stand on checksums? Not necessary for
initial submission?

Peter.


 
On Tue, 2019-08-27 at 12:53 +0200, Florian Westphal wrote:
> Florian Westphal <fw(a)strlen.de> wrote:
> > Peter Krystad <peter.krystad(a)linux.intel.com> wrote:
> > > Hi Florian -
> > > 
> > > My thinking here was that code in mptcp_parse_options() just parses the
> > > options without having knowledge about configuration. There is existing logic
> > > for peer requesting checksums in subflow_v4_init_req().
> > 
> > Uh, I don't understand this then.  Can you fix this?
> > 
> > mptcp-next can't talk to mptcp kernel unless one disables checksums on
> > the mptcp kernel.
> > 
> > mptcp (out of tree) excepts checksums in default settings and will reset
> > the connection quickly.  I don't see how to fallback to tcp in that case
> > except with the patch I sent (not setting mp capable).
> 
> I hacked this thing together but I can't say I like it, but I found no
> other solution to transport the needed information (csum requested)
> to the spots that need to withdraw the mp_capable=1 decision.
> 
>  net/mptcp/options.c |  3 +++
>  net/mptcp/subflow.c | 21 +++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 9f892478d336..2bfba0cee72c 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -320,6 +320,9 @@ void mptcp_rcv_synsent(struct sock *sk)
>  	struct subflow_context *subflow = subflow_ctx(sk);
>  
>  	if (subflow->request_mptcp && tp->rx_opt.mptcp.mp_capable) {
> +		if (tp->rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
> +			subflow->use_checksum = 1;
> +
>  		subflow->mp_capable = 1;
>  		subflow->remote_key = tp->rx_opt.mptcp.sndr_key;
>  		pr_debug("subflow=%p, remote_key=%llu", subflow,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index c9dcb601dce3..ec1e1ada657b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -74,6 +74,22 @@ static void subflow_v4_init_req(struct request_sock *req,
>  	subflow_req->mp_capable = 0;
>  	subflow_req->mp_join = 0;
>  
> +	/* RFC 6824, Section 3.1:
> +	 * "For the Checksum Required bit (labeled "A"), if either
> +	 * host requires the use of checksums, checksums MUST be used.
> +	 * In other words, the only way for checksums not to be used
> +	 * is if both hosts in their SYNs set A=0."
> +	 *
> +	 * Section 3.3.0:
> +	 * "If a checksum is not present when its use has been
> +	 * negotiated, the receiver MUST close the subflow with a RST as it is
> +	 * considered broken."
> +	 *
> +	 * We don't implement DSS checksum - fall back to TCP.
> +	 */
> +	if (rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
> +		return;
> +
>  	if (rx_opt.mptcp.mp_capable && rx_opt.mptcp.mp_join)
>  		return;
>  
> @@ -148,6 +164,11 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  	if (subflow->mp_capable && !subflow->conn_finished) {
>  		pr_debug("subflow=%p, remote_key=%llu", subflow_ctx(sk),
>  			 subflow->remote_key);
> +
> +		/* We don't support DSS checksums, no choice but to disable mptcp */
> +		if (subflow->use_checksum)
> +			subflow->mp_capable = 0;
> +
>  		mptcp_finish_connect(subflow->conn, subflow->mp_capable);
>  		subflow->conn_finished = 1;
>  

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Christoph Paasch]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]

Hello,

On 27/08/19 - 12:53:34, Florian Westphal wrote:
> Florian Westphal <fw(a)strlen.de> wrote:
> > Peter Krystad <peter.krystad(a)linux.intel.com> wrote:
> > > 
> > > Hi Florian -
> > > 
> > > My thinking here was that code in mptcp_parse_options() just parses the
> > > options without having knowledge about configuration. There is existing logic
> > > for peer requesting checksums in subflow_v4_init_req().

I don't think it is bad to have these decisions made in
mptcp_parse_options(). E.g., tcp_parse_options() also makes decisions based
on the system's config (see the checks for the TCP-timestamp sysctl and
others).

Passing around these things across the stack makes the design more complex
IMO.


Cheers,
Christoph

> > 
> > Uh, I don't understand this then.  Can you fix this?
> > 
> > mptcp-next can't talk to mptcp kernel unless one disables checksums on
> > the mptcp kernel.
> > 
> > mptcp (out of tree) excepts checksums in default settings and will reset
> > the connection quickly.  I don't see how to fallback to tcp in that case
> > except with the patch I sent (not setting mp capable).
> 
> I hacked this thing together but I can't say I like it, but I found no
> other solution to transport the needed information (csum requested)
> to the spots that need to withdraw the mp_capable=1 decision.
> 
>  net/mptcp/options.c |  3 +++
>  net/mptcp/subflow.c | 21 +++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 9f892478d336..2bfba0cee72c 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -320,6 +320,9 @@ void mptcp_rcv_synsent(struct sock *sk)
>  	struct subflow_context *subflow = subflow_ctx(sk);
>  
>  	if (subflow->request_mptcp && tp->rx_opt.mptcp.mp_capable) {
> +		if (tp->rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
> +			subflow->use_checksum = 1;
> +
>  		subflow->mp_capable = 1;
>  		subflow->remote_key = tp->rx_opt.mptcp.sndr_key;
>  		pr_debug("subflow=%p, remote_key=%llu", subflow,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index c9dcb601dce3..ec1e1ada657b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -74,6 +74,22 @@ static void subflow_v4_init_req(struct request_sock *req,
>  	subflow_req->mp_capable = 0;
>  	subflow_req->mp_join = 0;
>  
> +	/* RFC 6824, Section 3.1:
> +	 * "For the Checksum Required bit (labeled "A"), if either
> +	 * host requires the use of checksums, checksums MUST be used.
> +	 * In other words, the only way for checksums not to be used
> +	 * is if both hosts in their SYNs set A=0."
> +	 *
> +	 * Section 3.3.0:
> +	 * "If a checksum is not present when its use has been
> +	 * negotiated, the receiver MUST close the subflow with a RST as it is
> +	 * considered broken."
> +	 *
> +	 * We don't implement DSS checksum - fall back to TCP.
> +	 */
> +	if (rx_opt.mptcp.flags & MPTCP_CAP_CHECKSUM_REQD)
> +		return;
> +
>  	if (rx_opt.mptcp.mp_capable && rx_opt.mptcp.mp_join)
>  		return;
>  
> @@ -148,6 +164,11 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  	if (subflow->mp_capable && !subflow->conn_finished) {
>  		pr_debug("subflow=%p, remote_key=%llu", subflow_ctx(sk),
>  			 subflow->remote_key);
> +
> +		/* We don't support DSS checksums, no choice but to disable mptcp */
> +		if (subflow->use_checksum)
> +			subflow->mp_capable = 0;
> +
>  		mptcp_finish_connect(subflow->conn, subflow->mp_capable);
>  		subflow->conn_finished = 1;
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

Peter Krystad <peter.krystad(a)linux.intel.com> wrote:
> 
> Hi Florian -
> 
> My thinking here was that code in mptcp_parse_options() just parses the
> options without having knowledge about configuration. There is existing logic
> for peer requesting checksums in subflow_v4_init_req().

Uh, I don't understand this then.  Can you fix this?

mptcp-next can't talk to mptcp kernel unless one disables checksums on
the mptcp kernel.

mptcp (out of tree) excepts checksums in default settings and will reset
the connection quickly.  I don't see how to fallback to tcp in that case
except with the patch I sent (not setting mp capable).

Thanks.

Re: [MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Peter Krystad]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]


Hi Florian -

My thinking here was that code in mptcp_parse_options() just parses the
options without having knowledge about configuration. There is existing logic
for peer requesting checksums in subflow_v4_init_req().

Peter.

On Mon, 2019-08-26 at 14:48 +0200, Florian Westphal wrote:
> No other choice.  Without this, peer that requested DSS checksum will reset
> the connection as soon as DSS options without checksums arrive.
> 
> squashto: mptcp: Handle MPTCP TCP options
> 
> Cc: Peter Krystad <peter.krystad(a)linux.intel.com>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/options.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 9f892478d336..deee4394c4c5 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -37,6 +37,22 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
>  		    (mp_opt->flags & MPTCP_CAP_EXTENSIBILITY))
>  			break;
>  
> +		/* RFC 6824, Section 3.1:
> +		 * "For the Checksum Required bit (labeled "A"), if either
> +		 * host requires the use of checksums, checksums MUST be used.
> +		 * In other words, the only way for checksums not to be used
> +		 * is if both hosts in their SYNs set A=0."
> +		 *
> +		 * Section 3.3.0:
> +		 * "If a checksum is not present when its use has been
> +		 * negotiated, the receiver MUST close the subflow with a RST as it is
> +		 * considered broken."
> +		 *
> +		 * We don't implement DSS checksum - fall back to TCP.
> +		 */
> +		if (mp_opt->flags & MPTCP_CAP_CHECKSUM_REQD)
> +			break;
> +
>  		mp_opt->mp_capable = 1;
>  		mp_opt->sndr_key = get_unaligned_be64(ptr);
>  		ptr += 8;

[MPTCP] [PATCH] mptcp: fall back to tcp when dss checksum are requested

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

No other choice.  Without this, peer that requested DSS checksum will reset
the connection as soon as DSS options without checksums arrive.

squashto: mptcp: Handle MPTCP TCP options

Cc: Peter Krystad <peter.krystad(a)linux.intel.com>
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 net/mptcp/options.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 9f892478d336..deee4394c4c5 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -37,6 +37,22 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
 		    (mp_opt->flags & MPTCP_CAP_EXTENSIBILITY))
 			break;
 
+		/* RFC 6824, Section 3.1:
+		 * "For the Checksum Required bit (labeled "A"), if either
+		 * host requires the use of checksums, checksums MUST be used.
+		 * In other words, the only way for checksums not to be used
+		 * is if both hosts in their SYNs set A=0."
+		 *
+		 * Section 3.3.0:
+		 * "If a checksum is not present when its use has been
+		 * negotiated, the receiver MUST close the subflow with a RST as it is
+		 * considered broken."
+		 *
+		 * We don't implement DSS checksum - fall back to TCP.
+		 */
+		if (mp_opt->flags & MPTCP_CAP_CHECKSUM_REQD)
+			break;
+
 		mp_opt->mp_capable = 1;
 		mp_opt->sndr_key = get_unaligned_be64(ptr);
 		ptr += 8;
-- 
2.21.0