[MPTCP] [RFC v2 03/10] mptcp: token: rename token_join_response

[MPTCP] [RFC v2 03/10] mptcp: token: rename token_join_response

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]

This function validates the truncated hmac and computes a hmac for use
in the ack packet.  Rename it, place it where its used, and do the
hmac computation in the caller.

This also adds a pr_fmt specifier, so the included pr_debug will
auto-gain 'MPTCP' prefix.

v2: change name and place hmac calculation in the caller (Peter)
    add pr_fmt too
    squash two pr_debug() into one.

Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 net/mptcp/protocol.h |  1 -
 net/mptcp/subflow.c  | 34 ++++++++++++++++++++++++++++++----
 net/mptcp/token.c    | 31 -------------------------------
 3 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1655283dbd6a..89bd68f85856 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -227,7 +227,6 @@ void mptcp_finish_join(struct sock *sk);
 
 void token_new_request(struct request_sock *req, const struct sk_buff *skb);
 int token_join_request(struct request_sock *req, const struct sk_buff *skb);
-int token_join_response(struct sock *sk);
 int token_join_valid(struct request_sock *req,
 		     struct tcp_options_received *rx_opt);
 void token_destroy_request(u32 token);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index fc507e091cf5..8a8109466487 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -4,6 +4,8 @@
  * Copyright (c) 2017 - 2019, Intel Corporation.
  */
 
+#define pr_fmt(fmt) "MPTCP: " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -96,6 +98,25 @@ static void subflow_v4_init_req(struct request_sock *req,
 	}
 }
 
+/* validate received truncated hmac and create hmac for third ACK */
+static bool subflow_thmac_valid(struct subflow_context *subflow)
+{
+	u8 hmac[MPTCPOPT_HMAC_LEN];
+	u64 thmac;
+
+	crypto_hmac_sha1(subflow->remote_key, subflow->local_key,
+			 subflow->remote_nonce, subflow->local_nonce,
+			 (u32 *)hmac);
+
+	thmac = get_unaligned_be64(hmac);
+	pr_debug("subflow=%p, token=%u, thmac=%llu, subflow->thmac=%llu\n",
+		 subflow, subflow->token,
+		 (unsigned long long)thmac,
+		 (unsigned long long)subflow->thmac);
+
+	return thmac == subflow->thmac;
+}
+
 static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 {
 	struct subflow_context *subflow = subflow_ctx(sk);
@@ -119,13 +140,18 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u",
 			 subflow_ctx(sk), subflow->thmac,
 			 subflow->remote_nonce);
-		if (token_join_response(sk)) {
+		if (!subflow_thmac_valid(subflow)) {
 			subflow->mp_join = 0;
 			// @@ need to trigger RST
-		} else {
-			mptcp_finish_join(sk);
-			subflow->conn_finished = 1;
+			return;
 		}
+
+		crypto_hmac_sha1(subflow->local_key, subflow->remote_key,
+				 subflow->local_nonce, subflow->remote_nonce,
+				 (u32 *)subflow->hmac);
+
+		mptcp_finish_join(sk);
+		subflow->conn_finished = 1;
 	}
 }
 
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index b63ce35c9ed3..3163832d8ebf 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -93,28 +93,6 @@ static void new_req_join(struct request_sock *req, struct sock *sk,
 		 subflow_req->thmac);
 }
 
-static int new_rsp_join(struct sock *sk)
-{
-	struct subflow_context *subflow = subflow_ctx(sk);
-	u8 hmac[MPTCPOPT_HMAC_LEN];
-	u64 thmac;
-
-	crypto_hmac_sha1(subflow->remote_key, subflow->local_key,
-			 subflow->remote_nonce, subflow->local_nonce,
-			 (u32 *)hmac);
-
-	thmac = get_unaligned_be64(hmac);
-	pr_debug("thmac=%llu", thmac);
-	if (thmac != subflow->thmac)
-		return -1;
-
-	crypto_hmac_sha1(subflow->local_key, subflow->remote_key,
-			 subflow->local_nonce, subflow->remote_nonce,
-			 (u32 *)subflow->hmac);
-
-	return 0;
-}
-
 static int new_join_valid(struct request_sock *req, struct sock *sk,
 			  struct tcp_options_received *rx_opt)
 {
@@ -239,15 +217,6 @@ int token_join_request(struct request_sock *req, const struct sk_buff *skb)
 	return 0;
 }
 
-/* validate received truncated hmac and create hmac for third ACK */
-int token_join_response(struct sock *sk)
-{
-	struct subflow_context *subflow = subflow_ctx(sk);
-
-	pr_debug("subflow=%p, token=%u", subflow, subflow->token);
-	return new_rsp_join(sk);
-}
-
 /* validate hmac received in third ACK */
 int token_join_valid(struct request_sock *req,
 		     struct tcp_options_received *rx_opt)
-- 
2.21.0

Re: [MPTCP] [RFC v2 03/10] mptcp: token: rename token_join_response

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

Hi Florian,

