[MPTCP][PATCH mptcp-next 0/6] DSS checksum support

[MPTCP][PATCH mptcp-next 0/6] DSS checksum support

[Geliang Tang]

apply: export/20210320T065116 + "refactor mptcp_addr_info and cleanups"

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/134

Geliang Tang (6):
  mptcp: add a new sysctl checksum_enabled
  mptcp: add csum_enabled in mptcp_out_options
  mptcp: add the csum_reqd fields
  mptcp: add the DSS checksum sending
  mptcp: add the DSS checksum receiving
  mptcp: add trace event for DSS checksum

 Documentation/networking/mptcp-sysctl.rst |  8 +++
 include/net/mptcp.h                       |  4 +-
 include/trace/events/mptcp.h              | 51 +++++++++++++++
 net/mptcp/ctrl.c                          | 14 +++++
 net/mptcp/options.c                       | 76 ++++++++++++++++++++---
 net/mptcp/protocol.c                      |  4 ++
 net/mptcp/protocol.h                      | 14 ++++-
 net/mptcp/subflow.c                       | 46 ++++++++++++++
 8 files changed, 205 insertions(+), 12 deletions(-)
 create mode 100644 include/trace/events/mptcp.h

-- 
2.30.2

[MPTCP][PATCH mptcp-next 1/6] mptcp: add a new sysctl checksum_enabled

[Geliang Tang]

This patch added a new sysctl, named checksum_enabled, to control
whether DSS checksum can be enabled.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 Documentation/networking/mptcp-sysctl.rst |  8 ++++++++
 net/mptcp/ctrl.c                          | 14 ++++++++++++++
 net/mptcp/protocol.h                      |  1 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index 6af0196c4297..1128e09d6c4d 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -24,3 +24,11 @@ add_addr_timeout - INTEGER (seconds)
 	sysctl.
 
 	Default: 120
+
+checksum_enabled - INTEGER
+	Control whether DSS checksum can be enabled.
+
+	DSS checksum can be enabled if the value is nonzero. This is a
+	per-namespace sysctl.
+
+	Default: 1
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 96ba616f59bf..115cebc8c02a 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -19,6 +19,7 @@ struct mptcp_pernet {
 
 	int mptcp_enabled;
 	unsigned int add_addr_timeout;
+	int checksum_enabled;
 };
 
 static struct mptcp_pernet *mptcp_get_pernet(struct net *net)
@@ -36,6 +37,11 @@ unsigned int mptcp_get_add_addr_timeout(struct net *net)
 	return mptcp_get_pernet(net)->add_addr_timeout;
 }
 
+int mptcp_is_checksum_enabled(struct net *net)
+{
+	return mptcp_get_pernet(net)->checksum_enabled;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -52,6 +58,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.mode = 0644,
 		.proc_handler = proc_dointvec_jiffies,
 	},
+	{
+		.procname = "checksum_enabled",
+		.maxlen = sizeof(int),
+		.mode = 0644,
+		.proc_handler = proc_dointvec,
+	},
 	{}
 };
 
@@ -59,6 +71,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 {
 	pernet->mptcp_enabled = 1;
 	pernet->add_addr_timeout = TCP_RTO_MAX;
+	pernet->checksum_enabled = 1;
 }
 
 static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
@@ -75,6 +88,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 
 	table[0].data = &pernet->mptcp_enabled;
 	table[1].data = &pernet->add_addr_timeout;
+	table[2].data = &pernet->checksum_enabled;
 
 	hdr = register_net_sysctl(net, MPTCP_SYSCTL_PATH, table);
 	if (!hdr)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 14f0114be17a..fd36239d8905 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -519,6 +519,7 @@ static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *su
 
 int mptcp_is_enabled(struct net *net);
 unsigned int mptcp_get_add_addr_timeout(struct net *net);
+int mptcp_is_checksum_enabled(struct net *net);
 void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
 				     struct mptcp_options_received *mp_opt);
 bool mptcp_subflow_data_available(struct sock *sk);
-- 
2.30.2

[MPTCP][PATCH mptcp-next 2/6] mptcp: add csum_enabled in mptcp_out_options

[Geliang Tang]

