Re: [RFC PATCH mptcp-next v9 5/6] mptcp: add subflow_v(4,6)_send_synack()

[Dmytro Shytyi <dmytro@shytyi.net>]

Hello Paolo,

Thank you for comments.

My comments are in-line + the new patch in the bottom of this message 
with corrections.

On 9/21/2022 8:00 PM, Paolo Abeni wrote:
> On Wed, 2022-09-21 at 14:55 +0200, Dmytro Shytyi wrote:
>> In this patch we add skb to the msk, dequeue it from sk, remove TSs and
>> do skb mapping.
>>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>   net/ipv4/tcp_fastopen.c | 19 +++++++----
>>   net/mptcp/protocol.c    |  2 +-
>>   net/mptcp/protocol.h    |  1 +
>>   net/mptcp/subflow.c     | 70 +++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 85 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
>> index 45cc7f1ca296..d6b1380525ea 100644
>> --- a/net/ipv4/tcp_fastopen.c
>> +++ b/net/ipv4/tcp_fastopen.c
>> @@ -356,13 +356,20 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
>>   	if (foc->len == 0) /* Client requests a cookie */
>>   		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPFASTOPENCOOKIEREQD);
>>   
>> -	if (!((tcp_fastopen & TFO_SERVER_ENABLE) &&
>> -	      (syn_data || foc->len >= 0) &&
>> -	      tcp_fastopen_queue_check(sk))) {
>> -		foc->len = -1;
>> -		return NULL;
>> +	if (sk_is_mptcp(sk)) {
>> +		if (((syn_data || foc->len >= 0) &&
>> +		     tcp_fastopen_queue_check(sk))) {
>> +			foc->len = -1;
>> +			return NULL;
>> +		}
>> +	} else {
>> +		if (!((tcp_fastopen & TFO_SERVER_ENABLE) &&
>> +		      (syn_data || foc->len >= 0) &&
>> +		      tcp_fastopen_queue_check(sk))) {
>> +			foc->len = -1;
>> +			return NULL;
>> +		}
>>   	}
>> -
> Why the above chunk is needed?

I didn't check why yet, but without this I don't have Cookie reply from 
listener even if I activate fastopen in sysctl.

I realise this is not good to do. Currently it is more like bypass. I am 
going to look at if later.


>>   	if (tcp_fastopen_no_cookie(sk, dst, TFO_SERVER_COOKIE_NOT_REQD))
>>   		goto fastopen;
>>   
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index d5c502d141b4..6a593be6076b 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -200,7 +200,7 @@ static void mptcp_rfree(struct sk_buff *skb)
>>   	mptcp_rmem_uncharge(sk, len);
>>   }
>>   
>> -static void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk)
>> +void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk)
>>   {
>>   	skb_orphan(skb);
>>   	skb->sk = sk;
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index b9e251848099..58a04144fff0 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -845,6 +845,7 @@ int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>>   				      unsigned int optlen);
>>   void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
>>   				   struct mptcp_options_received mp_opt);
>> +void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk);
>>   // Fast Open Mechanism functions end
>>   
>>   static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 07dd23d0fe04..7deb80c2af69 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -307,6 +307,74 @@ 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,
> If you use 'ssk' instead of 'sk'

I dont think it is a good idea to change sk to ssk as argument and use 
it because of "const" compiler should throw an error if I will try to 
dequeue, etc from const struct sock *sk ant it is right. So I retreived 
ssk from subflow without const to do what is needed.

But still I modify the naming scheme as you suggested.

>> +				  struct flowi *fl,
>> +				  struct request_sock *req,
>> +				  struct tcp_fastopen_cookie *foc,
>> +				  enum tcp_synack_type synack_type,
>> +				  struct sk_buff *syn_skb)
>> +{
>> +	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;
> than you can use 'sk' here and respect mptcp convection for variable
> names.
Yes, Fixed.
>> +	struct inet_request_sock *ireq = inet_rsk(req);
>> +	struct mptcp_sock *msk = mptcp_sk(socket);
>> +	struct sock *var_sk = subflow->tcp_sock;
> This should be actually equal to ssk
var_sk changed to ssk
>> +	struct tcp_sock *tp = tcp_sk(sk);
>> +	struct sk_buff *skb;
> All the above variable should be moved under the 'if (synack_type ==
> TCP_SYNACK_FASTOPEN) {' branch, as there are used only there.
>
> Additionally if you move all the code in the 'if (synack_type ==
> TCP_SYNACK_FASTOPEN)' branch in a separate helper you can later reuse
> it in subflow_v6_send_synack()
>
>> +
>> +	//<evenutally clear tstamp_ok, as needed depending on cookie size>
>> +	//We add ts here as in the "if" below it has no effect.
> Please, don't use '//' for comments

Oh sorry, Yes in C we don't use these "//", I replaced it with "/* */".


>> +	if (foc->len > -1) {
>> +		ireq->tstamp_ok = 0;
>> +	}
> No need to add the '{' for a single line 'then' statement; addtionally
> I guess you can move the above check under the below if, and you should
> check for 'foc != NULL'