On 29/08/2019 11:06, Florian Westphal wrote:
> This function validates the truncated hmac and computes a hmac for use
> in the ack packet.  Rename it, place it where its used, and do the
> hmac computation in the caller.
> 
> This also adds a pr_fmt specifier, so the included pr_debug will
> auto-gain 'MPTCP' prefix.
> 
> v2: change name and place hmac calculation in the caller (Peter)
>     add pr_fmt too
>     squash two pr_debug() into one.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.h |  1 -
>  net/mptcp/subflow.c  | 34 ++++++++++++++++++++++++++++++----
>  net/mptcp/token.c    | 31 -------------------------------
>  3 files changed, 30 insertions(+), 36 deletions(-)
> 
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 1655283dbd6a..89bd68f85856 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -227,7 +227,6 @@ void mptcp_finish_join(struct sock *sk);
>  
>  void token_new_request(struct request_sock *req, const struct sk_buff *skb);
>  int token_join_request(struct request_sock *req, const struct sk_buff *skb);
> -int token_join_response(struct sock *sk);
>  int token_join_valid(struct request_sock *req,
>  		     struct tcp_options_received *rx_opt);
>  void token_destroy_request(u32 token);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index fc507e091cf5..8a8109466487 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -4,6 +4,8 @@
>   * Copyright (c) 2017 - 2019, Intel Corporation.
>   */
>  
> +#define pr_fmt(fmt) "MPTCP: " fmt

Good idea to add this!

A small detail: should we also add "subflow:" (or "sf:")?

    "MPTCP:sf: "

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [RFC v2 03/10] mptcp: token: rename token_join_response

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> A small detail: should we also add "subflow:" (or "sf:")?
> 
>     "MPTCP:sf: "

I modeled it after what is done for builtin code in net/, e.g.:

net/ipv4/ip_fragment.c:#define pr_fmt(fmt) "IPv4: " fmt
net/ipv4/ip_input.c:#define pr_fmt(fmt) "IPv4: " fmt
net/ipv4/ip_options.c:#define pr_fmt(fmt) "IPv4: " fmt
net/ipv4/route.c:#define pr_fmt(fmt) "IPv4: " fmt
net/ipv4/tcp.c:#define pr_fmt(fmt) "TCP: " fmt
net/ipv4/tcp_cong.c:#define pr_fmt(fmt) "TCP: " fmt
net/ipv4/tcp_input.c:#define pr_fmt(fmt) "TCP: " fmt
net/ipv4/tcp_ipv4.c:#define pr_fmt(fmt) "TCP: " fmt
net/ipv4/tcp_output.c:#define pr_fmt(fmt) "TCP: " fmt
net/ipv4/udp.c:#define pr_fmt(fmt) "UDP: " fmt
net/ipv4/udplite.c:#define pr_fmt(fmt) "UDPLite: " fmt
net/ipv4/xfrm4_tunnel.c:#define pr_fmt(fmt) "IPsec: " fmt
net/ipv6/addrconf.c:#define pr_fmt(fmt) "IPv6: " fmt
net/ipv6/af_inet6.c:#define pr_fmt(fmt) "IPv6: " fmt
net/ipv6/ah6.c:#define pr_fmt(fmt) "IPv6: " fmt
net/ipv6/esp6.c:#define pr_fmt(fmt) "IPv6: " fmt
...

So single "MPTCP: " seems more consistent with rest of kernel.
I have no strong opinion though, if others want more detailed
prefix too I can change it of course.

Re: [MPTCP] [RFC v2 03/10] mptcp: token: rename token_join_response

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

Hi Florian,

On 29/08/2019 12:19, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> A small detail: should we also add "subflow:" (or "sf:")?
>>
>>     "MPTCP:sf: "
> 
> I modeled it after what is done for builtin code in net/, e.g.:

Thank you for having looking at that!

> net/ipv4/ip_fragment.c:#define pr_fmt(fmt) "IPv4: " fmt
> net/ipv4/ip_input.c:#define pr_fmt(fmt) "IPv4: " fmt
> net/ipv4/ip_options.c:#define pr_fmt(fmt) "IPv4: " fmt
> net/ipv4/route.c:#define pr_fmt(fmt) "IPv4: " fmt
> net/ipv4/tcp.c:#define pr_fmt(fmt) "TCP: " fmt
> net/ipv4/tcp_cong.c:#define pr_fmt(fmt) "TCP: " fmt
> net/ipv4/tcp_input.c:#define pr_fmt(fmt) "TCP: " fmt
> net/ipv4/tcp_ipv4.c:#define pr_fmt(fmt) "TCP: " fmt
> net/ipv4/tcp_output.c:#define pr_fmt(fmt) "TCP: " fmt
> net/ipv4/udp.c:#define pr_fmt(fmt) "UDP: " fmt
> net/ipv4/udplite.c:#define pr_fmt(fmt) "UDPLite: " fmt
> net/ipv4/xfrm4_tunnel.c:#define pr_fmt(fmt) "IPsec: " fmt
> net/ipv6/addrconf.c:#define pr_fmt(fmt) "IPv6: " fmt
> net/ipv6/af_inet6.c:#define pr_fmt(fmt) "IPv6: " fmt
> net/ipv6/ah6.c:#define pr_fmt(fmt) "IPv6: " fmt
> net/ipv6/esp6.c:#define pr_fmt(fmt) "IPv6: " fmt
> ...
> 
> So single "MPTCP: " seems more consistent with rest of kernel.
> I have no strong opinion though, if others want more detailed
> prefix too I can change it of course.

I also don't have a strong opinion on that, I am fine to follow what
seems to be the standard :-)

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium