[PATCH mptcp-net v2 0/3] mptcp: fix request sock for subflow in v6

[PATCH mptcp-net v2 0/3] mptcp: fix request sock for subflow in v6

[Matthieu Baerts]

ChangeLog:
- v2:
  The CI reported the structure I moved from mptcp.h to subflow.c only can be
  static (from 'make C=1'). While at it, I also marked it as __ro_after_init.
  See patch 1/3 (also in 2/3 for the v6 part).

When working on validating the listener part of TCP FastOpen with Packetdrill, I
noticed the cookie in IPv6 was not the expected one.

The root cause was that req->rsk_ops->family was set to AF_INET while the
subflow was in v6. Patch 2/3 solves that.

While looking around, I noticed we were always calling the destructor from TCP
in v4 and I changed that in patch 3/3. I don't think there is a memory leaks in
usual cases.

Paolo did a pre-review (thanks again for that!) and suggested the patch 1/3.


Matthieu Baerts (3):
  mptcp: remove MPTCP 'ifdef' in TCP SYN cookies
  mptcp: dedicated request sock for subflow in v6
  mptcp: use proper req destructor for IPv6

 include/net/mptcp.h   | 12 +++++++--
 net/ipv4/syncookies.c |  7 +++--
 net/mptcp/subflow.c   | 61 ++++++++++++++++++++++++++++++++++++-------
 3 files changed, 64 insertions(+), 16 deletions(-)


base-commit: 8de87563b5eb14ed009c26cae1e6afbff35c93e0
-- 
2.37.2

[PATCH mptcp-net v2 1/3] mptcp: remove MPTCP 'ifdef' in TCP SYN cookies

[Matthieu Baerts]

To ease the maintenance, it is often recommended to avoid having #ifdef
preprocessor conditions.

Here the section related to CONFIG_MPTCP was quite short but the next
commit needs to add more code around. It is then cleaner to move
specific MPTCP code to functions located in net/mptcp directory.

Now that mptcp_subflow_request_sock_ops structure can be static, it can
also be marked as "read only after init".

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    v2:
     - mark mptcp_subflow_request_sock_ops as static and __ro_after_init

 include/net/mptcp.h   | 12 ++++++++++--
 net/ipv4/syncookies.c |  7 +++----
 net/mptcp/subflow.c   | 12 +++++++++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 412479ebf5ad..3c5c68618fcc 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -97,8 +97,6 @@ struct mptcp_out_options {
 };
 
 #ifdef CONFIG_MPTCP
-extern struct request_sock_ops mptcp_subflow_request_sock_ops;
-
 void mptcp_init(void);
 
 static inline bool sk_is_mptcp(const struct sock *sk)
@@ -188,6 +186,9 @@ void mptcp_seq_show(struct seq_file *seq);
 int mptcp_subflow_init_cookie_req(struct request_sock *req,
 				  const struct sock *sk_listener,
 				  struct sk_buff *skb);
+struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
+					       struct sock *sk_listener,
+					       bool attach_listener);
 
 __be32 mptcp_get_reset_option(const struct sk_buff *skb);
 
@@ -274,6 +275,13 @@ static inline int mptcp_subflow_init_cookie_req(struct request_sock *req,
 	return 0; /* TCP fallback */
 }
 
+static inline struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
+							     struct sock *sk_listener,
+							     bool attach_listener)
+{
+	return NULL;
+}
+
 static inline __be32 mptcp_reset_option(const struct sk_buff *skb)  { return htonl(0u); }
 #endif /* CONFIG_MPTCP */
 
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 942d2dfa1115..26fb97d1d4d9 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -288,12 +288,11 @@ struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
 	struct tcp_request_sock *treq;
 	struct request_sock *req;
 
-#ifdef CONFIG_MPTCP
 	if (sk_is_mptcp(sk))
-		ops = &mptcp_subflow_request_sock_ops;
-#endif
+		req = mptcp_subflow_reqsk_alloc(ops, sk, false);
+	else
+		req = inet_reqsk_alloc(ops, sk, false);
 