This patch add a new member csum_enabled in struct mptcp_out_options.
In mptcp_write_options, if this csum_enabled field is enabled, send out
the MP_CAPABLE suboption with the MPTCP_CAP_CHECKSUM_REQD flag.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 include/net/mptcp.h  |  3 ++-
 net/mptcp/options.c  | 10 ++++++++--
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  |  1 +
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 83f23774b908..36fb6907aa6f 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -64,7 +64,8 @@ struct mptcp_out_options {
 	u8 join_id;
 	u8 backup;
 	u8 reset_reason:4;
-	u8 reset_transient:1;
+	u8 reset_transient:1,
+	   csum_enabled:1;
 	u32 nonce;
 	u64 thmac;
 	u32 token;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e3fcd2b0ffd7..f45bebb923dc 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -379,6 +379,7 @@ bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
 	subflow->snd_isn = TCP_SKB_CB(skb)->end_seq;
 	if (subflow->request_mptcp) {
 		opts->suboptions = OPTION_MPTCP_MPC_SYN;
+		opts->csum_enabled = mptcp_is_checksum_enabled(sock_net(sk));
 		*size = TCPOLEN_MPTCP_MPC_SYN;
 		return true;
 	} else if (subflow->request_join) {
@@ -464,6 +465,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 		opts->suboptions = OPTION_MPTCP_MPC_ACK;
 		opts->sndr_key = subflow->local_key;
 		opts->rcvr_key = subflow->remote_key;
+		opts->csum_enabled = mptcp_is_checksum_enabled(sock_net(sk));
 
 		/* Section 3.1.
 		 * The MP_CAPABLE option is carried on the SYN, SYN/ACK, and ACK
@@ -786,6 +788,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 	if (subflow_req->mp_capable) {
 		opts->suboptions = OPTION_MPTCP_MPC_SYNACK;
 		opts->sndr_key = subflow_req->local_key;
+		opts->csum_enabled = subflow_req->csum_enabled;
 		*size = TCPOLEN_MPTCP_MPC_SYNACK;
 		pr_debug("subflow_req=%p, local_key=%llu",
 			 subflow_req, subflow_req->local_key);
@@ -1116,7 +1119,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 {
 	if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
 	     OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
-		u8 len;
+		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
 
 		if (OPTION_MPTCP_MPC_SYN & opts->suboptions)
 			len = TCPOLEN_MPTCP_MPC_SYN;
@@ -1127,9 +1130,12 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 		else
 			len = TCPOLEN_MPTCP_MPC_ACK;
 
+		if (opts->csum_enabled)
+			flag |= MPTCP_CAP_CHECKSUM_REQD;
+
 		*ptr++ = mptcp_option(MPTCPOPT_MP_CAPABLE, len,
 				      MPTCP_SUPPORTED_VERSION,
-				      MPTCP_CAP_HMAC_SHA256);
+				      flag);
 
 		if (!((OPTION_MPTCP_MPC_SYNACK | OPTION_MPTCP_MPC_ACK) &
 		    opts->suboptions))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index fd36239d8905..14f1e2fd3c08 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -335,7 +335,8 @@ struct mptcp_subflow_request_sock {
 	struct	tcp_request_sock sk;
 	u16	mp_capable : 1,
 		mp_join : 1,
-		backup : 1;
+		backup : 1,
+		csum_enabled : 1;
 	u8	local_id;
 	u8	remote_id;
 	u64	local_key;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 5fc3cada11dd..e2e9cc34713d 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -106,6 +106,7 @@ static void subflow_init_req(struct request_sock *req, const struct sock *sk_lis
 
 	subflow_req->mp_capable = 0;
 	subflow_req->mp_join = 0;
+	subflow_req->csum_enabled = mptcp_is_checksum_enabled(sock_net(sk_listener));
 	subflow_req->msk = NULL;
 	mptcp_token_init_request(req);
 }
-- 
2.30.2

[MPTCP][PATCH mptcp-next 3/6] mptcp: add the csum_reqd fields

[Geliang Tang]

This patch added a new flag csum_reqd in struct mptcp_options_received, if
the flag MPTCP_CAP_CHECKSUM_REQD is set in the receiving MP_CAPABLE
suboption, set this csum_reqd flag.

It also added another new csum_reqd member in struct mptcp_sock too, if the
csum_reqd flag in mptcp_options_received is set, set this member to true.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c  | 8 +++++---
 net/mptcp/protocol.c | 1 +
 net/mptcp/protocol.h | 2 ++
 net/mptcp/subflow.c  | 4 ++++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index f45bebb923dc..bdced173edff 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -69,11 +69,9 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		 * "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 (flags & MPTCP_CAP_CHECKSUM_REQD)
-			break;
+			mp_opt->csum_reqd = 1;
 
 		mp_opt->mp_capable = 1;
 		if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
@@ -340,6 +338,7 @@ void mptcp_get_options(const struct sk_buff *skb,
 	mp_opt->dss = 0;
 	mp_opt->mp_prio = 0;
 	mp_opt->reset = 0;
+	mp_opt->csum_reqd = 0;
 
 	length = (th->doff * 4) - sizeof(struct tcphdr);
 	ptr = (const unsigned char *)(th + 1);
@@ -1013,6 +1012,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		mptcp_schedule_work((struct sock *)msk);
 	}
 
+	if (mp_opt.csum_reqd)
+		WRITE_ONCE(msk->csum_reqd, true);
+
 	if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
 		if (!mp_opt.echo) {
 			mptcp_pm_add_addr_received(msk, &mp_opt.addr);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 9d7e7e13fba8..94c3568c7a60 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2364,6 +2364,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	msk->ack_hint = NULL;
 	msk->first = NULL;
 	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
+	WRITE_ONCE(msk->csum_reqd, mptcp_is_checksum_enabled(sock_net(sk)));
 
 	mptcp_pm_data_init(msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 14f1e2fd3c08..4fc4871595ef 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -132,6 +132,7 @@ struct mptcp_options_received {
 		rm_addr : 1,
 		mp_prio : 1,
 		echo : 1,
+		csum_reqd : 1,
 		backup : 1;
 	u32	token;
 	u32	nonce;
@@ -233,6 +234,7 @@ struct mptcp_sock {
 	bool		snd_data_fin_enable;
 	bool		rcv_fastclose;
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
+	bool		csum_reqd;
 	spinlock_t	join_list_lock;
 	struct sock	*ack_hint;
 	struct work_struct work;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index e2e9cc34713d..9fec1a273100 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -403,6 +403,8 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 			goto fallback;
 		}
 
+		if (mp_opt.csum_reqd)
+			WRITE_ONCE(mptcp_sk(parent)->csum_reqd, true);
 		subflow->mp_capable = 1;
 		subflow->can_ack = 1;
 		subflow->remote_key = mp_opt.sndr_key;
@@ -643,6 +645,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
 		if (!new_msk)
 			fallback = true;
+		else if (mp_opt.csum_reqd)
+			WRITE_ONCE(mptcp_sk(new_msk)->csum_reqd, true);
 	} else if (subflow_req->mp_join) {
 		mptcp_get_options(skb, &mp_opt);
 		if (!mp_opt.mp_join || !subflow_hmac_valid(req, &mp_opt) ||
-- 
2.30.2

[MPTCP][PATCH mptcp-next 4/6] mptcp: add the DSS checksum sending

[Geliang Tang]

In mptcp_established_options_dss, use the new function
mptcp_generate_dss_csum to generate the DSS checksum value.

In mptcp_write_options, send out DSS with the checksum value.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 include/net/mptcp.h  |  1 +
 net/mptcp/options.c  | 41 ++++++++++++++++++++++++++++++++++++++---
 net/mptcp/protocol.h |  7 +++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 36fb6907aa6f..869a6d98a72e 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -32,6 +32,7 @@ struct mptcp_ext {
 			frozen:1,
 			reset_transient:1;
 	u8		reset_reason:4;
+	u16		csum;
 };
 
 #define MPTCP_RM_IDS_MAX	8
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index bdced173edff..69eb15ef9385 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -521,6 +521,30 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
 	}
 }
 
+static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
+{
+	struct csum_pseudo_header header;
+	struct mptcp_ext *mpext;
+	__wsum csum;
+
+	if (!skb)
+		return 0;
+
+	mpext = mptcp_get_ext(skb);
+	if (!mpext || !mpext->use_map)
+		return 0;
+
+	header.data_seq = mpext->data_seq;
+	header.subflow_seq = mpext->subflow_seq;
+	header.data_len = mpext->data_len;
+	header.csum = 0;
+
+	csum = skb_checksum(skb, 0, skb->len, 0);
+	csum = csum_partial(&header, sizeof(header), csum);
+
+	return csum_fold(csum);
+}
+
 static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
 					  bool snd_data_fin_enable,
 					  unsigned int *size,
@@ -544,8 +568,11 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
 
 		remaining -= map_size;
 		dss_size = map_size;
-		if (mpext)
+		if (mpext) {
+			if (msk->csum_reqd)
+				mpext->csum = mptcp_generate_dss_csum(skb);
 			opts->ext_copy = *mpext;
+		}
 
 		if (skb && snd_data_fin_enable)
 			mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
@@ -1304,6 +1331,9 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
 			if (mpext->data_fin)
 				flags |= MPTCP_DSS_DATA_FIN;
+
+			if (mpext->csum)
+				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
 		}
 
 		*ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
@@ -1323,8 +1353,13 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			ptr += 2;
 			put_unaligned_be32(mpext->subflow_seq, ptr);
 			ptr += 1;
-			put_unaligned_be32(mpext->data_len << 16 |
-					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
+			if (mpext->csum) {
+				put_unaligned_be32(mpext->data_len << 16 |
+						   mpext->csum, ptr);
+			} else {
+				put_unaligned_be32(mpext->data_len << 16 |
+						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
+			}
 		}
 	}
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4fc4871595ef..f62cea8635f0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -333,6 +333,13 @@ static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk)
 	return list_first_entry_or_null(&msk->rtx_queue, struct mptcp_data_frag, list);
 }
 
+struct csum_pseudo_header {
+	u64 data_seq;
+	u32 subflow_seq;
+	u16 data_len;
+	u16 csum;
+};
+
 struct mptcp_subflow_request_sock {
 	struct	tcp_request_sock sk;
 	u16	mp_capable : 1,
-- 
2.30.2

[MPTCP][PATCH mptcp-next 5/6] mptcp: add the DSS checksum receiving

[Geliang Tang]

This patch added a new member csum in struct mptcp_options_received. In
mptcp_parse_option, parse the checksum value from the receiving DSS, and
save it in mp_opt->csum. In mptcp_incoming_options, pass it to
mpext->csum.

In validate_mapping, use the new function validate_dss_csum to validata
the DSS checksum.

In validate_dss_csum, if the data has been split in different packets,
skip validating.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c  | 13 +++++++++++--
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 69eb15ef9385..ed89f6c2ed49 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -206,9 +206,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			mp_opt->data_len = get_unaligned_be16(ptr);
 			ptr += 2;
 
-			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u",
+			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
+				mp_opt->csum = get_unaligned_be16(ptr);
+				ptr += 2;
+			}
+
+			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
 				 mp_opt->data_seq, mp_opt->subflow_seq,
-				 mp_opt->data_len);
+				 mp_opt->data_len, mp_opt->csum);
 		}
 
 		break;
@@ -339,6 +344,7 @@ void mptcp_get_options(const struct sk_buff *skb,
 	mp_opt->mp_prio = 0;
 	mp_opt->reset = 0;
 	mp_opt->csum_reqd = 0;
+	mp_opt->csum = 0;
 
 	length = (th->doff * 4) - sizeof(struct tcphdr);
 	ptr = (const unsigned char *)(th + 1);
@@ -1124,6 +1130,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		}
 		mpext->data_len = mp_opt.data_len;
 		mpext->use_map = 1;
+
+		if (READ_ONCE(msk->csum_reqd))
+			mpext->csum = mp_opt.csum;
 	}
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f62cea8635f0..51ef3173a2e5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -123,6 +123,7 @@ struct mptcp_options_received {
 	u64	data_seq;
 	u32	subflow_seq;
 	u16	data_len;
+	u16	csum;
 	u16	mp_capable : 1,
 		mp_join : 1,
 		fastclose : 1,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 9fec1a273100..2eeea7d527f0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -799,10 +799,45 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
 					  mptcp_subflow_get_map_offset(subflow);
 }
 
+static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
+{
+	struct csum_pseudo_header header;
+	struct mptcp_ext *mpext;
+	__wsum csum;
+
+	if (!skb)
+		goto out;
+
+	mpext = mptcp_get_ext(skb);
+	if (!mpext || !mpext->use_map || !mpext->csum ||
+	    mpext->data_len != skb->len)
+		goto out;
+
+	header.data_seq = mpext->data_seq;
+	header.subflow_seq = mpext->subflow_seq;
+	header.data_len = mpext->data_len;
+	header.csum = mpext->csum;
+
+	csum = skb_checksum(skb, 0, skb->len, 0);
+	csum = csum_partial(&header, sizeof(header), csum);
+
+	if (csum_fold(csum)) {
+		pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
+		       header.data_seq, header.subflow_seq, header.data_len,
+		       header.csum, csum_fold(csum));
+
+		return false;
+	}
+
+out:
+	return true;
+}
+
 static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 
 	if (unlikely(before(ssn, subflow->map_subflow_seq))) {
 		/* Mapping covers data later in the subflow stream,
@@ -817,6 +852,8 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
 		warn_bad_map(subflow, ssn + skb->len);
 		return false;
 	}
+	if (READ_ONCE(msk->csum_reqd))
+		validate_dss_csum(ssk, skb);
 	return true;
 }
 
-- 
2.30.2

[MPTCP][PATCH mptcp-next 6/6] mptcp: add trace event for DSS checksum

[Geliang Tang]

This patch added the trace event for DSS checksum. Add a tracepoint in
mptcp_generate_dss_csum and a tracepoint in validate_dss_csum.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 include/trace/events/mptcp.h | 51 ++++++++++++++++++++++++++++++++++++
 net/mptcp/options.c          |  4 +++
 net/mptcp/protocol.c         |  3 +++
 net/mptcp/subflow.c          |  4 +++
 4 files changed, 62 insertions(+)
 create mode 100644 include/trace/events/mptcp.h

diff --git a/include/trace/events/mptcp.h b/include/trace/events/mptcp.h
new file mode 100644
index 000000000000..a5b87e304303
--- /dev/null
+++ b/include/trace/events/mptcp.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM mptcp
+
+#if !defined(_TRACE_MPTCP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_MPTCP_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(mptcp_event_dss_csum,
+
+	TP_PROTO(struct csum_pseudo_header *header, __u16 csum),
+
+	TP_ARGS(header, csum),
+
+	TP_STRUCT__entry(
+		__field(__u64, data_seq)
+		__field(__u32, subflow_seq)
+		__field(__u16, data_len)
+		__field(__u16, dss_csum)
+		__field(__u16, csum)
+	),
+
+	TP_fast_assign(
+		__entry->data_seq = header->data_seq;
+		__entry->subflow_seq = header->subflow_seq;
+		__entry->data_len = header->data_len;
+		__entry->dss_csum = header->csum;
+		__entry->csum = csum;
+	),
+
+	TP_printk("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
+		  __entry->data_seq, __entry->subflow_seq, __entry->data_len,
+		  __entry->csum)
+);
+
+DEFINE_EVENT(mptcp_event_dss_csum, mptcp_generate_dss_csum,
+	TP_PROTO(struct csum_pseudo_header *header, __u16 csum),
+	TP_ARGS(header, csum));
+
+DEFINE_EVENT_PRINT(mptcp_event_dss_csum, validate_dss_csum,
+	TP_PROTO(struct csum_pseudo_header *header, __u16 csum),
+	TP_ARGS(header, csum),
+	TP_printk("data_seq=%llu subflow_seq=%u data_len=%u csum=%u, DSS checksum %s!",
+		  __entry->data_seq, __entry->subflow_seq, __entry->data_len,
+		  __entry->dss_csum, __entry->csum ? "error" : "done"));
+
+#endif /* _TRACE_MPTCP_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ed89f6c2ed49..8ec41bf56c3b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -13,6 +13,8 @@
 #include "protocol.h"
 #include "mib.h"
 
+#include <trace/events/mptcp.h>
+
 static bool mptcp_cap_flag_sha256(u8 flags)
 {
 	return (flags & MPTCP_CAP_FLAG_MASK) == MPTCP_CAP_HMAC_SHA256;
@@ -548,6 +550,8 @@ static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
 	csum = skb_checksum(skb, 0, skb->len, 0);
 	csum = csum_partial(&header, sizeof(header), csum);
 
+	trace_mptcp_generate_dss_csum(&header, csum_fold(csum));
+
 	return csum_fold(csum);
 }
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 94c3568c7a60..1f767612c917 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -25,6 +25,9 @@
 #include "protocol.h"
 #include "mib.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/mptcp.h>
+
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 struct mptcp6_sock {
 	struct mptcp_sock msk;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2eeea7d527f0..7e63d902a3de 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -27,6 +27,8 @@
 
 static void mptcp_subflow_ops_undo_override(struct sock *ssk);
 
+#include <trace/events/mptcp.h>
+
 static void SUBFLOW_REQ_INC_STATS(struct request_sock *req,
 				  enum linux_mptcp_mib_field field)
 {
@@ -821,6 +823,8 @@ static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
 	csum = skb_checksum(skb, 0, skb->len, 0);
 	csum = csum_partial(&header, sizeof(header), csum);
 
+	trace_validate_dss_csum(&header, csum_fold(csum));
+
 	if (csum_fold(csum)) {
 		pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
 		       header.data_seq, header.subflow_seq, header.data_len,
-- 
2.30.2

Re: [MPTCP][PATCH mptcp-next 5/6] mptcp: add the DSS checksum receiving

[Mat Martineau]

On Mon, 22 Mar 2021, Geliang Tang wrote:

> This patch added a new member csum in struct mptcp_options_received. In
> mptcp_parse_option, parse the checksum value from the receiving DSS, and
> save it in mp_opt->csum. In mptcp_incoming_options, pass it to
> mpext->csum.
>
> In validate_mapping, use the new function validate_dss_csum to validata
> the DSS checksum.
>
> In validate_dss_csum, if the data has been split in different packets,
> skip validating.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/options.c  | 13 +++++++++++--
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 69eb15ef9385..ed89f6c2ed49 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -206,9 +206,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> 			mp_opt->data_len = get_unaligned_be16(ptr);
> 			ptr += 2;
>
> -			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u",
> +			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {

Now that more of the checksum code has been added, I think that 
expected_opsize should be set to the exact value that is needed - either 
expect the checksum, or don't expect the checksum.

RFC 6824 said: "If a checksum is present, but its use had not been 
negotiated in the MP_CAPABLE handshake, the checksum field MUST be 
ignored."

RFC 8684 says: "If a checksum is present but its use had
not been negotiated in the MP_CAPABLE handshake, the receiver MUST
close the subflow with a RST, as it is not behaving as negotiated.
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."

There's some old RFC 6824-based code earlier in the function:

 		if (opsize != expected_opsize &&
 		    opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
 			break;

That needs to be changed to:

 		if (opsize != expected_opsize)
 			break;

and expected_opsize should include TCPOLEN_MPTCP_DSS_CHECKSUM only if the 
checksum is enabled.

> +				mp_opt->csum = get_unaligned_be16(ptr);
> +				ptr += 2;
> +			}
> +
> +			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
> 				 mp_opt->data_seq, mp_opt->subflow_seq,
> -				 mp_opt->data_len);
> +				 mp_opt->data_len, mp_opt->csum);
> 		}
>
> 		break;
> @@ -339,6 +344,7 @@ void mptcp_get_options(const struct sk_buff *skb,
> 	mp_opt->mp_prio = 0;
> 	mp_opt->reset = 0;
> 	mp_opt->csum_reqd = 0;
> +	mp_opt->csum = 0;
>
> 	length = (th->doff * 4) - sizeof(struct tcphdr);
> 	ptr = (const unsigned char *)(th + 1);
> @@ -1124,6 +1130,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> 		}
> 		mpext->data_len = mp_opt.data_len;
> 		mpext->use_map = 1;
> +
> +		if (READ_ONCE(msk->csum_reqd))
> +			mpext->csum = mp_opt.csum;
> 	}
> }
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f62cea8635f0..51ef3173a2e5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -123,6 +123,7 @@ struct mptcp_options_received {
> 	u64	data_seq;
> 	u32	subflow_seq;
> 	u16	data_len;
> +	u16	csum;
> 	u16	mp_capable : 1,
> 		mp_join : 1,
> 		fastclose : 1,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 9fec1a273100..2eeea7d527f0 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -799,10 +799,45 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
> 					  mptcp_subflow_get_map_offset(subflow);
> }
>
> +static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
> +{
> +	struct csum_pseudo_header header;
> +	struct mptcp_ext *mpext;
> +	__wsum csum;
> +
> +	if (!skb)
> +		goto out;
> +
> +	mpext = mptcp_get_ext(skb);
> +	if (!mpext || !mpext->use_map || !mpext->csum ||
> +	    mpext->data_len != skb->len)
> +		goto out;
> +
> +	header.data_seq = mpext->data_seq;
> +	header.subflow_seq = mpext->subflow_seq;
> +	header.data_len = mpext->data_len;
> +	header.csum = mpext->csum;
> +
> +	csum = skb_checksum(skb, 0, skb->len, 0);

The checksum isn't calculated using the data from a single skb, it 
requires the complete reassembled data for the current mapping. If we're 
lucky, GRO has placed everything in one skb. But that can't be assumed.

The checksum can be calculated incrementally as skbs arrive, but checksum 
verification will require changes to data reassembly and MPTCP-level 
acknowledgements, and will delay movement of data to the msk until the 
checksum is verified. Also review my comments on the MP_FAIL RFC patch:

https://lists.01.org/hyperkitty/list/mptcp@lists.01.org/message/KSEEB5Q4ND3GKOW7S3FZ32PHCWWON5QC/


Thanks for the patch set!

Mat


> +	csum = csum_partial(&header, sizeof(header), csum);
> +
> +	if (csum_fold(csum)) {
> +		pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
> +		       header.data_seq, header.subflow_seq, header.data_len,
> +		       header.csum, csum_fold(csum));
> +
> +		return false;
> +	}
> +
> +out:
> +	return true;
> +}
> +
> static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> 	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>
> 	if (unlikely(before(ssn, subflow->map_subflow_seq))) {
> 		/* Mapping covers data later in the subflow stream,
> @@ -817,6 +852,8 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
> 		warn_bad_map(subflow, ssn + skb->len);
> 		return false;
> 	}
> +	if (READ_ONCE(msk->csum_reqd))
> +		validate_dss_csum(ssk, skb);
> 	return true;
> }
>
> -- 
> 2.30.2

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next 5/6] mptcp: add the DSS checksum receiving

[Geliang Tang]

Hi Mat,

Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年3月23日周二 上午7:40写道：
>
> On Mon, 22 Mar 2021, Geliang Tang wrote:
>
> > This patch added a new member csum in struct mptcp_options_received. In
> > mptcp_parse_option, parse the checksum value from the receiving DSS, and
> > save it in mp_opt->csum. In mptcp_incoming_options, pass it to
> > mpext->csum.
> >
> > In validate_mapping, use the new function validate_dss_csum to validata
> > the DSS checksum.
> >
> > In validate_dss_csum, if the data has been split in different packets,
> > skip validating.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > net/mptcp/options.c  | 13 +++++++++++--
> > net/mptcp/protocol.h |  1 +
> > net/mptcp/subflow.c  | 37 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 69eb15ef9385..ed89f6c2ed49 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -206,9 +206,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> >                       mp_opt->data_len = get_unaligned_be16(ptr);
> >                       ptr += 2;
> >
> > -                     pr_debug("data_seq=%llu subflow_seq=%u data_len=%u",
> > +                     if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
>
> Now that more of the checksum code has been added, I think that
> expected_opsize should be set to the exact value that is needed - either
> expect the checksum, or don't expect the checksum.
>
> RFC 6824 said: "If a checksum is present, but its use had not been
> negotiated in the MP_CAPABLE handshake, the checksum field MUST be
> ignored."
>
> RFC 8684 says: "If a checksum is present but its use had
> not been negotiated in the MP_CAPABLE handshake, the receiver MUST
> close the subflow with a RST, as it is not behaving as negotiated.
> 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."
>
> There's some old RFC 6824-based code earlier in the function:
>
>                 if (opsize != expected_opsize &&
>                     opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
>                         break;
>
> That needs to be changed to:
>
>                 if (opsize != expected_opsize)
>                         break;
>
> and expected_opsize should include TCPOLEN_MPTCP_DSS_CHECKSUM only if the
> checksum is enabled.
>
> > +                             mp_opt->csum = get_unaligned_be16(ptr);
> > +                             ptr += 2;
> > +                     }
> > +
> > +                     pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
> >                                mp_opt->data_seq, mp_opt->subflow_seq,
> > -                              mp_opt->data_len);
> > +                              mp_opt->data_len, mp_opt->csum);
> >               }
> >
> >               break;
> > @@ -339,6 +344,7 @@ void mptcp_get_options(const struct sk_buff *skb,
> >       mp_opt->mp_prio = 0;
> >       mp_opt->reset = 0;
> >       mp_opt->csum_reqd = 0;
> > +     mp_opt->csum = 0;
> >
> >       length = (th->doff * 4) - sizeof(struct tcphdr);
> >       ptr = (const unsigned char *)(th + 1);
> > @@ -1124,6 +1130,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> >               }
> >               mpext->data_len = mp_opt.data_len;
> >               mpext->use_map = 1;
> > +
> > +             if (READ_ONCE(msk->csum_reqd))
> > +                     mpext->csum = mp_opt.csum;
> >       }
> > }
> >
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index f62cea8635f0..51ef3173a2e5 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -123,6 +123,7 @@ struct mptcp_options_received {
> >       u64     data_seq;
> >       u32     subflow_seq;
> >       u16     data_len;
> > +     u16     csum;
> >       u16     mp_capable : 1,
> >               mp_join : 1,
> >               fastclose : 1,
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 9fec1a273100..2eeea7d527f0 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -799,10 +799,45 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
> >                                         mptcp_subflow_get_map_offset(subflow);
> > }
> >
> > +static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
> > +{
> > +     struct csum_pseudo_header header;
> > +     struct mptcp_ext *mpext;
> > +     __wsum csum;
> > +
> > +     if (!skb)
> > +             goto out;
> > +
> > +     mpext = mptcp_get_ext(skb);
> > +     if (!mpext || !mpext->use_map || !mpext->csum ||
> > +         mpext->data_len != skb->len)
> > +             goto out;
> > +
> > +     header.data_seq = mpext->data_seq;
> > +     header.subflow_seq = mpext->subflow_seq;
> > +     header.data_len = mpext->data_len;
> > +     header.csum = mpext->csum;
> > +
> > +     csum = skb_checksum(skb, 0, skb->len, 0);
>
> The checksum isn't calculated using the data from a single skb, it
> requires the complete reassembled data for the current mapping. If we're
> lucky, GRO has placed everything in one skb. But that can't be assumed.
>
> The checksum can be calculated incrementally as skbs arrive, but checksum
> verification will require changes to data reassembly and MPTCP-level
> acknowledgements, and will delay movement of data to the msk until the
> checksum is verified. Also review my comments on the MP_FAIL RFC patch:

I need some help here.

How can I get the reassembled data for the current mapping? And in which
function to calculate the skbs checksum and where to verify them? Could
you please give me more detail?

Thanks.

-Geliang

>
> https://lists.01.org/hyperkitty/list/mptcp@lists.01.org/message/KSEEB5Q4ND3GKOW7S3FZ32PHCWWON5QC/
>
>
> Thanks for the patch set!
>
> Mat
>
>
> > +     csum = csum_partial(&header, sizeof(header), csum);
> > +
> > +     if (csum_fold(csum)) {
> > +             pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
> > +                    header.data_seq, header.subflow_seq, header.data_len,
> > +                    header.csum, csum_fold(csum));
> > +
> > +             return false;
> > +     }
> > +
> > +out:
> > +     return true;
> > +}
> > +
> > static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
> > {
> >       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> >       u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
> > +     struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >
> >       if (unlikely(before(ssn, subflow->map_subflow_seq))) {
> >               /* Mapping covers data later in the subflow stream,
> > @@ -817,6 +852,8 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
> >               warn_bad_map(subflow, ssn + skb->len);
> >               return false;
> >       }
> > +     if (READ_ONCE(msk->csum_reqd))
> > +             validate_dss_csum(ssk, skb);
> >       return true;
> > }
> >
> > --
> > 2.30.2
>
> --
> Mat Martineau
> Intel

Re: [MPTCP][PATCH mptcp-next 5/6] mptcp: add the DSS checksum receiving

[Mat Martineau]

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

On Tue, 23 Mar 2021, Geliang Tang wrote:

> Hi Mat,
>
> Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年3月23日周二 上午7:40写道：
>>
>> On Mon, 22 Mar 2021, Geliang Tang wrote:
>>
>>> This patch added a new member csum in struct mptcp_options_received. In
>>> mptcp_parse_option, parse the checksum value from the receiving DSS, and
>>> save it in mp_opt->csum. In mptcp_incoming_options, pass it to
>>> mpext->csum.
>>>
>>> In validate_mapping, use the new function validate_dss_csum to validata
>>> the DSS checksum.
>>>
>>> In validate_dss_csum, if the data has been split in different packets,
>>> skip validating.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>> net/mptcp/options.c  | 13 +++++++++++--
>>> net/mptcp/protocol.h |  1 +
>>> net/mptcp/subflow.c  | 37 +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 49 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 69eb15ef9385..ed89f6c2ed49 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -206,9 +206,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>>                       mp_opt->data_len = get_unaligned_be16(ptr);
>>>                       ptr += 2;
>>>
>>> -                     pr_debug("data_seq=%llu subflow_seq=%u data_len=%u",
>>> +                     if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
>>
>> Now that more of the checksum code has been added, I think that
>> expected_opsize should be set to the exact value that is needed - either
>> expect the checksum, or don't expect the checksum.
>>
>> RFC 6824 said: "If a checksum is present, but its use had not been
>> negotiated in the MP_CAPABLE handshake, the checksum field MUST be
>> ignored."
>>
>> RFC 8684 says: "If a checksum is present but its use had
>> not been negotiated in the MP_CAPABLE handshake, the receiver MUST
>> close the subflow with a RST, as it is not behaving as negotiated.
>> 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."
>>
>> There's some old RFC 6824-based code earlier in the function:
>>
>>                 if (opsize != expected_opsize &&
>>                     opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
>>                         break;
>>
>> That needs to be changed to:
>>
>>                 if (opsize != expected_opsize)
>>                         break;
>>
>> and expected_opsize should include TCPOLEN_MPTCP_DSS_CHECKSUM only if the
>> checksum is enabled.
>>
>>> +                             mp_opt->csum = get_unaligned_be16(ptr);
>>> +                             ptr += 2;
>>> +                     }
>>> +
>>> +                     pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
>>>                                mp_opt->data_seq, mp_opt->subflow_seq,
>>> -                              mp_opt->data_len);
>>> +                              mp_opt->data_len, mp_opt->csum);
>>>               }
>>>
>>>               break;
>>> @@ -339,6 +344,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>>>       mp_opt->mp_prio = 0;
>>>       mp_opt->reset = 0;
>>>       mp_opt->csum_reqd = 0;
>>> +     mp_opt->csum = 0;
>>>
>>>       length = (th->doff * 4) - sizeof(struct tcphdr);
>>>       ptr = (const unsigned char *)(th + 1);
>>> @@ -1124,6 +1130,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>>               }
>>>               mpext->data_len = mp_opt.data_len;
>>>               mpext->use_map = 1;
>>> +
>>> +             if (READ_ONCE(msk->csum_reqd))
>>> +                     mpext->csum = mp_opt.csum;
>>>       }
>>> }
>>>
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index f62cea8635f0..51ef3173a2e5 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -123,6 +123,7 @@ struct mptcp_options_received {
>>>       u64     data_seq;
>>>       u32     subflow_seq;
>>>       u16     data_len;
>>> +     u16     csum;
>>>       u16     mp_capable : 1,
>>>               mp_join : 1,
>>>               fastclose : 1,
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 9fec1a273100..2eeea7d527f0 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -799,10 +799,45 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
>>>                                         mptcp_subflow_get_map_offset(subflow);
>>> }
>>>
>>> +static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
>>> +{
>>> +     struct csum_pseudo_header header;
>>> +     struct mptcp_ext *mpext;
>>> +     __wsum csum;
>>> +
>>> +     if (!skb)
>>> +             goto out;
>>> +
>>> +     mpext = mptcp_get_ext(skb);
>>> +     if (!mpext || !mpext->use_map || !mpext->csum ||
>>> +         mpext->data_len != skb->len)
>>> +             goto out;
>>> +
>>> +     header.data_seq = mpext->data_seq;
>>> +     header.subflow_seq = mpext->subflow_seq;
>>> +     header.data_len = mpext->data_len;
>>> +     header.csum = mpext->csum;
>>> +
>>> +     csum = skb_checksum(skb, 0, skb->len, 0);
>>
>> The checksum isn't calculated using the data from a single skb, it
>> requires the complete reassembled data for the current mapping. If we're
>> lucky, GRO has placed everything in one skb. But that can't be assumed.
>>
>> The checksum can be calculated incrementally as skbs arrive, but checksum
>> verification will require changes to data reassembly and MPTCP-level
>> acknowledgements, and will delay movement of data to the msk until the
>> checksum is verified. Also review my comments on the MP_FAIL RFC patch:
>
> I need some help here.
>
> How can I get the reassembled data for the current mapping? And in which
> function to calculate the skbs checksum and where to verify them? Could
> you please give me more detail?
>

Look at __mptcp_mvoe_skbs_from_subflow() - the skbs will arrive in 
ssk->sk_receive_queue. The way the rx code works now, it is fairly 
aggressive about moving data to the msk as quickly as possible.

There will need to be a check to see if checksums are enabled, delaying 
the call to __mptcp_move_skb() until enough data is available in 
ssk->sk_receive_queue for the full mapping (ssk->map_data_len), then 
validating the checksum for the pseudoheader and the full mapping, and 
finally moving the skbs covered by that mapping.

The data covered by the checksum might not align with the skbs, so the 
code will have to make sure it is only using the data with sequence 
numbers in the range required by the mapping.

Delaying __mptcp_move_skb() will also delay changes to msk->ack_seq, which 
is a good thing: that will mean the data is not acked until the checksum 
is validated.


Does that give a good place to start? The exact place to calculate the 
checksum isn't immediately obvious - might be up the call stack from where 
__mptcp_move_skb() is called.

Mat


>
>>
>> https://lists.01.org/hyperkitty/list/mptcp@lists.01.org/message/KSEEB5Q4ND3GKOW7S3FZ32PHCWWON5QC/
>>
>>
>> Thanks for the patch set!
>>
>> Mat
>>
>>
>>> +     csum = csum_partial(&header, sizeof(header), csum);
>>> +
>>> +     if (csum_fold(csum)) {
>>> +             pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
>>> +                    header.data_seq, header.subflow_seq, header.data_len,
>>> +                    header.csum, csum_fold(csum));
>>> +
>>> +             return false;
>>> +     }
>>> +
>>> +out:
>>> +     return true;
>>> +}
>>> +
>>> static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
>>> {
>>>       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>>>       u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
>>> +     struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>
>>>       if (unlikely(before(ssn, subflow->map_subflow_seq))) {
>>>               /* Mapping covers data later in the subflow stream,
>>> @@ -817,6 +852,8 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
>>>               warn_bad_map(subflow, ssn + skb->len);
>>>               return false;
>>>       }
>>> +     if (READ_ONCE(msk->csum_reqd))
>>> +             validate_dss_csum(ssk, skb);
>>>       return true;
>>> }
>>>
>>> --
>>> 2.30.2
>>
>> --
>> Mat Martineau
>> Intel
>

--
Mat Martineau
Intel

Re: [MPTCP] [MPTCP][PATCH mptcp-next 5/6] mptcp: add the DSS checksum receiving

[Paolo Abeni]

On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
> This patch added a new member csum in struct mptcp_options_received. In
> mptcp_parse_option, parse the checksum value from the receiving DSS, and
> save it in mp_opt->csum. In mptcp_incoming_options, pass it to
> mpext->csum.
> 
> In validate_mapping, use the new function validate_dss_csum to validata
> the DSS checksum.
> 
> In validate_dss_csum, if the data has been split in different packets,
> skip validating.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/options.c  | 13 +++++++++++--
>  net/mptcp/protocol.h |  1 +
>  net/mptcp/subflow.c  | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 69eb15ef9385..ed89f6c2ed49 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -206,9 +206,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  			mp_opt->data_len = get_unaligned_be16(ptr);
>  			ptr += 2;
>  
> -			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u",
> +			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
> +				mp_opt->csum = get_unaligned_be16(ptr);
> +				ptr += 2;
> +			}
> +
> +			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%u",
>  				 mp_opt->data_seq, mp_opt->subflow_seq,
> -				 mp_opt->data_len);
> +				 mp_opt->data_len, mp_opt->csum);
>  		}
>  
>  		break;
> @@ -339,6 +344,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>  	mp_opt->mp_prio = 0;
>  	mp_opt->reset = 0;
>  	mp_opt->csum_reqd = 0;
> +	mp_opt->csum = 0;
>  
>  	length = (th->doff * 4) - sizeof(struct tcphdr);
>  	ptr = (const unsigned char *)(th + 1);
> @@ -1124,6 +1130,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>  		}
>  		mpext->data_len = mp_opt.data_len;
>  		mpext->use_map = 1;
> +
> +		if (READ_ONCE(msk->csum_reqd))
> +			mpext->csum = mp_opt.csum;
>  	}
>  }
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f62cea8635f0..51ef3173a2e5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -123,6 +123,7 @@ struct mptcp_options_received {
>  	u64	data_seq;
>  	u32	subflow_seq;
>  	u16	data_len;
> +	u16	csum;
>  	u16	mp_capable : 1,
>  		mp_join : 1,
>  		fastclose : 1,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 9fec1a273100..2eeea7d527f0 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -799,10 +799,45 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
>  					  mptcp_subflow_get_map_offset(subflow);
>  }
>  
> +static bool validate_dss_csum(struct sock *ssk, struct sk_buff *skb)
> +{
> +	struct csum_pseudo_header header;
> +	struct mptcp_ext *mpext;
> +	__wsum csum;
> +
> +	if (!skb)
> +		goto out;
> +
> +	mpext = mptcp_get_ext(skb);
> +	if (!mpext || !mpext->use_map || !mpext->csum ||
> +	    mpext->data_len != skb->len)
> +		goto out;
> +
> +	header.data_seq = mpext->data_seq;
> +	header.subflow_seq = mpext->subflow_seq;
> +	header.data_len = mpext->data_len;
> +	header.csum = mpext->csum;
> +
> +	csum = skb_checksum(skb, 0, skb->len, 0);
> +	csum = csum_partial(&header, sizeof(header), csum);
> +
> +	if (csum_fold(csum)) {
> +		pr_err("DSS checksum error! data_seq=%llu subflow_seq=%u data_len=%u csum=%u %u",
> +		       header.data_seq, header.subflow_seq, header.data_len,
> +		       header.csum, csum_fold(csum));

Here we likely need to increment a new mib counter, and avoid any
printk, otherwise an evil peer could easily DoS a server.

/P

Re: [MPTCP][PATCH mptcp-next 4/6] mptcp: add the DSS checksum sending

[Mat Martineau]

On Mon, 22 Mar 2021, Geliang Tang wrote:

> In mptcp_established_options_dss, use the new function
> mptcp_generate_dss_csum to generate the DSS checksum value.
>
> In mptcp_write_options, send out DSS with the checksum value.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> include/net/mptcp.h  |  1 +
> net/mptcp/options.c  | 41 ++++++++++++++++++++++++++++++++++++++---
> net/mptcp/protocol.h |  7 +++++++
> 3 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 36fb6907aa6f..869a6d98a72e 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -32,6 +32,7 @@ struct mptcp_ext {
> 			frozen:1,
> 			reset_transient:1;
> 	u8		reset_reason:4;
> +	u16		csum;
> };
>
> #define MPTCP_RM_IDS_MAX	8
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index bdced173edff..69eb15ef9385 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -521,6 +521,30 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> 	}
> }
>
> +static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
> +{
> +	struct csum_pseudo_header header;
> +	struct mptcp_ext *mpext;
> +	__wsum csum;
> +
> +	if (!skb)
> +		return 0;
> +
> +	mpext = mptcp_get_ext(skb);
> +	if (!mpext || !mpext->use_map)
> +		return 0;
> +
> +	header.data_seq = mpext->data_seq;
> +	header.subflow_seq = mpext->subflow_seq;
> +	header.data_len = mpext->data_len;
> +	header.csum = 0;
> +
> +	csum = skb_checksum(skb, 0, skb->len, 0);
> +	csum = csum_partial(&header, sizeof(header), csum);
> +
> +	return csum_fold(csum);
> +}
> +
> static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> 					  bool snd_data_fin_enable,
> 					  unsigned int *size,
> @@ -544,8 +568,11 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>
> 		remaining -= map_size;
> 		dss_size = map_size;
> -		if (mpext)
> +		if (mpext) {
> +			if (msk->csum_reqd)
> +				mpext->csum = mptcp_generate_dss_csum(skb);
> 			opts->ext_copy = *mpext;
> +		}
>
> 		if (skb && snd_data_fin_enable)
> 			mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> @@ -1304,6 +1331,9 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> 			if (mpext->data_fin)
> 				flags |= MPTCP_DSS_DATA_FIN;
> +
> +			if (mpext->csum)

Instead of relying on a non-zero mpext->csum, what about using 
opts->csum_enabled instead? (Would have to make sure that flag is set in 
mptcp_out_options for data packets too)

This would be more compatible with hardware offload if that gets added 
later.

> +				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> 		}
>
> 		*ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> @@ -1323,8 +1353,13 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			ptr += 2;
> 			put_unaligned_be32(mpext->subflow_seq, ptr);
> 			ptr += 1;
> -			put_unaligned_be32(mpext->data_len << 16 |
> -					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> +			if (mpext->csum) {

Same as above, want to test the same opts->csum_enabled value.

> +				put_unaligned_be32(mpext->data_len << 16 |
> +						   mpext->csum, ptr);
> +			} else {
> +				put_unaligned_be32(mpext->data_len << 16 |
> +						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> +			}
> 		}
> 	}
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 4fc4871595ef..f62cea8635f0 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -333,6 +333,13 @@ static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk)
> 	return list_first_entry_or_null(&msk->rtx_queue, struct mptcp_data_frag, list);
> }
>
> +struct csum_pseudo_header {
> +	u64 data_seq;
> +	u32 subflow_seq;
> +	u16 data_len;
> +	u16 csum;
> +};
> +
> struct mptcp_subflow_request_sock {
> 	struct	tcp_request_sock sk;
> 	u16	mp_capable : 1,
> -- 
> 2.30.2

--
Mat Martineau
Intel

Re: [MPTCP] Re: [MPTCP][PATCH mptcp-next 4/6] mptcp: add the DSS checksum sending

[Paolo Abeni]

On Mon, 2021-03-22 at 16:10 -0700, Mat Martineau wrote:
> On Mon, 22 Mar 2021, Geliang Tang wrote:
> 
> > In mptcp_established_options_dss, use the new function
> > mptcp_generate_dss_csum to generate the DSS checksum value.
> > 
> > In mptcp_write_options, send out DSS with the checksum value.
> > 
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > include/net/mptcp.h  |  1 +
> > net/mptcp/options.c  | 41 ++++++++++++++++++++++++++++++++++++++---
> > net/mptcp/protocol.h |  7 +++++++
> > 3 files changed, 46 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index 36fb6907aa6f..869a6d98a72e 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -32,6 +32,7 @@ struct mptcp_ext {
> > 			frozen:1,
> > 			reset_transient:1;
> > 	u8		reset_reason:4;
> > +	u16		csum;
> > };
> > 
> > #define MPTCP_RM_IDS_MAX	8
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index bdced173edff..69eb15ef9385 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -521,6 +521,30 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> > 	}
> > }
> > 
> > +static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
> > +{
> > +	struct csum_pseudo_header header;
> > +	struct mptcp_ext *mpext;
> > +	__wsum csum;
> > +
> > +	if (!skb)
> > +		return 0;
> > +
> > +	mpext = mptcp_get_ext(skb);
> > +	if (!mpext || !mpext->use_map)
> > +		return 0;
> > +
> > +	header.data_seq = mpext->data_seq;
> > +	header.subflow_seq = mpext->subflow_seq;
> > +	header.data_len = mpext->data_len;
> > +	header.csum = 0;
> > +
> > +	csum = skb_checksum(skb, 0, skb->len, 0);
> > +	csum = csum_partial(&header, sizeof(header), csum);
> > +
> > +	return csum_fold(csum);
> > +}
> > +
> > static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 					  bool snd_data_fin_enable,
> > 					  unsigned int *size,
> > @@ -544,8 +568,11 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 
> > 		remaining -= map_size;
> > 		dss_size = map_size;
> > -		if (mpext)
> > +		if (mpext) {
> > +			if (msk->csum_reqd)
> > +				mpext->csum = mptcp_generate_dss_csum(skb);
> > 			opts->ext_copy = *mpext;
> > +		}
> > 
> > 		if (skb && snd_data_fin_enable)
> > 			mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> > @@ -1304,6 +1331,9 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > 			flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> > 			if (mpext->data_fin)
> > 				flags |= MPTCP_DSS_DATA_FIN;
> > +
> > +			if (mpext->csum)
> 
> Instead of relying on a non-zero mpext->csum, what about using 
> opts->csum_enabled instead? (Would have to make sure that flag is set in 
> mptcp_out_options for data packets too)
> 
> This would be more compatible with hardware offload if that gets added 
> later.

Side note, not sure if already discussed in the last mtgs, xmit csum
offload looks doable, togethar with TSO !?! The nic have all the pieces
in place/available to compute it...

/P

Re: [MPTCP] Re: [MPTCP][PATCH mptcp-next 4/6] mptcp: add the DSS checksum sending

[Mat Martineau]

On Tue, 23 Mar 2021, Paolo Abeni wrote:

> On Mon, 2021-03-22 at 16:10 -0700, Mat Martineau wrote:
>> On Mon, 22 Mar 2021, Geliang Tang wrote:
>>
>>> In mptcp_established_options_dss, use the new function
>>> mptcp_generate_dss_csum to generate the DSS checksum value.
>>>
>>> In mptcp_write_options, send out DSS with the checksum value.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>> include/net/mptcp.h  |  1 +
>>> net/mptcp/options.c  | 41 ++++++++++++++++++++++++++++++++++++++---
>>> net/mptcp/protocol.h |  7 +++++++
>>> 3 files changed, 46 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>> index 36fb6907aa6f..869a6d98a72e 100644
>>> --- a/include/net/mptcp.h
>>> +++ b/include/net/mptcp.h
>>> @@ -32,6 +32,7 @@ struct mptcp_ext {
>>> 			frozen:1,
>>> 			reset_transient:1;
>>> 	u8		reset_reason:4;
>>> +	u16		csum;
>>> };
>>>
>>> #define MPTCP_RM_IDS_MAX	8
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index bdced173edff..69eb15ef9385 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -521,6 +521,30 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
>>> 	}
>>> }
>>>
>>> +static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
>>> +{
>>> +	struct csum_pseudo_header header;
>>> +	struct mptcp_ext *mpext;
>>> +	__wsum csum;
>>> +
>>> +	if (!skb)
>>> +		return 0;
>>> +
>>> +	mpext = mptcp_get_ext(skb);
>>> +	if (!mpext || !mpext->use_map)
>>> +		return 0;
>>> +
>>> +	header.data_seq = mpext->data_seq;
>>> +	header.subflow_seq = mpext->subflow_seq;
>>> +	header.data_len = mpext->data_len;
>>> +	header.csum = 0;
>>> +
>>> +	csum = skb_checksum(skb, 0, skb->len, 0);
>>> +	csum = csum_partial(&header, sizeof(header), csum);
>>> +
>>> +	return csum_fold(csum);
>>> +}
>>> +
>>> static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>>> 					  bool snd_data_fin_enable,
>>> 					  unsigned int *size,
>>> @@ -544,8 +568,11 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>>>
>>> 		remaining -= map_size;
>>> 		dss_size = map_size;
>>> -		if (mpext)
>>> +		if (mpext) {
>>> +			if (msk->csum_reqd)
>>> +				mpext->csum = mptcp_generate_dss_csum(skb);
>>> 			opts->ext_copy = *mpext;
>>> +		}
>>>
>>> 		if (skb && snd_data_fin_enable)
>>> 			mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
>>> @@ -1304,6 +1331,9 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>> 			flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
>>> 			if (mpext->data_fin)
>>> 				flags |= MPTCP_DSS_DATA_FIN;
>>> +
>>> +			if (mpext->csum)
>>
>> Instead of relying on a non-zero mpext->csum, what about using
>> opts->csum_enabled instead? (Would have to make sure that flag is set in
>> mptcp_out_options for data packets too)
>>
>> This would be more compatible with hardware offload if that gets added
>> later.
>
> Side note, not sure if already discussed in the last mtgs, xmit csum
> offload looks doable, togethar with TSO !?! The nic have all the pieces
> in place/available to compute it...
>

Yes, I did talk about this with one of the NIC maintainers and the tx side 
seemed doable. See notes near the end of 
https://lore.kernel.org/mptcp/e308928e-872d-a7b3-2b0b-751572915de2@tessares.net/

--
Mat Martineau
Intel

Re: [MPTCP] [MPTCP][PATCH mptcp-next 4/6] mptcp: add the DSS checksum sending

[Paolo Abeni]

On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
> In mptcp_established_options_dss, use the new function
> mptcp_generate_dss_csum to generate the DSS checksum value.
> 
> In mptcp_write_options, send out DSS with the checksum value.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  include/net/mptcp.h  |  1 +
>  net/mptcp/options.c  | 41 ++++++++++++++++++++++++++++++++++++++---
>  net/mptcp/protocol.h |  7 +++++++
>  3 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 36fb6907aa6f..869a6d98a72e 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -32,6 +32,7 @@ struct mptcp_ext {
>  			frozen:1,
>  			reset_transient:1;
>  	u8		reset_reason:4;
> +	u16		csum;
>  };
>  
>  #define MPTCP_RM_IDS_MAX	8
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index bdced173edff..69eb15ef9385 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -521,6 +521,30 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
>  	}
>  }
>  
> +static u16 mptcp_generate_dss_csum(struct sk_buff *skb)
> +{
> +	struct csum_pseudo_header header;
> +	struct mptcp_ext *mpext;
> +	__wsum csum;
> +
> +	if (!skb)
> +		return 0;
> +
> +	mpext = mptcp_get_ext(skb);
> +	if (!mpext || !mpext->use_map)
> +		return 0;
> +
> +	header.data_seq = mpext->data_seq;
> +	header.subflow_seq = mpext->subflow_seq;
> +	header.data_len = mpext->data_len;
> +	header.csum = 0;
> +
> +	csum = skb_checksum(skb, 0, skb->len, 0);
> +	csum = csum_partial(&header, sizeof(header), csum);
> +
> +	return csum_fold(csum);
> +}
> +
>  static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>  					  bool snd_data_fin_enable,
>  					  unsigned int *size,
> @@ -544,8 +568,11 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>  
>  		remaining -= map_size;
>  		dss_size = map_size;
> -		if (mpext)
> +		if (mpext) {
> +			if (msk->csum_reqd)
> +				mpext->csum = mptcp_generate_dss_csum(skb);
>  			opts->ext_copy = *mpext;
> +		}
>  
>  		if (skb && snd_data_fin_enable)
>  			mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> @@ -1304,6 +1331,9 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  			flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
>  			if (mpext->data_fin)
>  				flags |= MPTCP_DSS_DATA_FIN;
> +
> +			if (mpext->csum)
> +				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
>  		}
>  
>  		*ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);

If csum is enabled, we additionally need to insert the csum even in the
MPC+data packets.

In a similar way, we need to extract the csum value from MPC+data
packets in the input path, when csum is enabled (in the next patch)

/P

Re: [MPTCP][PATCH mptcp-next 3/6] mptcp: add the csum_reqd fields

[Mat Martineau]

On Mon, 22 Mar 2021, Geliang Tang wrote:

> This patch added a new flag csum_reqd in struct mptcp_options_received, if
> the flag MPTCP_CAP_CHECKSUM_REQD is set in the receiving MP_CAPABLE
> suboption, set this csum_reqd flag.
>
> It also added another new csum_reqd member in struct mptcp_sock too, if the
> csum_reqd flag in mptcp_options_received is set, set this member to true.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/options.c  | 8 +++++---
> net/mptcp/protocol.c | 1 +
> net/mptcp/protocol.h | 2 ++
> net/mptcp/subflow.c  | 4 ++++
> 4 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index f45bebb923dc..bdced173edff 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -69,11 +69,9 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> 		 * "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 (flags & MPTCP_CAP_CHECKSUM_REQD)
> -			break;
> +			mp_opt->csum_reqd = 1;
>
> 		mp_opt->mp_capable = 1;
> 		if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
> @@ -340,6 +338,7 @@ void mptcp_get_options(const struct sk_buff *skb,
> 	mp_opt->dss = 0;
> 	mp_opt->mp_prio = 0;
> 	mp_opt->reset = 0;
> +	mp_opt->csum_reqd = 0;
>
> 	length = (th->doff * 4) - sizeof(struct tcphdr);
> 	ptr = (const unsigned char *)(th + 1);
> @@ -1013,6 +1012,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> 		mptcp_schedule_work((struct sock *)msk);
> 	}
>
> +	if (mp_opt.csum_reqd)
> +		WRITE_ONCE(msk->csum_reqd, true);
> +

Is this assignment needed here? It looks like setting msk->csum_reqd in 
subflow_finish_connect() and subflow_syn_recv_sock() should cover both 
outgoing and incoming connections.

Thanks,

Mat

> 	if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
> 		if (!mp_opt.echo) {
> 			mptcp_pm_add_addr_received(msk, &mp_opt.addr);
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 9d7e7e13fba8..94c3568c7a60 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2364,6 +2364,7 @@ static int __mptcp_init_sock(struct sock *sk)
> 	msk->ack_hint = NULL;
> 	msk->first = NULL;
> 	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
> +	WRITE_ONCE(msk->csum_reqd, mptcp_is_checksum_enabled(sock_net(sk)));
>
> 	mptcp_pm_data_init(msk);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 14f1e2fd3c08..4fc4871595ef 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -132,6 +132,7 @@ struct mptcp_options_received {
> 		rm_addr : 1,
> 		mp_prio : 1,
> 		echo : 1,
> +		csum_reqd : 1,
> 		backup : 1;
> 	u32	token;
> 	u32	nonce;
> @@ -233,6 +234,7 @@ struct mptcp_sock {
> 	bool		snd_data_fin_enable;
> 	bool		rcv_fastclose;
> 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
> +	bool		csum_reqd;
> 	spinlock_t	join_list_lock;
> 	struct sock	*ack_hint;
> 	struct work_struct work;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index e2e9cc34713d..9fec1a273100 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -403,6 +403,8 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> 			goto fallback;
> 		}
>
> +		if (mp_opt.csum_reqd)
> +			WRITE_ONCE(mptcp_sk(parent)->csum_reqd, true);
> 		subflow->mp_capable = 1;
> 		subflow->can_ack = 1;
> 		subflow->remote_key = mp_opt.sndr_key;
> @@ -643,6 +645,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
> 		if (!new_msk)
> 			fallback = true;
> +		else if (mp_opt.csum_reqd)
> +			WRITE_ONCE(mptcp_sk(new_msk)->csum_reqd, true);
> 	} else if (subflow_req->mp_join) {
> 		mptcp_get_options(skb, &mp_opt);
> 		if (!mp_opt.mp_join || !subflow_hmac_valid(req, &mp_opt) ||
> -- 
> 2.30.2

--
Mat Martineau
Intel

Re: [MPTCP] [MPTCP][PATCH mptcp-next 3/6] mptcp: add the csum_reqd fields

[Paolo Abeni]

On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
> This patch added a new flag csum_reqd in struct mptcp_options_received, if
> the flag MPTCP_CAP_CHECKSUM_REQD is set in the receiving MP_CAPABLE
> suboption, set this csum_reqd flag.
> 
> It also added another new csum_reqd member in struct mptcp_sock too, if the
> csum_reqd flag in mptcp_options_received is set, set this member to true.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/options.c  | 8 +++++---
>  net/mptcp/protocol.c | 1 +
>  net/mptcp/protocol.h | 2 ++
>  net/mptcp/subflow.c  | 4 ++++
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index f45bebb923dc..bdced173edff 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -69,11 +69,9 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		 * "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 (flags & MPTCP_CAP_CHECKSUM_REQD)
> -			break;
> +			mp_opt->csum_reqd = 1;
>  
>  		mp_opt->mp_capable = 1;
>  		if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
> @@ -340,6 +338,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>  	mp_opt->dss = 0;
>  	mp_opt->mp_prio = 0;
>  	mp_opt->reset = 0;
> +	mp_opt->csum_reqd = 0;
>  
>  	length = (th->doff * 4) - sizeof(struct tcphdr);
>  	ptr = (const unsigned char *)(th + 1);
> @@ -1013,6 +1012,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>  		mptcp_schedule_work((struct sock *)msk);
>  	}
>  
> +	if (mp_opt.csum_reqd)
> +		WRITE_ONCE(msk->csum_reqd, true);
> +
>  	if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
>  		if (!mp_opt.echo) {
>  			mptcp_pm_add_addr_received(msk, &mp_opt.addr);
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 9d7e7e13fba8..94c3568c7a60 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2364,6 +2364,7 @@ static int __mptcp_init_sock(struct sock *sk)
>  	msk->ack_hint = NULL;
>  	msk->first = NULL;
>  	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
> +	WRITE_ONCE(msk->csum_reqd, mptcp_is_checksum_enabled(sock_net(sk)));
>  
>  	mptcp_pm_data_init(msk);
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 14f1e2fd3c08..4fc4871595ef 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -132,6 +132,7 @@ struct mptcp_options_received {
>  		rm_addr : 1,
>  		mp_prio : 1,
>  		echo : 1,
> +		csum_reqd : 1,
>  		backup : 1;
>  	u32	token;
>  	u32	nonce;
> @@ -233,6 +234,7 @@ struct mptcp_sock {
>  	bool		snd_data_fin_enable;
>  	bool		rcv_fastclose;
>  	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
> +	bool		csum_reqd;
>  	spinlock_t	join_list_lock;
>  	struct sock	*ack_hint;
>  	struct work_struct work;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index e2e9cc34713d..9fec1a273100 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -403,6 +403,8 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  			goto fallback;
>  		}
>  
> +		if (mp_opt.csum_reqd)
> +			WRITE_ONCE(mptcp_sk(parent)->csum_reqd, true);
>  		subflow->mp_capable = 1;
>  		subflow->can_ack = 1;
>  		subflow->remote_key = mp_opt.sndr_key;
> @@ -643,6 +645,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
>  		if (!new_msk)
>  			fallback = true;
> +		else if (mp_opt.csum_reqd)
> +			WRITE_ONCE(mptcp_sk(new_msk)->csum_reqd, true);

I think this chunk would be better located inside mptcp_sk_clone(). 

@Mat, @Florian, slighly related: if we do the assignment there, the
WRITE_ONCE will not be required, right? - since that functions runs
under msk lock. We already have a few WRITE_ONCE there which are likely
not really needed...

/P

Re: [MPTCP] [MPTCP][PATCH mptcp-next 3/6] mptcp: add the csum_reqd fields

[Mat Martineau]

On Tue, 23 Mar 2021, Paolo Abeni wrote:

> On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
>> This patch added a new flag csum_reqd in struct mptcp_options_received, if
>> the flag MPTCP_CAP_CHECKSUM_REQD is set in the receiving MP_CAPABLE
>> suboption, set this csum_reqd flag.
>>
>> It also added another new csum_reqd member in struct mptcp_sock too, if the
>> csum_reqd flag in mptcp_options_received is set, set this member to true.
>>
>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> ---
>>  net/mptcp/options.c  | 8 +++++---
>>  net/mptcp/protocol.c | 1 +
>>  net/mptcp/protocol.h | 2 ++
>>  net/mptcp/subflow.c  | 4 ++++
>>  4 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index f45bebb923dc..bdced173edff 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -69,11 +69,9 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		 * "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 (flags & MPTCP_CAP_CHECKSUM_REQD)
>> -			break;
>> +			mp_opt->csum_reqd = 1;
>>
>>  		mp_opt->mp_capable = 1;
>>  		if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
>> @@ -340,6 +338,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>>  	mp_opt->dss = 0;
>>  	mp_opt->mp_prio = 0;
>>  	mp_opt->reset = 0;
>> +	mp_opt->csum_reqd = 0;
>>
>>  	length = (th->doff * 4) - sizeof(struct tcphdr);
>>  	ptr = (const unsigned char *)(th + 1);
>> @@ -1013,6 +1012,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>  		mptcp_schedule_work((struct sock *)msk);
>>  	}
>>
>> +	if (mp_opt.csum_reqd)
>> +		WRITE_ONCE(msk->csum_reqd, true);
>> +
>>  	if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
>>  		if (!mp_opt.echo) {
>>  			mptcp_pm_add_addr_received(msk, &mp_opt.addr);
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 9d7e7e13fba8..94c3568c7a60 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2364,6 +2364,7 @@ static int __mptcp_init_sock(struct sock *sk)
>>  	msk->ack_hint = NULL;
>>  	msk->first = NULL;
>>  	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
>> +	WRITE_ONCE(msk->csum_reqd, mptcp_is_checksum_enabled(sock_net(sk)));
>>
>>  	mptcp_pm_data_init(msk);
>>
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 14f1e2fd3c08..4fc4871595ef 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -132,6 +132,7 @@ struct mptcp_options_received {
>>  		rm_addr : 1,
>>  		mp_prio : 1,
>>  		echo : 1,
>> +		csum_reqd : 1,
>>  		backup : 1;
>>  	u32	token;
>>  	u32	nonce;
>> @@ -233,6 +234,7 @@ struct mptcp_sock {
>>  	bool		snd_data_fin_enable;
>>  	bool		rcv_fastclose;
>>  	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
>> +	bool		csum_reqd;
>>  	spinlock_t	join_list_lock;
>>  	struct sock	*ack_hint;
>>  	struct work_struct work;
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index e2e9cc34713d..9fec1a273100 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -403,6 +403,8 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>>  			goto fallback;
>>  		}
>>
>> +		if (mp_opt.csum_reqd)
>> +			WRITE_ONCE(mptcp_sk(parent)->csum_reqd, true);
>>  		subflow->mp_capable = 1;
>>  		subflow->can_ack = 1;
>>  		subflow->remote_key = mp_opt.sndr_key;
>> @@ -643,6 +645,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>>  		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
>>  		if (!new_msk)
>>  			fallback = true;
>> +		else if (mp_opt.csum_reqd)
>> +			WRITE_ONCE(mptcp_sk(new_msk)->csum_reqd, true);
>
> I think this chunk would be better located inside mptcp_sk_clone().
>
> @Mat, @Florian, slighly related: if we do the assignment there, the
> WRITE_ONCE will not be required, right? - since that functions runs
> under msk lock. We already have a few WRITE_ONCE there which are likely
> not really needed...

Yeah, I think it would fit well in mptcp_sk_clone(). Given that csum_reqd 
doesn't change after the connection starts I'm not sure 
WRITE_ONCE/READ_ONCE are really needed for the value, and I think the need 
for WRITE_ONCE is determined by the lockless readers rather than whether 
the writer is under msk lock (there's a recent LWN article I really should 
read to confirm my understanding).

--
Mat Martineau
Intel

Re: [MPTCP] [MPTCP][PATCH mptcp-next 2/6] mptcp: add csum_enabled in mptcp_out_options

[Paolo Abeni]

On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
> This patch add a new member csum_enabled in struct mptcp_out_options.
> In mptcp_write_options, if this csum_enabled field is enabled, send out
> the MP_CAPABLE suboption with the MPTCP_CAP_CHECKSUM_REQD flag.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  include/net/mptcp.h  |  3 ++-
>  net/mptcp/options.c  | 10 ++++++++--
>  net/mptcp/protocol.h |  3 ++-
>  net/mptcp/subflow.c  |  1 +
>  4 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 83f23774b908..36fb6907aa6f 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -64,7 +64,8 @@ struct mptcp_out_options {
>  	u8 join_id;
>  	u8 backup;
>  	u8 reset_reason:4;
> -	u8 reset_transient:1;
> +	u8 reset_transient:1,
> +	   csum_enabled:1;
>  	u32 nonce;
>  	u64 thmac;
>  	u32 token;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index e3fcd2b0ffd7..f45bebb923dc 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -379,6 +379,7 @@ bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
>  	subflow->snd_isn = TCP_SKB_CB(skb)->end_seq;
>  	if (subflow->request_mptcp) {
>  		opts->suboptions = OPTION_MPTCP_MPC_SYN;
> +		opts->csum_enabled = mptcp_is_checksum_enabled(sock_net(sk));
>  		*size = TCPOLEN_MPTCP_MPC_SYN;
>  		return true;
>  	} else if (subflow->request_join) {
> @@ -464,6 +465,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
>  		opts->suboptions = OPTION_MPTCP_MPC_ACK;
>  		opts->sndr_key = subflow->local_key;
>  		opts->rcvr_key = subflow->remote_key;
> +		opts->csum_enabled = mptcp_is_checksum_enabled(sock_net(sk));

Note that 'mptcp_is_checksum_enabled(sock_net(sk))' can change its
value between SYN and 3rd ack, depending on user-space actions.

I think it would be better 'cache' the
mptcp_is_checksum_enabled(sock_net(sk)) value inside some new msk field
at msk creation time (for outgoing socket), so that this will always be
consistent and we could expose the 'csum requested' info to user-space
via diag in a reliable way.

For incoming socket, I think we could copy the 'subflow_req-
>csum_enabled' into the msk.

Cheers,

Paolo

Re: [MPTCP][PATCH mptcp-next 1/6] mptcp: add a new sysctl checksum_enabled

[Mat Martineau]

On Mon, 22 Mar 2021, Geliang Tang wrote:

> This patch added a new sysctl, named checksum_enabled, to control
> whether DSS checksum can be enabled.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> Documentation/networking/mptcp-sysctl.rst |  8 ++++++++
> net/mptcp/ctrl.c                          | 14 ++++++++++++++
> net/mptcp/protocol.h                      |  1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
> index 6af0196c4297..1128e09d6c4d 100644
> --- a/Documentation/networking/mptcp-sysctl.rst
> +++ b/Documentation/networking/mptcp-sysctl.rst
> @@ -24,3 +24,11 @@ add_addr_timeout - INTEGER (seconds)
> 	sysctl.
>
> 	Default: 120
> +
> +checksum_enabled - INTEGER
> +	Control whether DSS checksum can be enabled.
> +
> +	DSS checksum can be enabled if the value is nonzero. This is a
> +	per-namespace sysctl.
> +
> +	Default: 1

In my opinion, this should default to 0 for compatibility with previous 
upstream kernel versions and because it sounds like checksums aren't used 
as much in practice. But I'm not totally committed to that - what do those 
of you with more MPTCP deployment experience think?

Mat


> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 96ba616f59bf..115cebc8c02a 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -19,6 +19,7 @@ struct mptcp_pernet {
>
> 	int mptcp_enabled;
> 	unsigned int add_addr_timeout;
> +	int checksum_enabled;
> };
>
> static struct mptcp_pernet *mptcp_get_pernet(struct net *net)
> @@ -36,6 +37,11 @@ unsigned int mptcp_get_add_addr_timeout(struct net *net)
> 	return mptcp_get_pernet(net)->add_addr_timeout;
> }
>
> +int mptcp_is_checksum_enabled(struct net *net)
> +{
> +	return mptcp_get_pernet(net)->checksum_enabled;
> +}
> +
> static struct ctl_table mptcp_sysctl_table[] = {
> 	{
> 		.procname = "enabled",
> @@ -52,6 +58,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
> 		.mode = 0644,
> 		.proc_handler = proc_dointvec_jiffies,
> 	},
> +	{
> +		.procname = "checksum_enabled",
> +		.maxlen = sizeof(int),
> +		.mode = 0644,
> +		.proc_handler = proc_dointvec,
> +	},
> 	{}
> };
>
> @@ -59,6 +71,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
> {
> 	pernet->mptcp_enabled = 1;
> 	pernet->add_addr_timeout = TCP_RTO_MAX;
> +	pernet->checksum_enabled = 1;
> }
>
> static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
> @@ -75,6 +88,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
>
> 	table[0].data = &pernet->mptcp_enabled;
> 	table[1].data = &pernet->add_addr_timeout;
> +	table[2].data = &pernet->checksum_enabled;
>
> 	hdr = register_net_sysctl(net, MPTCP_SYSCTL_PATH, table);
> 	if (!hdr)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 14f0114be17a..fd36239d8905 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -519,6 +519,7 @@ static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *su
>
> int mptcp_is_enabled(struct net *net);
> unsigned int mptcp_get_add_addr_timeout(struct net *net);
> +int mptcp_is_checksum_enabled(struct net *net);
> void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
> 				     struct mptcp_options_received *mp_opt);
> bool mptcp_subflow_data_available(struct sock *sk);
> -- 
> 2.30.2

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next 1/6] mptcp: add a new sysctl checksum_enabled

[Mat Martineau]

On Mon, 22 Mar 2021, Mat Martineau wrote:

>
> On Mon, 22 Mar 2021, Geliang Tang wrote:
>
>> This patch added a new sysctl, named checksum_enabled, to control
>> whether DSS checksum can be enabled.
>> 
>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> ---
>> Documentation/networking/mptcp-sysctl.rst |  8 ++++++++
>> net/mptcp/ctrl.c                          | 14 ++++++++++++++
>> net/mptcp/protocol.h                      |  1 +
>> 3 files changed, 23 insertions(+)
>> 
>> diff --git a/Documentation/networking/mptcp-sysctl.rst 
>> b/Documentation/networking/mptcp-sysctl.rst
>> index 6af0196c4297..1128e09d6c4d 100644
>> --- a/Documentation/networking/mptcp-sysctl.rst
>> +++ b/Documentation/networking/mptcp-sysctl.rst
>> @@ -24,3 +24,11 @@ add_addr_timeout - INTEGER (seconds)
>> 	sysctl.
>>
>> 	Default: 120
>> +
>> +checksum_enabled - INTEGER
>> +	Control whether DSS checksum can be enabled.
>> +
>> +	DSS checksum can be enabled if the value is nonzero. This is a
>> +	per-namespace sysctl.
>> +
>> +	Default: 1
>
> In my opinion, this should default to 0 for compatibility with previous 
> upstream kernel versions and because it sounds like checksums aren't used as 
> much in practice. But I'm not totally committed to that - what do those of 
> you with more MPTCP deployment experience think?
>
> Mat
>
>
>> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
>> index 96ba616f59bf..115cebc8c02a 100644
>> --- a/net/mptcp/ctrl.c
>> +++ b/net/mptcp/ctrl.c
>> @@ -19,6 +19,7 @@ struct mptcp_pernet {
>>
>> 	int mptcp_enabled;
>> 	unsigned int add_addr_timeout;
>> +	int checksum_enabled;
>> };
>> 
>> static struct mptcp_pernet *mptcp_get_pernet(struct net *net)
>> @@ -36,6 +37,11 @@ unsigned int mptcp_get_add_addr_timeout(struct net *net)
>> 	return mptcp_get_pernet(net)->add_addr_timeout;
>> }
>> 
>> +int mptcp_is_checksum_enabled(struct net *net)
>> +{
>> +	return mptcp_get_pernet(net)->checksum_enabled;
>> +}
>> +

One more thing, it would be good to have the checksum functionality forced 
off until all the code is in place. It would be more git-bisect-compatible 
if mptcp_is_checksum_enabled() always returns false and the incoming 
checksum flag is rejected until the last patch in the series.


Mat


>> static struct ctl_table mptcp_sysctl_table[] = {
>> 	{
>> 		.procname = "enabled",
>> @@ -52,6 +58,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
>> 		.mode = 0644,
>> 		.proc_handler = proc_dointvec_jiffies,
>> 	},
>> +	{
>> +		.procname = "checksum_enabled",
>> +		.maxlen = sizeof(int),
>> +		.mode = 0644,
>> +		.proc_handler = proc_dointvec,
>> +	},
>> 	{}
>> };
>> 
>> @@ -59,6 +71,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet 
>> *pernet)
>> {
>> 	pernet->mptcp_enabled = 1;
>> 	pernet->add_addr_timeout = TCP_RTO_MAX;
>> +	pernet->checksum_enabled = 1;
>> }
>> 
>> static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet 
>> *pernet)
>> @@ -75,6 +88,7 @@ static int mptcp_pernet_new_table(struct net *net, struct 
>> mptcp_pernet *pernet)
>>
>> 	table[0].data = &pernet->mptcp_enabled;
>> 	table[1].data = &pernet->add_addr_timeout;
>> +	table[2].data = &pernet->checksum_enabled;
>>
>> 	hdr = register_net_sysctl(net, MPTCP_SYSCTL_PATH, table);
>> 	if (!hdr)
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 14f0114be17a..fd36239d8905 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -519,6 +519,7 @@ static inline void mptcp_subflow_delegated_done(struct 
>> mptcp_subflow_context *su
>> 
>> int mptcp_is_enabled(struct net *net);
>> unsigned int mptcp_get_add_addr_timeout(struct net *net);
>> +int mptcp_is_checksum_enabled(struct net *net);
>> void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
>> 				     struct mptcp_options_received *mp_opt);
>> bool mptcp_subflow_data_available(struct sock *sk);
>> -- 
>> 2.30.2

--
Mat Martineau
Intel