Re: [RFC PATCH mptcp-next v8 6/7] add skb to mskq in tcp_fastopen_add_skb()

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
> In the following patches we add skb to msk->receive_queue in the MPTCP
> fastopen context.
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  include/net/tcp.h       |  2 +-
>  net/ipv4/tcp_fastopen.c | 55 +++++++++++++++++++++++++++++++++++------
>  net/ipv4/tcp_input.c    | 11 +++++++--
>  net/mptcp/protocol.c    |  4 +--
>  net/mptcp/protocol.h    |  2 ++
>  5 files changed, 62 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index a7d49e42470a..6456f90ed9ed 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1749,7 +1749,7 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
>  			      void *primary_key, void *backup_key);
>  int tcp_fastopen_get_cipher(struct net *net, struct inet_connection_sock *icsk,
>  			    u64 *key);
> -void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb);
> +void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb, struct request_sock *req);
>  struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
>  			      struct request_sock *req,
>  			      struct tcp_fastopen_cookie *foc,
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 45cc7f1ca296..566706172828 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -3,6 +3,7 @@
>  #include <linux/tcp.h>
>  #include <linux/rcupdate.h>
>  #include <net/tcp.h>
> +#include "../mptcp/protocol.h"
>  
>  void tcp_fastopen_init_key_once(struct net *net)
>  {
> @@ -166,8 +167,12 @@ static void tcp_fastopen_cookie_gen(struct sock *sk,
>  /* If an incoming SYN or SYNACK frame contains a payload and/or FIN,
>   * queue this additional data / FIN.
>   */
> -void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
> +void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb, struct request_sock *req)
>  {
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct tcp_request_sock *tcp_r_sock = tcp_rsk(req);
> +	struct sock *socket = mptcp_subflow_ctx(sk)->conn;
> +	struct mptcp_sock *msk = mptcp_sk(socket);
>  	struct tcp_sock *tp = tcp_sk(sk);
>  
>  	if (TCP_SKB_CB(skb)->end_seq == tp->rcv_nxt)
> @@ -194,7 +199,34 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
>  	TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_SYN;
>  
>  	tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
> +
> +	if (req && tp->syn_fastopen && sk_is_mptcp(sk))
> +		tcp_r_sock = tcp_rsk(req);
> +	else
> +		goto add_skb_to_sk;
> +
> +	msk->is_mptfo = 1;
> +
> +	//Solves: WARNING: at 704 _mptcp_move_skbs_from_subflow+0x5d0/0x651
> +	tp->copied_seq += tp->rcv_nxt - tcp_r_sock->rcv_isn - 1;
> +
> +	subflow->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> +
> +	//Solves: BAD mapping: ssn=0 map_seq=1 map_data_len=3
> +	subflow->ssn_offset = tp->copied_seq - 1;
> +
> +	skb_orphan(skb);
> +	skb->sk = socket;
> +	skb->destructor = mptcp_rfree;
> +	atomic_add(skb->truesize, &socket->sk_rmem_alloc);
> +	msk->rmem_fwd_alloc -= skb->truesize;
> +
> +	__skb_queue_tail(&msk->receive_queue, skb);
> +	atomic64_set(&msk->rcv_wnd_sent, mptcp_subflow_get_mapped_dsn(subflow));
> +	goto avoid_add_skb_to_sk;

AFAICS the above:

- mark the passive socket as an TFO one.
- set the mapping the DSS mapping for the TFO syn data
- bypass the usual MPTCP receive path

I think we can do all the above elsewhere, with no need to touch the
tcp code in an IMHO cleaner way, something alike the following (mostly
pseudo code, only ipv4 example, the ipv6 part should be added, too):

---
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..4e23fa9f0083 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -307,6 +307,28 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 	return NULL;
 }
 
+static int subflow_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
+				  struct flowi *fl,
+				  struct request_sock *req,
+				  struct tcp_fastopen_cookie *foc,
+				  enum tcp_synack_type synack_type,
+				  struct sk_buff *syn_skb)
+{
+	if (synack_type == TCP_SYNACK_FASTOPEN) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+
+		<mark subflow/msk as "mptfo">
+		<evenutally clear tstamp_ok, as needed depending on cookie size>
+		<dequeue the skb from sk receive queue>
+		<set the skb mapping>
+		<acquire the msk data lock>
+		<move skb into msk receive queue>
+		<call msk data_ready>
+		<release the msk data lock>
+	}
+	return tcp_request_sock_ipv4_ops.send_synack(sk, dst, fl, req, foc, synack_type, syn_skb);
+}
+
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 					      struct sk_buff *skb,
@@ -1939,6 +1960,7 @@ void __init mptcp_subflow_init(void)
 
 	subflow_request_sock_ipv4_ops = tcp_request_sock_ipv4_ops;
 	subflow_request_sock_ipv4_ops.route_req = subflow_v4_route_req;
+	subflow_request_sock_ipv4_ops.send_synack = subflow_v4_send_synack;
 
 	subflow_specific = ipv4_specific;
 	subflow_specific.conn_request = subflow_v4_conn_request;