-	req = inet_reqsk_alloc(ops, sk, false);
 	if (!req)
 		return NULL;
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2159b5f9988f..3f670f2d5c5c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -529,7 +529,7 @@ static int subflow_v6_rebuild_header(struct sock *sk)
 }
 #endif
 
-struct request_sock_ops mptcp_subflow_request_sock_ops;
+static struct request_sock_ops mptcp_subflow_request_sock_ops __ro_after_init;
 static struct tcp_request_sock_ops subflow_request_sock_ipv4_ops __ro_after_init;
 
 static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
@@ -582,6 +582,16 @@ static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 }
 #endif
 
+struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
+					       struct sock *sk_listener,
+					       bool attach_listener)
+{
+	ops = &mptcp_subflow_request_sock_ops;
+
+	return inet_reqsk_alloc(ops, sk_listener, attach_listener);
+}
+EXPORT_SYMBOL(mptcp_subflow_reqsk_alloc);
+
 /* validate hmac received in third ACK */
 static bool subflow_hmac_valid(const struct request_sock *req,
 			       const struct mptcp_options_received *mp_opt)
-- 
2.37.2

Re: [PATCH mptcp-net v2 1/3] mptcp: remove MPTCP 'ifdef' in TCP SYN cookies

[Mat Martineau]

On Wed, 30 Nov 2022, Matthieu Baerts wrote:

> To ease the maintenance, it is often recommended to avoid having #ifdef
> preprocessor conditions.
>
> Here the section related to CONFIG_MPTCP was quite short but the next
> commit needs to add more code around. It is then cleaner to move
> specific MPTCP code to functions located in net/mptcp directory.
>
> Now that mptcp_subflow_request_sock_ops structure can be static, it can
> also be marked as "read only after init".
>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Hi Matthieu -

I noticed today that there's no Fixes: tag for this commit, which the 
netdev CI will check for. Like you say in the commit message, this patch 
is a small bit of refactoring to prepare for the following patch(es) - 
should it share Fixes: tags with those or just let the git dependencies 
speak for themselves?

- Mat

> ---
>
> Notes:
>    v2:
>     - mark mptcp_subflow_request_sock_ops as static and __ro_after_init
>
> include/net/mptcp.h   | 12 ++++++++++--
> net/ipv4/syncookies.c |  7 +++----
> net/mptcp/subflow.c   | 12 +++++++++++-
> 3 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 412479ebf5ad..3c5c68618fcc 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -97,8 +97,6 @@ struct mptcp_out_options {
> };
>
> #ifdef CONFIG_MPTCP
> -extern struct request_sock_ops mptcp_subflow_request_sock_ops;
> -
> void mptcp_init(void);
>
> static inline bool sk_is_mptcp(const struct sock *sk)
> @@ -188,6 +186,9 @@ void mptcp_seq_show(struct seq_file *seq);
> int mptcp_subflow_init_cookie_req(struct request_sock *req,
> 				  const struct sock *sk_listener,
> 				  struct sk_buff *skb);
> +struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
> +					       struct sock *sk_listener,
> +					       bool attach_listener);
>
> __be32 mptcp_get_reset_option(const struct sk_buff *skb);
>
> @@ -274,6 +275,13 @@ static inline int mptcp_subflow_init_cookie_req(struct request_sock *req,
> 	return 0; /* TCP fallback */
> }
>
> +static inline struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
> +							     struct sock *sk_listener,
> +							     bool attach_listener)
> +{
> +	return NULL;
> +}
> +
> static inline __be32 mptcp_reset_option(const struct sk_buff *skb)  { return htonl(0u); }
> #endif /* CONFIG_MPTCP */
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 942d2dfa1115..26fb97d1d4d9 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -288,12 +288,11 @@ struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
> 	struct tcp_request_sock *treq;
> 	struct request_sock *req;
>
> -#ifdef CONFIG_MPTCP
> 	if (sk_is_mptcp(sk))
> -		ops = &mptcp_subflow_request_sock_ops;
> -#endif
> +		req = mptcp_subflow_reqsk_alloc(ops, sk, false);
> +	else
> +		req = inet_reqsk_alloc(ops, sk, false);
>
> -	req = inet_reqsk_alloc(ops, sk, false);
> 	if (!req)
> 		return NULL;
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 2159b5f9988f..3f670f2d5c5c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -529,7 +529,7 @@ static int subflow_v6_rebuild_header(struct sock *sk)
> }
> #endif
>
> -struct request_sock_ops mptcp_subflow_request_sock_ops;
> +static struct request_sock_ops mptcp_subflow_request_sock_ops __ro_after_init;
> static struct tcp_request_sock_ops subflow_request_sock_ipv4_ops __ro_after_init;
>
> static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
> @@ -582,6 +582,16 @@ static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
> }
> #endif
>
> +struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
> +					       struct sock *sk_listener,
> +					       bool attach_listener)
> +{
> +	ops = &mptcp_subflow_request_sock_ops;
> +
> +	return inet_reqsk_alloc(ops, sk_listener, attach_listener);
> +}
> +EXPORT_SYMBOL(mptcp_subflow_reqsk_alloc);
> +
> /* validate hmac received in third ACK */
> static bool subflow_hmac_valid(const struct request_sock *req,
> 			       const struct mptcp_options_received *mp_opt)
> -- 
> 2.37.2
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net v2 1/3] mptcp: remove MPTCP 'ifdef' in TCP SYN cookies

