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

[Dmytro Shytyi <dmytro@shytyi.net>]

Thank you Paolo for this interesting comment with great approach.

Version 9 of mptfo is coming with implemented and working 
"subflow_v4_send_synack()"


On 9/20/2022 6:02 PM, Paolo Abeni wrote:
> 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;
>
>
>
>