If I move the foc check into "if", no metter how I modify it : "foc != 
NULL" or " for != -1", it doesn't affect the TS, but it should.

I suppose I observe this behaviour because of "tcp_conn_request()" 
starting from line 7013: tcp_input.c - net/ipv4/tcp_input.c - Linux 
source code (v5.19.10) - Bootlin 
<https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_input.c#L7013>


I keep "foc" check outside "if" statement for the moment.



>
>> +	if (synack_type == TCP_SYNACK_FASTOPEN) {
>> +		// <mark subflow/msk as "mptfo">
>> +		msk->is_mptfo = 1;
>> +
>> +		skb = skb_peek(&var_sk->sk_receive_queue);
>> +
>> +		//<dequeue the skb from sk receive queue>
>> +		__skb_unlink(skb, &var_sk->sk_receive_queue);
>> +		skb_ext_reset(skb);
>> +		skb_orphan(skb);
>> +
>> +		//<set the skb mapping>
>> +		//Solves: WARNING: at 704 _mptcp_move_skbs_from_subflow+0x5d0/0x651
> Please, drop the WARNING reference.
Fixed
>> +		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
> Same here.
FIxed
>> +		subflow->ssn_offset = tp->copied_seq - 1;
>> +
>> +		//<acquire the msk data lock>
>> +		lock_sock((struct sock *)msk);
> the mptcp data lock is acquired with:
>
> 		 mptcp_data_lock((struct sock *)msk)
>
> which become
> 		mptcp_data_lock(sk);
>
> if you apply the mptcp variable name conventions.
Fixed
> I think you additionally need to init the mptcp CB for skb.
>
> I'm not sure what will happen to later skb coming via
> __mptcp_move_skb(). You must ensure that they will not be coalesced.
> Probably correctly initializing the mptcp CB should ensure the above.

Could you please evolve the comment? Set the MPTCP_SKB_CB: offset , 
map_seq, end_seq?


>> +
>> +		//<move skb into msk receive queue>
>> +		mptcp_set_owner_r(skb, (struct sock *)msk);
>> +		__skb_queue_tail(&msk->receive_queue, skb);
>> +		atomic64_set(&msk->rcv_wnd_sent, mptcp_subflow_get_mapped_dsn(subflow));
>> +
>> +		//<call msk data_ready>
>> +		((struct sock *)msk)->sk_data_ready((struct sock *)msk);
> or just:
> 		sk->sk_data_ready(sk);
>
> (when respecting the mptcp variables name conventions)
Fixed.
>> +
>> +		//<release the msk data lock>
>> +		release_sock((struct sock *)msk);
>> +	}
>> +	return tcp_request_sock_ipv4_ops.send_synack(sk, dst, fl, req, foc, synack_type, syn_skb);
>> +}
>> +
>> +static int subflow_v6_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)
>> +{
> This is not enough, you need to do the same things as under the 'if
> (synack_type == TCP_SYNACK_FASTOPEN) {' conditional in
> subflow_v4_send_synack()

Yes, it was a placeholder. I was wondering if the content is  the same 
as in v4 probably I could add a helper and reuse in v4/6. And hopefully 
you suggest to do so in this message :)


>> +	return tcp_request_sock_ipv6_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,
>> @@ -1920,6 +1988,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;
>> @@ -1933,6 +2002,7 @@ void __init mptcp_subflow_init(void)
>>   #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>   	subflow_request_sock_ipv6_ops = tcp_request_sock_ipv6_ops;
>>   	subflow_request_sock_ipv6_ops.route_req = subflow_v6_route_req;
>> +	subflow_request_sock_ipv6_ops.send_synack = subflow_v6_send_synack;
>>   
>>   	subflow_v6_specific = ipv6_specific;
>>   	subflow_v6_specific.conn_request = subflow_v6_conn_request;


Here is a commit with modifictions:

 From 13de5d435a4323519235c811719bd9d5fd1802de Mon Sep 17 00:00:00 2001
From: Dmytro Shytyi <dmytro@shytyi.net>
Date: Tue, 20 Sep 2022 19:53:56 +0200
Subject: [RFC PATCH mptcp-next v9 5/6] mptcp: add 
subflow_v(4,6)_send_synack()

In this patch we add skb to the msk, dequeue it from sk, remove TSs and
do skb mapping.

Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
---
  net/ipv4/tcp_fastopen.c | 19 ++++++---
  net/mptcp/protocol.c    |  2 +-
  net/mptcp/protocol.h    |  1 +
  net/mptcp/subflow.c     | 85 +++++++++++++++++++++++++++++++++++++++++
  4 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 45cc7f1ca296..d6b1380525ea 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -356,13 +356,20 @@ struct sock *tcp_try_fastopen(struct sock *sk, 
struct sk_buff *skb,
      if (foc->len == 0) /* Client requests a cookie */
          NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPFASTOPENCOOKIEREQD);

-    if (!((tcp_fastopen & TFO_SERVER_ENABLE) &&
-          (syn_data || foc->len >= 0) &&
-          tcp_fastopen_queue_check(sk))) {
-        foc->len = -1;
-        return NULL;
+    if (sk_is_mptcp(sk)) {
+        if (((syn_data || foc->len >= 0) &&
+             tcp_fastopen_queue_check(sk))) {
+            foc->len = -1;
+            return NULL;
+        }
+    } else {
+        if (!((tcp_fastopen & TFO_SERVER_ENABLE) &&
+              (syn_data || foc->len >= 0) &&
+              tcp_fastopen_queue_check(sk))) {
+            foc->len = -1;
+            return NULL;
+        }
      }
-
      if (tcp_fastopen_no_cookie(sk, dst, TFO_SERVER_COOKIE_NOT_REQD))
          goto fastopen;

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d5c502d141b4..6a593be6076b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -200,7 +200,7 @@ static void mptcp_rfree(struct sk_buff *skb)
      mptcp_rmem_uncharge(sk, len);
  }

-static void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk)
+void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk)
  {
      skb_orphan(skb);
      skb->sk = sk;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index b9e251848099..58a04144fff0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -845,6 +845,7 @@ int mptcp_setsockopt_sol_tcp_fastopen(struct 
mptcp_sock *msk, sockptr_t optval,
                        unsigned int optlen);
  void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct 
mptcp_subflow_context *subflow,
                     struct mptcp_options_received mp_opt);