[Matthieu Baerts]

Hi Mat,

On 05/12/2022 22:52, Mat Martineau wrote:
> On Wed, 30 Nov 2022, Matthieu Baerts wrote:
> 
>> To ease the maintenance, it is often recommended to avoid having #ifdef
>> preprocessor conditions.
>>
>> Here the section related to CONFIG_MPTCP was quite short but the next
>> commit needs to add more code around. It is then cleaner to move
>> specific MPTCP code to functions located in net/mptcp directory.
>>
>> Now that mptcp_subflow_request_sock_ops structure can be static, it can
>> also be marked as "read only after init".
>>
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> Hi Matthieu -
> 
> I noticed today that there's no Fixes: tag for this commit, which the
> netdev CI will check for. Like you say in the commit message, this patch
> is a small bit of refactoring to prepare for the following patch(es) -
> should it share Fixes: tags with those or just let the git dependencies
> speak for themselves?

I don't think we need to add a Fixes tag because as you said, it is
mentioned this modification is needed for the next patch. I'm sure
netdev maintainers will understand why it doesn't have a "Fixes" tag.
But I guess we can also add it (even if it is not really a fix).

We should probably Cc 'stable' ML on the 3 patches but we never really
did before.

Up to you, if you prefer, I can add it :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net v2 1/3] mptcp: remove MPTCP 'ifdef' in TCP SYN cookies

[Mat Martineau]

On Tue, 6 Dec 2022, Matthieu Baerts wrote:

> Hi Mat,
>
> On 05/12/2022 22:52, Mat Martineau wrote:
>> On Wed, 30 Nov 2022, Matthieu Baerts wrote:
>>
>>> To ease the maintenance, it is often recommended to avoid having #ifdef
>>> preprocessor conditions.
>>>
>>> Here the section related to CONFIG_MPTCP was quite short but the next
>>> commit needs to add more code around. It is then cleaner to move
>>> specific MPTCP code to functions located in net/mptcp directory.
>>>
>>> Now that mptcp_subflow_request_sock_ops structure can be static, it can
>>> also be marked as "read only after init".
>>>
>>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>
>> Hi Matthieu -
>>
>> I noticed today that there's no Fixes: tag for this commit, which the
>> netdev CI will check for. Like you say in the commit message, this patch
>> is a small bit of refactoring to prepare for the following patch(es) -
>> should it share Fixes: tags with those or just let the git dependencies
>> speak for themselves?
>
> I don't think we need to add a Fixes tag because as you said, it is
> mentioned this modification is needed for the next patch. I'm sure
> netdev maintainers will understand why it doesn't have a "Fixes" tag.
> But I guess we can also add it (even if it is not really a fix).
>
> We should probably Cc 'stable' ML on the 3 patches but we never really
> did before.
>
> Up to you, if you prefer, I can add it :)
>

I think it's ok to try without it, not every patch merged to -net has one.

Regarding cc: to stable, I've been trying to do that for patches we do 
want backported. We don't want that tag included when sending to mptcp-net 
on our ML as git-send-email would spam the stable list, so I've been 
adding them to the .patch files before upstreaming to netdev. Adding the 
Cc: stable tag in the export branch would probably be ok as long as we're 
careful in other cases where the patches are emailed (when not 
upstreaming).

--
Mat Martineau
Intel

Re: [PATCH mptcp-net v2 1/3] mptcp: remove MPTCP 'ifdef' in TCP SYN cookies

[Matthieu Baerts]

Hi Mat,

On 06/12/2022 18:20, Mat Martineau wrote:
> On Tue, 6 Dec 2022, Matthieu Baerts wrote:
> 
>> Hi Mat,
>>
>> On 05/12/2022 22:52, Mat Martineau wrote:
>>> On Wed, 30 Nov 2022, Matthieu Baerts wrote:
>>>
>>>> To ease the maintenance, it is often recommended to avoid having #ifdef
>>>> preprocessor conditions.
>>>>
>>>> Here the section related to CONFIG_MPTCP was quite short but the next
>>>> commit needs to add more code around. It is then cleaner to move
>>>> specific MPTCP code to functions located in net/mptcp directory.
>>>>
>>>> Now that mptcp_subflow_request_sock_ops structure can be static, it can
>>>> also be marked as "read only after init".
>>>>
>>>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>
>>> Hi Matthieu -
>>>
>>> I noticed today that there's no Fixes: tag for this commit, which the
>>> netdev CI will check for. Like you say in the commit message, this patch
>>> is a small bit of refactoring to prepare for the following patch(es) -
>>> should it share Fixes: tags with those or just let the git dependencies
>>> speak for themselves?
>>
>> I don't think we need to add a Fixes tag because as you said, it is
>> mentioned this modification is needed for the next patch. I'm sure
>> netdev maintainers will understand why it doesn't have a "Fixes" tag.
>> But I guess we can also add it (even if it is not really a fix).
>>
>> We should probably Cc 'stable' ML on the 3 patches but we never really
>> did before.
>>
>> Up to you, if you prefer, I can add it :)
>>
> 
> I think it's ok to try without it, not every patch merged to -net has one.

Sounds good to me. We will see if they ask to always have it. We can
also explicitly ask the question when sending the patches, just to be sure.

> Regarding cc: to stable, I've been trying to do that for patches we do
> want backported. We don't want that tag included when sending to
> mptcp-net on our ML as git-send-email would spam the stable list, so
> I've been adding them to the .patch files before upstreaming to netdev.
> Adding the Cc: stable tag in the export branch would probably be ok as
> long as we're careful in other cases where the patches are emailed (when
> not upstreaming).

It makes sense to add then when upstreaming them. I just added an item
in the list there:

https://github.com/multipath-tcp/mptcp_net-next/blob/scripts/utils/README.md

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-net v2 2/3] mptcp: dedicated request sock for subflow in v6

[Matthieu Baerts]

tcp_request_sock_ops structure is specific to IPv4. It should then not
be used with MPTCP subflows on top of IPv6.

For example, it contains the 'family' field, initialised to AF_INET.
This 'family' field is used by TCP FastOpen code to generate the cookie
but also by TCP Metrics, SELinux and SYN Cookies. Using the wrong family
will not lead to crashes but displaying/using/checking wrong things.

Note that 'send_reset' callback from request_sock_ops structure is used
in some error paths. It is then also important to use the correct one
for IPv4 or IPv6.

The slab name can also be different in IPv4 and IPv6, it will be used
when printing some log messages. The slab pointer will anyway be the
same because the object size is the same for both v4 and v6. A
BUILD_BUG_ON() has also been added to make sure this size is the same.

Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/subflow.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3f670f2d5c5c..30524dd7d0ec 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -529,7 +529,7 @@ static int subflow_v6_rebuild_header(struct sock *sk)
 }
 #endif
 
-static struct request_sock_ops mptcp_subflow_request_sock_ops __ro_after_init;
+static struct request_sock_ops mptcp_subflow_v4_request_sock_ops __ro_after_init;
 static struct tcp_request_sock_ops subflow_request_sock_ipv4_ops __ro_after_init;
 
 static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
