[RFC PATCH mptcp-next 0/4] MP_JOIN fallback for TCP listeners

[RFC PATCH mptcp-next 0/4] MP_JOIN fallback for TCP listeners

[Mat Martineau]

In discussion of Florian's "replace per-addr listener sockets" series, I
proposed this:

    One possibility that doesn't disturb the hot path too much: Let the
    TCP listener try to handle the SYN. In tcp_parse_options(), add just
    enough code to set a bit in struct tcp_options_received if MP_JOIN
    is present (don't fully parse the MPTCP option). If MP_JOIN is there
    and MPTCP is enabled, return early from tcp_check_req() and have
    tcp_v4_rcv() treat it similar to req_stolen==true. Instead of
    kicking back to a regular lookup, try a token lookup.

It seemed to me that the hot path impact would be minimal. To better
show that, I put together these RFC patches. Only one conditional is
added to the hot path, checking a bit in struct tcp_received options
(which should be in the data cache anyway). The other hooks are:

 * tcp_parse_options() switch statement (only executed if a MPTCP option
   is present).

 * The tcp_check_req()-returned-NULL error paths for IPv4 and IPv6.

In addition, the added code in this series is almost entirely optimized
out if CONFIG_MPTCP is not set. With one more ifdef for the
tcp_parse_option() change, the optimized kernel binary may even be
entirely unchanged.

Compared to Florian's proposal (add token lookup before the listener
lookup) this does change more lines of TCP code, but has less runtime
impact.

Major caveat: this is very lightly tested, build and a quick check to
make sure it didn't break TCP.

This applies on top of "mptcp: replace per-addr listener sockets" v4.


Mat Martineau (4):
  mptcp: Move some symbols to be visible outside the MPTCP subsystem
  tcp: Allow tcp_check_req() to return more status values
  tcp: Check for the presence of a MP_JOIN option
  tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener

 include/linux/tcp.h      |  3 ++-
 include/net/mptcp.h      | 18 ++++++++++++++++++
 include/net/tcp.h        | 10 +++++++++-
 net/ipv4/tcp_input.c     | 12 ++++++++++--
 net/ipv4/tcp_ipv4.c      | 13 ++++++++++---
 net/ipv4/tcp_minisocks.c | 14 ++++++++++----
 net/ipv6/tcp_ipv6.c      | 13 ++++++++++---
 net/mptcp/protocol.h     | 12 ------------
 8 files changed, 69 insertions(+), 26 deletions(-)

-- 
2.35.1

[RFC PATCH mptcp-next 1/4] mptcp: Move some symbols to be visible outside the MPTCP subsystem

[Mat Martineau]

Later patches will be checking MPTCP enabled status and a TCP option,
so make those symbols available in TCP code.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 include/net/mptcp.h  | 18 ++++++++++++++++++
 net/mptcp/protocol.h | 12 ------------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index b8939d7ea12e..211f395d8139 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -16,6 +16,17 @@ struct mptcp_info;
 struct mptcp_sock;
 struct seq_file;
 