+void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk);
  // Fast Open Mechanism functions end

  static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 07dd23d0fe04..cca888bae261 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -307,6 +307,89 @@ static struct dst_entry *subflow_v4_route_req(const 
struct sock *sk,
      return NULL;
  }

+static void subflow_fastopen_set_params(struct mptcp_subflow_context 
*subflow,
+                    struct request_sock *req)
+{
+    struct tcp_request_sock *tcp_r_sock = tcp_rsk(req);
+    struct sock *ssk = subflow->tcp_sock;
+    struct sock *sk = subflow->conn;
+    struct mptcp_sock *msk;
+    struct sk_buff *skb;
+    struct tcp_sock *tp;
+
+    msk = mptcp_sk(sk);
+    tp = tcp_sk(ssk);
+
+    /* <mark subflow/msk as "mptfo"> */
+    msk->is_mptfo = 1;
+
+    skb = skb_peek(&ssk->sk_receive_queue);
+
+    /* <dequeue the skb from sk receive queue> */
+    __skb_unlink(skb, &ssk->sk_receive_queue);
+    skb_ext_reset(skb);
+    skb_orphan(skb);
+
+    /* <set the skb mapping> */
+    tp->copied_seq += tp->rcv_nxt - tcp_r_sock->rcv_isn - 1;
+    subflow->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
+    subflow->ssn_offset = tp->copied_seq - 1;
+
+    /* <acquire the msk data lock> */
+    mptcp_data_lock(sk);
+
+    /* <move skb into msk receive queue> */
+    mptcp_set_owner_r(skb, sk);
+    __skb_queue_tail(&msk->receive_queue, skb);
+    atomic64_set(&msk->rcv_wnd_sent, 
mptcp_subflow_get_mapped_dsn(subflow));
+
+    /* <call msk data_ready> */
+    (sk)->sk_data_ready(sk);
+
+    /* <release the msk data lock> */
+    mptcp_data_unlock(sk);
+}
+
+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)
+{
+    struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+    struct inet_request_sock *ireq = inet_rsk(req);
+
+    /* <evenutally clear tstamp_ok, as needed depending on cookie size> */
+    if (foc != NULL)
+        ireq->tstamp_ok = 0;
+
+    if (synack_type == TCP_SYNACK_FASTOPEN) {
+        subflow_fastopen_set_params(subflow, req);
+    }
+    return tcp_request_sock_ipv4_ops.send_synack(sk, dst, fl, req, foc, 
synack_type, syn_skb);
+}
+
+static int subflow_v6_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)
+{
+    struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+    struct inet_request_sock *ireq = inet_rsk(req);
+
+    /* <evenutally clear tstamp_ok, as needed depending on cookie size> */
+    if (foc != NULL)
+        ireq->tstamp_ok = 0;
+
+    if (synack_type == TCP_SYNACK_FASTOPEN) {
+        subflow_fastopen_set_params(subflow, req);
+    }
+    return tcp_request_sock_ipv6_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,
@@ -1920,6 +2003,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;
@@ -1933,6 +2017,7 @@ void __init mptcp_subflow_init(void)
  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
      subflow_request_sock_ipv6_ops = tcp_request_sock_ipv6_ops;
      subflow_request_sock_ipv6_ops.route_req = subflow_v6_route_req;
+    subflow_request_sock_ipv6_ops.send_synack = subflow_v6_send_synack;

      subflow_v6_specific = ipv6_specific;
      subflow_v6_specific.conn_request = subflow_v6_conn_request;
-- 
2.25.1