@@ -542,7 +542,7 @@ static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	if (skb_rtable(skb)->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
 		goto drop;
 
-	return tcp_conn_request(&mptcp_subflow_request_sock_ops,
+	return tcp_conn_request(&mptcp_subflow_v4_request_sock_ops,
 				&subflow_request_sock_ipv4_ops,
 				sk, skb);
 drop:
@@ -551,6 +551,7 @@ static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 }
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
+static struct request_sock_ops mptcp_subflow_v6_request_sock_ops __ro_after_init;
 static struct tcp_request_sock_ops subflow_request_sock_ipv6_ops __ro_after_init;
 static struct inet_connection_sock_af_ops subflow_v6_specific __ro_after_init;
 static struct inet_connection_sock_af_ops subflow_v6m_specific __ro_after_init;
@@ -573,7 +574,7 @@ static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 		return 0;
 	}
 
-	return tcp_conn_request(&mptcp_subflow_request_sock_ops,
+	return tcp_conn_request(&mptcp_subflow_v6_request_sock_ops,
 				&subflow_request_sock_ipv6_ops, sk, skb);
 
 drop:
@@ -586,7 +587,12 @@ struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *op
 					       struct sock *sk_listener,
 					       bool attach_listener)
 {
-	ops = &mptcp_subflow_request_sock_ops;
+	if (ops->family == AF_INET)
+		ops = &mptcp_subflow_v4_request_sock_ops;
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	else if (ops->family == AF_INET6)
+		ops = &mptcp_subflow_v6_request_sock_ops;
+#endif
 
 	return inet_reqsk_alloc(ops, sk_listener, attach_listener);
 }
@@ -1914,7 +1920,6 @@ static struct tcp_ulp_ops subflow_ulp_ops __read_mostly = {
 static int subflow_ops_init(struct request_sock_ops *subflow_ops)
 {
 	subflow_ops->obj_size = sizeof(struct mptcp_subflow_request_sock);
-	subflow_ops->slab_name = "request_sock_subflow";
 
 	subflow_ops->slab = kmem_cache_create(subflow_ops->slab_name,
 					      subflow_ops->obj_size, 0,
@@ -1931,9 +1936,10 @@ static int subflow_ops_init(struct request_sock_ops *subflow_ops)
 
 void __init mptcp_subflow_init(void)
 {
-	mptcp_subflow_request_sock_ops = tcp_request_sock_ops;
-	if (subflow_ops_init(&mptcp_subflow_request_sock_ops) != 0)
-		panic("MPTCP: failed to init subflow request sock ops\n");
+	mptcp_subflow_v4_request_sock_ops = tcp_request_sock_ops;
+	mptcp_subflow_v4_request_sock_ops.slab_name = "request_sock_subflow_v4";
+	if (subflow_ops_init(&mptcp_subflow_v4_request_sock_ops) != 0)
+		panic("MPTCP: failed to init subflow v4 request sock ops\n");
 
 	subflow_request_sock_ipv4_ops = tcp_request_sock_ipv4_ops;
 	subflow_request_sock_ipv4_ops.route_req = subflow_v4_route_req;
@@ -1948,6 +1954,18 @@ void __init mptcp_subflow_init(void)
 	tcp_prot_override.release_cb = tcp_release_cb_override;
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	/* In struct mptcp_subflow_request_sock, we assume the TCP request sock
+	 * structures for v4 and v6 have the same size. It should not changed in
+	 * the future but better to make sure to be warned if it is no longer
+	 * the case.
+	 */
+	BUILD_BUG_ON(sizeof(struct tcp_request_sock) != sizeof(struct tcp6_request_sock));
+
+	mptcp_subflow_v6_request_sock_ops = tcp6_request_sock_ops;
+	mptcp_subflow_v6_request_sock_ops.slab_name = "request_sock_subflow_v6";
+	if (subflow_ops_init(&mptcp_subflow_v6_request_sock_ops) != 0)
+		panic("MPTCP: failed to init subflow v6 request sock ops\n");
+
 	subflow_request_sock_ipv6_ops = tcp_request_sock_ipv6_ops;
 	subflow_request_sock_ipv6_ops.route_req = subflow_v6_route_req;
 
-- 
2.37.2

[PATCH mptcp-net v2 3/3] mptcp: use proper req destructor for IPv6

[Matthieu Baerts]

Before, only the destructor from TCP request sock in IPv4 was called
even if the subflow was in v4.

It is important to use the right destructor to avoid memory leaks with
some advanced IPv6 features, e.g. when the request socks contain
specific IPv6 options.

Fixes: 79c0949e9a09 ("mptcp: Add key generation and token tree")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/subflow.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 30524dd7d0ec..613f515fedf0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -45,7 +45,6 @@ static void subflow_req_destructor(struct request_sock *req)
 		sock_put((struct sock *)subflow_req->msk);
 
 	mptcp_token_destroy_request(req);
-	tcp_request_sock_ops.destructor(req);
 }
 
 static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
@@ -550,6 +549,12 @@ static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	return 0;
 }
 
+static void subflow_v4_req_destructor(struct request_sock *req)
+{
+	subflow_req_destructor(req);
+	tcp_request_sock_ops.destructor(req);
+}
+
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 static struct request_sock_ops mptcp_subflow_v6_request_sock_ops __ro_after_init;
 static struct tcp_request_sock_ops subflow_request_sock_ipv6_ops __ro_after_init;
@@ -581,6 +586,12 @@ static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	tcp_listendrop(sk);
 	return 0; /* don't send reset */
 }
+
+static void subflow_v6_req_destructor(struct request_sock *req)
+{
+	subflow_req_destructor(req);
+	tcp6_request_sock_ops.destructor(req);
+}
 #endif
 
 struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
@@ -1929,8 +1940,6 @@ static int subflow_ops_init(struct request_sock_ops *subflow_ops)
 	if (!subflow_ops->slab)
 		return -ENOMEM;
 
-	subflow_ops->destructor = subflow_req_destructor;
-
 	return 0;
 }
 
@@ -1938,6 +1947,8 @@ void __init mptcp_subflow_init(void)
 {
 	mptcp_subflow_v4_request_sock_ops = tcp_request_sock_ops;
 	mptcp_subflow_v4_request_sock_ops.slab_name = "request_sock_subflow_v4";
+	mptcp_subflow_v4_request_sock_ops.destructor = subflow_v4_req_destructor;
+
 	if (subflow_ops_init(&mptcp_subflow_v4_request_sock_ops) != 0)
 		panic("MPTCP: failed to init subflow v4 request sock ops\n");
 
@@ -1963,6 +1974,8 @@ void __init mptcp_subflow_init(void)
 
 	mptcp_subflow_v6_request_sock_ops = tcp6_request_sock_ops;
 	mptcp_subflow_v6_request_sock_ops.slab_name = "request_sock_subflow_v6";
+	mptcp_subflow_v6_request_sock_ops.destructor = subflow_v6_req_destructor;
+
 	if (subflow_ops_init(&mptcp_subflow_v6_request_sock_ops) != 0)
 		panic("MPTCP: failed to init subflow v6 request sock ops\n");
 
-- 
2.37.2

Re: mptcp: use proper req destructor for IPv6: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5358507365498880
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5358507365498880/summary/summary.txt

- KVM Validation: debug:
  - Critical: Global Timeout ❌:
  - Task: https://cirrus-ci.com/task/6484407272341504
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6484407272341504/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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: mptcp: use proper req destructor for IPv6: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5358507365498880
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5358507365498880/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6128572050440192
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6128572050440192/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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: [PATCH mptcp-net v2 3/3] mptcp: use proper req destructor for IPv6

[Mat Martineau]

On Wed, 30 Nov 2022, Matthieu Baerts wrote:

> Before, only the destructor from TCP request sock in IPv4 was called
> even if the subflow was in v4.

Did you intend "even if the subflow was IPv6." ?

Other than that, the series looks good to me. Fine if you fix the above 
when applying.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

>
> It is important to use the right destructor to avoid memory leaks with
> some advanced IPv6 features, e.g. when the request socks contain
> specific IPv6 options.
>
> Fixes: 79c0949e9a09 ("mptcp: Add key generation and token tree")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> net/mptcp/subflow.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 30524dd7d0ec..613f515fedf0 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -45,7 +45,6 @@ static void subflow_req_destructor(struct request_sock *req)
> 		sock_put((struct sock *)subflow_req->msk);
>
> 	mptcp_token_destroy_request(req);
> -	tcp_request_sock_ops.destructor(req);
> }
>
> static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
> @@ -550,6 +549,12 @@ static int subflow_v4_conn_request(struct sock *sk, struct sk_buff *skb)
> 	return 0;
> }
>
> +static void subflow_v4_req_destructor(struct request_sock *req)
> +{
> +	subflow_req_destructor(req);
> +	tcp_request_sock_ops.destructor(req);
> +}
> +
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> static struct request_sock_ops mptcp_subflow_v6_request_sock_ops __ro_after_init;
> static struct tcp_request_sock_ops subflow_request_sock_ipv6_ops __ro_after_init;
> @@ -581,6 +586,12 @@ static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
> 	tcp_listendrop(sk);
> 	return 0; /* don't send reset */
> }
> +
> +static void subflow_v6_req_destructor(struct request_sock *req)
> +{
> +	subflow_req_destructor(req);
> +	tcp6_request_sock_ops.destructor(req);
> +}
> #endif
>
> struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
> @@ -1929,8 +1940,6 @@ static int subflow_ops_init(struct request_sock_ops *subflow_ops)
> 	if (!subflow_ops->slab)
> 		return -ENOMEM;
>
> -	subflow_ops->destructor = subflow_req_destructor;
> -
> 	return 0;
> }
>
> @@ -1938,6 +1947,8 @@ void __init mptcp_subflow_init(void)
> {
> 	mptcp_subflow_v4_request_sock_ops = tcp_request_sock_ops;
> 	mptcp_subflow_v4_request_sock_ops.slab_name = "request_sock_subflow_v4";
> +	mptcp_subflow_v4_request_sock_ops.destructor = subflow_v4_req_destructor;
> +
> 	if (subflow_ops_init(&mptcp_subflow_v4_request_sock_ops) != 0)
> 		panic("MPTCP: failed to init subflow v4 request sock ops\n");
>
> @@ -1963,6 +1974,8 @@ void __init mptcp_subflow_init(void)
>
> 	mptcp_subflow_v6_request_sock_ops = tcp6_request_sock_ops;
> 	mptcp_subflow_v6_request_sock_ops.slab_name = "request_sock_subflow_v6";
> +	mptcp_subflow_v6_request_sock_ops.destructor = subflow_v6_req_destructor;
> +
> 	if (subflow_ops_init(&mptcp_subflow_v6_request_sock_ops) != 0)
> 		panic("MPTCP: failed to init subflow v6 request sock ops\n");
>
> -- 
> 2.37.2
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net v2 3/3] mptcp: use proper req destructor for IPv6

[Matthieu Baerts]

Hi Mat,

On 02/12/2022 23:56, Mat Martineau wrote:
> On Wed, 30 Nov 2022, Matthieu Baerts wrote:
> 
>> Before, only the destructor from TCP request sock in IPv4 was called
>> even if the subflow was in v4.
> 
> Did you intend "even if the subflow was IPv6." ?

Oops, good catch, thank you, fixed now! (s/in v4/IPv6/)

> Other than that, the series looks good to me. Fine if you fix the above
> when applying.
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the review!

Now in our tree (fixes for -net) with Mat's RvB tag:


New patches for t/upstream-net & t/upstream:
- 5819cf24db5f: mptcp: remove MPTCP 'ifdef' in TCP SYN cookies
- 46b00edc3b54: mptcp: dedicated request sock for subflow in v6
- 2df171723dbb: mptcp: use proper req destructor for IPv6
- Results: f48d2409e825..81092f16490f (export-net)
- Results: 9324c815f96d..ec528d5ae7e7 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20221205T102301
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20221205T102301

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: mptcp: use proper req destructor for IPv6: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/4825337440239616
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4825337440239616/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6514187300503552
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6514187300503552/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5951237347082240
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5951237347082240/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5071165463461888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5071165463461888/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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)