+/* MPTCP option subtypes */
+#define MPTCPOPT_MP_CAPABLE	0
+#define MPTCPOPT_MP_JOIN	1
+#define MPTCPOPT_DSS		2
+#define MPTCPOPT_ADD_ADDR	3
+#define MPTCPOPT_RM_ADDR	4
+#define MPTCPOPT_MP_PRIO	5
+#define MPTCPOPT_MP_FAIL	6
+#define MPTCPOPT_MP_FASTCLOSE	7
+#define MPTCPOPT_RST		8
+
 /* MPTCP sk_buff extension data */
 struct mptcp_ext {
 	union {
@@ -100,6 +111,8 @@ extern struct request_sock_ops mptcp_subflow_request_sock_ops;
 
 void mptcp_init(void);
 
+int mptcp_is_enabled(const struct net *net);
+
 static inline bool sk_is_mptcp(const struct sock *sk)
 {
 	return tcp_sk(sk)->is_mptcp;
@@ -219,6 +232,11 @@ static inline void mptcp_init(void)
 {
 }
 
+static inline int mptcp_is_enabled(const struct net *net)
+{
+	return 0;
+}
+
 static inline bool sk_is_mptcp(const struct sock *sk)
 {
 	return false;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7ec2513e1c2f..75c852f0dd3a 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -36,17 +36,6 @@
 #define OPTIONS_MPTCP_MPJ	(OPTION_MPTCP_MPJ_SYN | OPTION_MPTCP_MPJ_SYNACK | \
 				 OPTION_MPTCP_MPJ_ACK)
 
-/* MPTCP option subtypes */
-#define MPTCPOPT_MP_CAPABLE	0
-#define MPTCPOPT_MP_JOIN	1
-#define MPTCPOPT_DSS		2
-#define MPTCPOPT_ADD_ADDR	3
-#define MPTCPOPT_RM_ADDR	4
-#define MPTCPOPT_MP_PRIO	5
-#define MPTCPOPT_MP_FAIL	6
-#define MPTCPOPT_MP_FASTCLOSE	7
-#define MPTCPOPT_RST		8
-
 /* MPTCP suboption lengths */
 #define TCPOLEN_MPTCP_MPC_SYN		4
 #define TCPOLEN_MPTCP_MPC_SYNACK	12
@@ -577,7 +566,6 @@ static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *su
 	clear_bit(action, &subflow->delegated_status);
 }
 
-int mptcp_is_enabled(const struct net *net);
 unsigned int mptcp_get_add_addr_timeout(const struct net *net);
 int mptcp_is_checksum_enabled(const struct net *net);
 int mptcp_allow_join_id0(const struct net *net);
-- 
2.35.1

[RFC PATCH mptcp-next 2/4] tcp: Allow tcp_check_req() to return more status values

[Mat Martineau]

tcp_check_req() can already use an output parameter to set req_status,
indicating whether the current thread has lost a race to process the req.

Convert this bool pointer to instead use an enum, so tcp_check_req() can
look for and set status values indicating other situations. This is used
in a subsequent commit to check for a MPTCP MP_JOIN header. There is no
functional change intended in this patch.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 include/net/tcp.h        | 10 +++++++++-
 net/ipv4/tcp_input.c     |  4 ++--
 net/ipv4/tcp_ipv4.c      |  6 +++---
 net/ipv4/tcp_minisocks.c |  6 +++---
 net/ipv6/tcp_ipv6.c      |  6 +++---
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 04f4650e0ff0..0215acaec29f 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -382,9 +382,17 @@ enum tcp_tw_status {
 enum tcp_tw_status tcp_timewait_state_process(struct inet_timewait_sock *tw,
 					      struct sk_buff *skb,
 					      const struct tcphdr *th);
+
+enum tcp_req_status {
+	TCP_REQ_OK = 0,
+	TCP_REQ_LOST_RACE = 1,
+	TCP_REQ_MP_JOIN = 2
+};
+
 struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			   struct request_sock *req, bool fastopen,
-			   bool *lost_race);
+			   enum tcp_req_status *req_status);
+
 int tcp_child_process(struct sock *parent, struct sock *child,
 		      struct sk_buff *skb);
 void tcp_enter_loss(struct sock *sk);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2088f93fa37b..7eb02f9da3c9 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6457,12 +6457,12 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 	req = rcu_dereference_protected(tp->fastopen_rsk,
 					lockdep_sock_is_held(sk));
 	if (req) {
-		bool req_stolen;
+		enum tcp_req_status req_status;
 
 		WARN_ON_ONCE(sk->sk_state != TCP_SYN_RECV &&
 		    sk->sk_state != TCP_FIN_WAIT1);
 
-		if (!tcp_check_req(sk, skb, req, true, &req_stolen))
+		if (!tcp_check_req(sk, skb, req, true, &req_status))
 			goto discard;
 	}
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index feb779d1fd21..73e5726af8d0 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2030,8 +2030,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		goto do_time_wait;
 
 	if (sk->sk_state == TCP_NEW_SYN_RECV) {
+		enum tcp_req_status req_status = TCP_REQ_OK;
 		struct request_sock *req = inet_reqsk(sk);
-		bool req_stolen = false;
 		struct sock *nsk;
 
 		sk = req->rsk_listener;
@@ -2067,13 +2067,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
 			th = (const struct tcphdr *)skb->data;
 			iph = ip_hdr(skb);
 			tcp_v4_fill_cb(skb, iph, th);
-			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
+			nsk = tcp_check_req(sk, skb, req, false, &req_status);
 		} else {
 			drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
 		}
 		if (!nsk) {
 			reqsk_put(req);
-			if (req_stolen) {
+			if (req_status == TCP_REQ_LOST_RACE) {
 				/* Another cpu got exclusive access to req
 				 * and created a full blown socket.
 				 * Try to feed this packet to this socket
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 6366df7aaf2a..3d989f8676a5 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -562,7 +562,7 @@ EXPORT_SYMBOL(tcp_create_openreq_child);
 
 struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			   struct request_sock *req,
-			   bool fastopen, bool *req_stolen)
+			   bool fastopen, enum tcp_req_status *req_status)
 {
 	struct tcp_options_received tmp_opt;
 	struct sock *child;
@@ -774,7 +774,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 
 	sock_rps_save_rxhash(child, skb);
 	tcp_synack_rtt_meas(child, req);
-	*req_stolen = !own_req;
+	*req_status = own_req ? TCP_REQ_OK : TCP_REQ_LOST_RACE;
 	return inet_csk_complete_hashdance(sk, child, req, own_req);
 
 listen_overflow:
@@ -803,7 +803,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 
 		if (unlinked)
 			__NET_INC_STATS(sock_net(sk), LINUX_MIB_EMBRYONICRSTS);
-		*req_stolen = !unlinked;
+		*req_status = unlinked ? TCP_REQ_OK : TCP_REQ_LOST_RACE;
 	}
 	return NULL;
 }
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index b414e2f77fa3..4706aad6dd8f 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1682,8 +1682,8 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		goto do_time_wait;
 
 	if (sk->sk_state == TCP_NEW_SYN_RECV) {
+		enum tcp_req_status req_status = TCP_REQ_OK;
 		struct request_sock *req = inet_reqsk(sk);
-		bool req_stolen = false;
 		struct sock *nsk;
 
 		sk = req->rsk_listener;
@@ -1716,13 +1716,13 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 			th = (const struct tcphdr *)skb->data;
 			hdr = ipv6_hdr(skb);
 			tcp_v6_fill_cb(skb, hdr, th);
-			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
+			nsk = tcp_check_req(sk, skb, req, false, &req_status);
 		} else {
 			drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
 		}
 		if (!nsk) {
 			reqsk_put(req);
-			if (req_stolen) {
+			if (req_status == TCP_REQ_LOST_RACE) {
 				/* Another cpu got exclusive access to req
 				 * and created a full blown socket.
 				 * Try to feed this packet to this socket
-- 
2.35.1

[RFC PATCH mptcp-next 3/4] tcp: Check for the presence of a MP_JOIN option

[Mat Martineau]

Add a bit to struct tcp_options_received and set that bit if a
MPTCP option with the MP_JOIN subtype is present in a TCP header.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 include/linux/tcp.h  | 3 ++-
 net/ipv4/tcp_input.c | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 1168302b7927..46a14bce8019 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -93,7 +93,8 @@ struct tcp_options_received {
 		snd_wscale : 4,	/* Window scaling received from sender	*/
 		rcv_wscale : 4;	/* Window scaling to send to receiver	*/
 	u8	saw_unknown:1,	/* Received unknown option		*/
-		unused:7;
+		saw_mp_join:1,	/* Received MPTCP MP_JOIN		*/
+		unused:6;
 	u8	num_sacks;	/* Number of SACK blocks		*/
 	u16	user_mss;	/* mss requested by user in ioctl	*/
 	u16	mss_clamp;	/* Maximal mss, negotiated at connection setup */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7eb02f9da3c9..845f0e6906b5 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4122,6 +4122,14 @@ void tcp_parse_options(const struct net *net,
 				opt_rx->saw_unknown = 1;
 				break;
 
+			case TCPOPT_MPTCP:
+				__u8 mptcp_subtype = *ptr >> 4;
+
+				if (mptcp_subtype == MPTCPOPT_MP_JOIN)
+					opt_rx->saw_mp_join = 1;
+
+				break;
+
 			default:
 				opt_rx->saw_unknown = 1;
 			}
-- 
2.35.1

[RFC PATCH mptcp-next 4/4] tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener

[Mat Martineau]

MPTCP uses a TCP SYN packet with the MP_JOIN option to add subflows to
an existing MPTCP connection. This uses token lookup to match the
incoming SYN with the appropriate MPTCP socket instead of the port
number (in accordance with RFC 8684), and it is possible for a TCP
listener to be present at the port that the MPTCP peer is trying to
connect to. If the TCP listener ignores the MP_JOIN option and sends a
normal TCP SYN-ACK without a MP_JOIN option header, the peer will be
forced to send a RST.

Given that a peer sending MP_JOIN is guaranteed to reject a regular TCP
SYN-ACK, it's better to attempt a MPTCP token lookup and either let
MPTCP continue the handshake or immediately reject the connection
attempt.

The only new conditional in the TCP SYN handling hot path is a check of
the saw_mp_join bit. If that is set, tcp_check_req() returns early and
tcp_v4_rcv() or tcp_v6_rcv() call the MPTCP join handler in an error
path.

If MPTCP is not configured, all the code added in this commit should be
optimized away.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 net/ipv4/tcp_ipv4.c      | 7 +++++++
 net/ipv4/tcp_minisocks.c | 8 +++++++-
 net/ipv6/tcp_ipv6.c      | 7 +++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 73e5726af8d0..124ad393e6fb 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2082,6 +2082,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
 				tcp_v4_restore_cb(skb);
 				sock_put(sk);
 				goto lookup;
+			} else if (req_status == TCP_REQ_MP_JOIN) {
+				struct sock *msk = mptcp_handle_join4(skb);
+
+				if (msk) {
+					sk = msk;
+					goto process;
+				}
 			}
 			goto discard_and_relse;
 		}
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 3d989f8676a5..4fdca31b33bb 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -572,10 +572,16 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	bool own_req;
 
 	tmp_opt.saw_tstamp = 0;
+	tmp_opt.saw_mp_join = 0;
 	if (th->doff > (sizeof(struct tcphdr)>>2)) {
 		tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
 
-		if (tmp_opt.saw_tstamp) {
+		if (unlikely(tmp_opt.saw_mp_join) &&
+		    mptcp_is_enabled(sock_net(sk))) {
+			*req_status = TCP_REQ_MP_JOIN;
+
+			return NULL;
+		} else if (tmp_opt.saw_tstamp) {
 			tmp_opt.ts_recent = req->ts_recent;
 			if (tmp_opt.rcv_tsecr)
 				tmp_opt.rcv_tsecr -= tcp_rsk(req)->ts_off;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4706aad6dd8f..d46fa8243e83 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1731,6 +1731,13 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 				tcp_v6_restore_cb(skb);
 				sock_put(sk);
 				goto lookup;
+			} else if (req_status == TCP_REQ_MP_JOIN) {
+				struct sock *msk = mptcp_handle_join6(skb);
+
+				if (msk) {
+					sk = msk;
+					goto process;
+				}
 			}
 			goto discard_and_relse;
 		}
-- 
2.35.1

Re: tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener: Build Failure

[MPTCP CI]

Hi Mat,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/20220224232226.259304-5-mathew.j.martineau@linux.intel.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1895883332

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0f58cdeb67d0

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener: Tests Results

[MPTCP CI]

Hi Mat,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/6161624997298176
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6161624997298176/summary/summary.txt

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/5598675043876864
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5598675043876864/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0f58cdeb67d0

Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [RFC PATCH mptcp-next 4/4] tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener

[Paolo Abeni]

On Thu, 2022-02-24 at 15:22 -0800, Mat Martineau wrote:
> MPTCP uses a TCP SYN packet with the MP_JOIN option to add subflows to
> an existing MPTCP connection. This uses token lookup to match the
> incoming SYN with the appropriate MPTCP socket instead of the port
> number (in accordance with RFC 8684), and it is possible for a TCP
> listener to be present at the port that the MPTCP peer is trying to
> connect to. If the TCP listener ignores the MP_JOIN option and sends a
> normal TCP SYN-ACK without a MP_JOIN option header, the peer will be
> forced to send a RST.
> 
> Given that a peer sending MP_JOIN is guaranteed to reject a regular TCP
> SYN-ACK, it's better to attempt a MPTCP token lookup and either let
> MPTCP continue the handshake or immediately reject the connection
> attempt.
> 
> The only new conditional in the TCP SYN handling hot path is a check of
> the saw_mp_join bit. If that is set, tcp_check_req() returns early and
> tcp_v4_rcv() or tcp_v6_rcv() call the MPTCP join handler in an error
> path.
> 
> If MPTCP is not configured, all the code added in this commit should be
> optimized away.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
>  net/ipv4/tcp_ipv4.c      | 7 +++++++
>  net/ipv4/tcp_minisocks.c | 8 +++++++-
>  net/ipv6/tcp_ipv6.c      | 7 +++++++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 73e5726af8d0..124ad393e6fb 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -2082,6 +2082,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  				tcp_v4_restore_cb(skb);
>  				sock_put(sk);
>  				goto lookup;
> +			} else if (req_status == TCP_REQ_MP_JOIN) {
> +				struct sock *msk = mptcp_handle_join4(skb);
> +
> +				if (msk) {
> +					sk = msk;
> +					goto process;
> +				}

If I read correctly, this will handle MPJ 3rd ack packets, we will need
a similar chunk for incoming MPJ syn in tcp_rcv_state_process(), and
likely something additional for syncookies.

>  			}
>  			goto discard_and_relse;
>  		}
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 3d989f8676a5..4fdca31b33bb 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -572,10 +572,16 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>  	bool own_req;
>  
>  	tmp_opt.saw_tstamp = 0;
> +	tmp_opt.saw_mp_join = 0;
>  	if (th->doff > (sizeof(struct tcphdr)>>2)) {
>  		tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
>  
> -		if (tmp_opt.saw_tstamp) {
> +		if (unlikely(tmp_opt.saw_mp_join) &&
> +		    mptcp_is_enabled(sock_net(sk))) {
> +			*req_status = TCP_REQ_MP_JOIN;

If I read correctly, this skips all the state and sequence number check
we have on plain TCP 3rd ack. I think we want to preserve them.


Cheers,

Paolo

Re: [RFC PATCH mptcp-next 4/4] tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener

[Mat Martineau]

On Fri, 25 Feb 2022, Paolo Abeni wrote:

> On Thu, 2022-02-24 at 15:22 -0800, Mat Martineau wrote:
>> MPTCP uses a TCP SYN packet with the MP_JOIN option to add subflows to
>> an existing MPTCP connection. This uses token lookup to match the
>> incoming SYN with the appropriate MPTCP socket instead of the port
>> number (in accordance with RFC 8684), and it is possible for a TCP
>> listener to be present at the port that the MPTCP peer is trying to
>> connect to. If the TCP listener ignores the MP_JOIN option and sends a
>> normal TCP SYN-ACK without a MP_JOIN option header, the peer will be
>> forced to send a RST.
>>
>> Given that a peer sending MP_JOIN is guaranteed to reject a regular TCP
>> SYN-ACK, it's better to attempt a MPTCP token lookup and either let
>> MPTCP continue the handshake or immediately reject the connection
>> attempt.
>>
>> The only new conditional in the TCP SYN handling hot path is a check of
>> the saw_mp_join bit. If that is set, tcp_check_req() returns early and
>> tcp_v4_rcv() or tcp_v6_rcv() call the MPTCP join handler in an error
>> path.
>>
>> If MPTCP is not configured, all the code added in this commit should be
>> optimized away.
>>
>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> ---
>>  net/ipv4/tcp_ipv4.c      | 7 +++++++
>>  net/ipv4/tcp_minisocks.c | 8 +++++++-
>>  net/ipv6/tcp_ipv6.c      | 7 +++++++
>>  3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>> index 73e5726af8d0..124ad393e6fb 100644
>> --- a/net/ipv4/tcp_ipv4.c
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -2082,6 +2082,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>  				tcp_v4_restore_cb(skb);
>>  				sock_put(sk);
>>  				goto lookup;
>> +			} else if (req_status == TCP_REQ_MP_JOIN) {
>> +				struct sock *msk = mptcp_handle_join4(skb);
>> +
>> +				if (msk) {
>> +					sk = msk;
>> +					goto process;
>> +				}
>
> If I read correctly, this will handle MPJ 3rd ack packets, we will need
> a similar chunk for incoming MPJ syn in tcp_rcv_state_process(), and
> likely something additional for syncookies.
>

I think you're correct here and below. In trying to put a quick RFC 
together, I was thinking about what happens on this code path when trying 
to get the TCP listener to reject, but skipped over the use of this code 
by the MPTCP subflow listener. Handling both cases does make this approach 
more intrusive to the TCP code.

>>  			}
>>  			goto discard_and_relse;
>>  		}
>> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
>> index 3d989f8676a5..4fdca31b33bb 100644
>> --- a/net/ipv4/tcp_minisocks.c
>> +++ b/net/ipv4/tcp_minisocks.c
>> @@ -572,10 +572,16 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>>  	bool own_req;
>>
>>  	tmp_opt.saw_tstamp = 0;
>> +	tmp_opt.saw_mp_join = 0;
>>  	if (th->doff > (sizeof(struct tcphdr)>>2)) {
>>  		tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
>>
>> -		if (tmp_opt.saw_tstamp) {
>> +		if (unlikely(tmp_opt.saw_mp_join) &&
>> +		    mptcp_is_enabled(sock_net(sk))) {
>> +			*req_status = TCP_REQ_MP_JOIN;
>
> If I read correctly, this skips all the state and sequence number check
> we have on plain TCP 3rd ack. I think we want to preserve them.

--
Mat Martineau
Intel

Re: [RFC PATCH mptcp-next 4/4] tcp: Add TCP hooks for detecting MP_JOIN on a regular TCP listener

[Mat Martineau]

On Wed, 2 Mar 2022, Mat Martineau wrote:

> On Fri, 25 Feb 2022, Paolo Abeni wrote:
>
>> On Thu, 2022-02-24 at 15:22 -0800, Mat Martineau wrote:
>>> MPTCP uses a TCP SYN packet with the MP_JOIN option to add subflows to
>>> an existing MPTCP connection. This uses token lookup to match the
>>> incoming SYN with the appropriate MPTCP socket instead of the port
>>> number (in accordance with RFC 8684), and it is possible for a TCP
>>> listener to be present at the port that the MPTCP peer is trying to
>>> connect to. If the TCP listener ignores the MP_JOIN option and sends a
>>> normal TCP SYN-ACK without a MP_JOIN option header, the peer will be
>>> forced to send a RST.
>>> 
>>> Given that a peer sending MP_JOIN is guaranteed to reject a regular TCP
>>> SYN-ACK, it's better to attempt a MPTCP token lookup and either let
>>> MPTCP continue the handshake or immediately reject the connection
>>> attempt.
>>> 
>>> The only new conditional in the TCP SYN handling hot path is a check of
>>> the saw_mp_join bit. If that is set, tcp_check_req() returns early and
>>> tcp_v4_rcv() or tcp_v6_rcv() call the MPTCP join handler in an error
>>> path.
>>> 
>>> If MPTCP is not configured, all the code added in this commit should be
>>> optimized away.
>>> 
>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>> ---
>>>  net/ipv4/tcp_ipv4.c      | 7 +++++++
>>>  net/ipv4/tcp_minisocks.c | 8 +++++++-
>>>  net/ipv6/tcp_ipv6.c      | 7 +++++++
>>>  3 files changed, 21 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>>> index 73e5726af8d0..124ad393e6fb 100644
>>> --- a/net/ipv4/tcp_ipv4.c
>>> +++ b/net/ipv4/tcp_ipv4.c
>>> @@ -2082,6 +2082,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>>  				tcp_v4_restore_cb(skb);
>>>  				sock_put(sk);
>>>  				goto lookup;
>>> +			} else if (req_status == TCP_REQ_MP_JOIN) {
>>> +				struct sock *msk = mptcp_handle_join4(skb);
>>> +
>>> +				if (msk) {
>>> +					sk = msk;
>>> +					goto process;
>>> +				}
>> 
>> If I read correctly, this will handle MPJ 3rd ack packets, we will need
>> a similar chunk for incoming MPJ syn in tcp_rcv_state_process(), and
>> likely something additional for syncookies.
>> 
>
> I think you're correct here and below. In trying to put a quick RFC 
> together, I was thinking about what happens on this code path when 
> trying to get the TCP listener to reject, but skipped over the use of 
> this code by the MPTCP subflow listener. Handling both cases does make 
> this approach more intrusive to the TCP code.
>

...on second thought, maybe not much more intrusive?

>>>  			}
>>>  			goto discard_and_relse;
>>>  		}
>>> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
>>> index 3d989f8676a5..4fdca31b33bb 100644
>>> --- a/net/ipv4/tcp_minisocks.c
>>> +++ b/net/ipv4/tcp_minisocks.c
>>> @@ -572,10 +572,16 @@ struct sock *tcp_check_req(struct sock *sk, struct 
>>> sk_buff *skb,
>>>  	bool own_req;
>>>
>>>  	tmp_opt.saw_tstamp = 0;
>>> +	tmp_opt.saw_mp_join = 0;
>>>  	if (th->doff > (sizeof(struct tcphdr)>>2)) {
>>>  		tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
>>> 
>>> -		if (tmp_opt.saw_tstamp) {
>>> +		if (unlikely(tmp_opt.saw_mp_join) &&

Make sure this only triggers on a regular TCP listener:

+		    !sk_is_mptcp(sk) &&

>>> +		    mptcp_is_enabled(sock_net(sk))) {
>>> +			*req_status = TCP_REQ_MP_JOIN;

Then the TCP_REQ_MP_JOIN status is only ever encountered on a plain TCP 
listener where the MP_JOIN header is present. This was my original intent 
but the above conditional was buggy.

>> 
>> If I read correctly, this skips all the state and sequence number check
>> we have on plain TCP 3rd ack. I think we want to preserve them.
>

For plain TCP 3rd ack, there's no MP_JOIN option and tcp_check_req() 
behavior is unchanged.

For a MP_JOIN SYN being processed by a plain TCP listener, this returns 
early, then the token lookup is attempted as if there had been no TCP 
listener to begin with. If there's a token match then the MPTCP listener 
will do all the checks like it did before.

--
Mat Martineau
Intel

Re: [RFC PATCH mptcp-next 0/4] MP_JOIN fallback for TCP listeners

[Paolo Abeni]

On Thu, 2022-02-24 at 15:22 -0800, Mat Martineau wrote:
> Compared to Florian's proposal (add token lookup before the listener
> lookup) this does change more lines of TCP code, but has less runtime
> impact.

I think this will need more changes to TCP code, see my reply to patch
4/4.

I really don't know which one will affect plain TCP setup more from
performance PoV: we will have a token lookup for each incoming syn
packet with both impl. With this patch we still need "mptcp: replace
per-addr listener sockets", right? otherwise issues/203 will not be
addressed.

The thing that concerns me more is that this approach brings quite a
bit of MTPCP logic inside the TCP code with little/no isolation.

More I think about it and more I belive we will should try to code as
simple as possible. And the easyest way to address issues/203 is
possibly at the documentation level...

Thanks,

Paolo

Re: [RFC PATCH mptcp-next 0/4] MP_JOIN fallback for TCP listeners

[Mat Martineau]

On Fri, 25 Feb 2022, Paolo Abeni wrote:

> On Thu, 2022-02-24 at 15:22 -0800, Mat Martineau wrote:
>> Compared to Florian's proposal (add token lookup before the listener
>> lookup) this does change more lines of TCP code, but has less runtime
>> impact.
>
> I think this will need more changes to TCP code, see my reply to patch
> 4/4.
>
> I really don't know which one will affect plain TCP setup more from
> performance PoV: we will have a token lookup for each incoming syn
> packet with both impl.

This proposal has less runtime impact on plain TCP. It uses the existing 
TCP option parsing pass in tcp_check_req(), and adds one conditional 
that's looking at the new saw_mp_join bit that was possibly set when 
parsing options.

Florian's "mptcp: handle join requests early" is a smaller change to the 
TCP code, but adds an extra function call and a new TCP option parsing 
pass for every incoming SYN. Since the MP_JOIN option will not typically 
be present, it doesn't look like there are many extra CPU cycles other 
than that option parsing pass.

> With this patch we still need "mptcp: replace
> per-addr listener sockets", right? otherwise issues/203 will not be
> addressed.

Right.

>
> The thing that concerns me more is that this approach brings quite a
> bit of MTPCP logic inside the TCP code with little/no isolation.
>

If there's enough interest for a RFC v2 I could refactor with some new 
mptcp helpers.

> More I think about it and more I belive we will should try to code as
> simple as possible. And the easyest way to address issues/203 is
> possibly at the documentation level...
>

There seems to be enough benefit to socket users to justify some amount of 
code change to avoid confusing and hard-to-troubleshoot behavior. I still 
lean toward making it work better, and acknowledge the need for better 
docs no matter the solution :)

--
Mat Martineau
Intel