[PATCH mptcp-next 0/4] reflect mptcp_pm_add_addr_signal

[PATCH mptcp-next 0/4] reflect mptcp_pm_add_addr_signal

[Geliang Tang]

mptcp_pm_add_addr_signal() has too many parameters right now. This issue
was introduced by me and I promised to fix it last year:

https://patchwork.kernel.org/project/mptcp/patch/f0920ba1126ff81aa1acbff47e805e5573c64abc.1626158123.git.geliangtang@gmail.com/

This patch set reflected this function and dropped the port and echo
parameters.

As defined in $3.4.1:
                       1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
  +---------------+---------------+-------+-------+---------------+
  |     Kind      |     Length    |Subtype|(rsv)|E|  Address ID   |
  +---------------+---------------+-------+-------+---------------+
  |           Address (IPv4: 4 octets / IPv6: 16 octets)          |
  +-------------------------------+-------------------------------+
  |   Port (2 octets, optional)   |                               |
  +-------------------------------+                               |
  |                Truncated HMAC (8 octets, if E=0)              |
  |                               +-------------------------------+
  |                               |
  +-------------------------------+

The echo bit is one of the important properties of a MPTCP address. It
makes sense to put it into struct mptcp_addr_info with others properties
like the address family, the id number and the port number too. So patch
2 added the echo bit in struct mptcp_addr_info. With this change, we can
drop the echo parameter of mptcp_pm_announce_addr too, or even drop the
addr_signal status MPTCP_ADD_ADDR_ECHO later (not finish yet).

Geliang Tang (4):
  mptcp: drop port parameter of mptcp_pm_add_addr_signal
  mptcp: add echo bit in mptcp_addr_info
  mptcp: drop echo parameter of mptcp_pm_add_addr_signal
  mptcp: drop echo parameter of mptcp_pm_announce_addr

 include/net/mptcp.h    |  3 ++-
 net/mptcp/options.c    | 26 +++++++++++++-------------
 net/mptcp/pm.c         | 29 +++++++++++++++--------------
 net/mptcp/pm_netlink.c |  6 +++---
 net/mptcp/protocol.h   |  8 +++-----
 5 files changed, 36 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH mptcp-next 1/4] mptcp: drop port parameter of mptcp_pm_add_addr_signal

[Geliang Tang]

Drop the port parameter of mptcp_pm_add_addr_signal() and reflect it to
avoid passing too many parameters.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c  | 5 ++---
 net/mptcp/pm.c       | 7 ++++---
 net/mptcp/protocol.h | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 7b615dc10897..4e516e88ab88 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -652,7 +652,6 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	bool drop_other_suboptions = false;
 	unsigned int opt_size = *size;
 	bool echo;
-	bool port;
 	int len;
 
 	/* add addr will strip the existing options, be sure to avoid breaking
@@ -661,12 +660,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	if (!mptcp_pm_should_add_signal(msk) ||
 	    (opts->suboptions & (OPTION_MPTCP_MPJ_ACK | OPTION_MPTCP_MPC_ACK)) ||
 	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
-		    &echo, &port, &drop_other_suboptions))
+		    &echo, &drop_other_suboptions))
 		return false;
 
 	if (drop_other_suboptions)
 		remaining += opt_size;
-	len = mptcp_add_addr_len(opts->addr.family, echo, port);
+	len = mptcp_add_addr_len(opts->addr.family, echo, !!opts->addr.port);
 	if (remaining < len)
 		return false;
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1f8878cc29e3..99db7270e461 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -284,11 +284,12 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
 			      struct mptcp_addr_info *addr, bool *echo,
-			      bool *port, bool *drop_other_suboptions)
+			      bool *drop_other_suboptions)
 {
 	int ret = false;
 	u8 add_addr;
 	u8 family;
+	bool port;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -306,10 +307,10 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 	}
 
 	*echo = mptcp_pm_should_add_signal_echo(msk);
-	*port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
+	port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
 
 	family = *echo ? msk->pm.remote.family : msk->pm.local.family;
-	if (remaining < mptcp_add_addr_len(family, *echo, *port))
+	if (remaining < mptcp_add_addr_len(family, *echo, port))
 		goto out_unlock;
 
 	if (*echo) {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f37f087caab3..0eebfc9f39bc 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -835,7 +835,7 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
 			      struct mptcp_addr_info *addr, bool *echo,
-			      bool *port, bool *drop_other_suboptions);
+			      bool *drop_other_suboptions);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-- 
2.34.1

[PATCH mptcp-next 2/4] mptcp: add echo bit in mptcp_addr_info

[Geliang Tang]

The echo bit is one of the important properties of a MPTCP address. It
makes sense to put it into struct mptcp_addr_info with others properties
like the address family, the id number and the port number too.

This patch added the echo bit in struct mptcp_addr_info. Use this instead
of using the struct member echo in struct mptcp_options_received.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/net/mptcp.h  |  3 ++-
 net/mptcp/options.c  | 16 +++++++++-------
 net/mptcp/protocol.h |  1 -
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 8b1afd6f5cc4..53f66e1ca4fc 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -48,7 +48,8 @@ struct mptcp_rm_list {
 
 struct mptcp_addr_info {
 	u8			id;
-	sa_family_t		family;
+	u8			echo:1,
+				family:4;
 	__be16			port;
 	union {
 		struct in_addr	addr;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4e516e88ab88..4070a9104386 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -234,8 +234,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_ADD_ADDR:
-		mp_opt->echo = (*ptr++) & MPTCP_ADDR_ECHO;
-		if (!mp_opt->echo) {
+		mp_opt->addr.echo = (*ptr++) & MPTCP_ADDR_ECHO;
+		if (!mp_opt->addr.echo) {
 			if (opsize == TCPOLEN_MPTCP_ADD_ADDR ||
 			    opsize == TCPOLEN_MPTCP_ADD_ADDR_PORT)
 				mp_opt->addr.family = AF_INET;
@@ -283,13 +283,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			}
 		}
 #endif
-		if (!mp_opt->echo) {
+		if (!mp_opt->addr.echo) {
 			mp_opt->ahmac = get_unaligned_be64(ptr);
 			ptr += 8;
 		}
 		pr_debug("ADD_ADDR%s: id=%d, ahmac=%llu, echo=%d, port=%d",
 			 (mp_opt->addr.family == AF_INET6) ? "6" : "",
-			 mp_opt->addr.id, mp_opt->ahmac, mp_opt->echo, ntohs(mp_opt->addr.port));
+			 mp_opt->addr.id, mp_opt->ahmac,
+			 mp_opt->addr.echo, ntohs(mp_opt->addr.port));
 		break;
 
 	case MPTCPOPT_RM_ADDR:
@@ -945,7 +946,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	}
 
 	if (((mp_opt->suboptions & OPTION_MPTCP_DSS) && mp_opt->use_ack) ||
-	    ((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) && !mp_opt->echo)) {
+	    ((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) && !mp_opt->addr.echo)) {
 		/* subflows are fully established as soon as we get any
 		 * additional ack, including ADD_ADDR.
 		 */
@@ -1076,7 +1077,7 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk,
 {
 	u64 hmac = 0;
 
-	if (mp_opt->echo)
+	if (mp_opt->addr.echo)
 		return true;
 
 	hmac = add_addr_generate_hmac(msk->remote_key,
@@ -1129,7 +1130,8 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 
 		if ((mp_opt.suboptions & OPTION_MPTCP_ADD_ADDR) &&
 		    add_addr_hmac_valid(msk, &mp_opt)) {
-			if (!mp_opt.echo) {
+			if (!mp_opt.addr.echo) {
+				mp_opt.addr.echo = 1;
 				mptcp_pm_add_addr_received(msk, &mp_opt.addr);
 				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
 			} else {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 0eebfc9f39bc..4becac89a4a6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -151,7 +151,6 @@ struct mptcp_options_received {
 		mpc_map:1,
 		reset_reason:4,
 		reset_transient:1,
-		echo:1,
 		backup:1,
 		deny_join_id0:1,
 		__unused:2;
-- 
2.34.1

[PATCH mptcp-next 3/4] mptcp: drop echo parameter of mptcp_pm_add_addr_signal

[Geliang Tang]

Drop the echo parameter of mptcp_pm_add_addr_signal() and reflect it to
avoid passing too many parameters.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c  |  9 ++++-----
 net/mptcp/pm.c       | 13 +++++++------
 net/mptcp/protocol.h |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4070a9104386..9d7228f12473 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -652,7 +652,6 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 	bool drop_other_suboptions = false;
 	unsigned int opt_size = *size;
-	bool echo;
 	int len;
 
 	/* add addr will strip the existing options, be sure to avoid breaking
@@ -661,12 +660,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	if (!mptcp_pm_should_add_signal(msk) ||
 	    (opts->suboptions & (OPTION_MPTCP_MPJ_ACK | OPTION_MPTCP_MPC_ACK)) ||
 	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
-		    &echo, &drop_other_suboptions))
+		    &drop_other_suboptions))
 		return false;
 
 	if (drop_other_suboptions)
 		remaining += opt_size;
-	len = mptcp_add_addr_len(opts->addr.family, echo, !!opts->addr.port);
+	len = mptcp_add_addr_len(opts->addr.family, opts->addr.echo, !!opts->addr.port);
 	if (remaining < len)
 		return false;
 
@@ -684,13 +683,13 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 		*size -= opt_size;
 	}
 	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
-	if (!echo) {
+	if (!opts->addr.echo) {
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
 						     msk->remote_key,
 						     &opts->addr);
 	}
 	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
-		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+		 opts->addr.id, opts->ahmac, opts->addr.echo, ntohs(opts->addr.port));
 
 	return true;
 }
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 99db7270e461..8dd78424f86e 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -283,13 +283,14 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
-			      struct mptcp_addr_info *addr, bool *echo,
+			      struct mptcp_addr_info *addr,
 			      bool *drop_other_suboptions)
 {
 	int ret = false;
 	u8 add_addr;
 	u8 family;
 	bool port;
+	bool echo;
 
 	spin_lock_bh(&msk->pm.lock);
 
@@ -306,14 +307,14 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 		*drop_other_suboptions = true;
 	}
 
-	*echo = mptcp_pm_should_add_signal_echo(msk);
-	port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
+	echo = mptcp_pm_should_add_signal_echo(msk);
+	port = !!(echo ? msk->pm.remote.port : msk->pm.local.port);
 
-	family = *echo ? msk->pm.remote.family : msk->pm.local.family;
-	if (remaining < mptcp_add_addr_len(family, *echo, port))
+	family = echo ? msk->pm.remote.family : msk->pm.local.family;
+	if (remaining < mptcp_add_addr_len(family, echo, port))
 		goto out_unlock;
 
-	if (*echo) {
+	if (echo) {
 		*addr = msk->pm.remote;
 		add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
 	} else {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4becac89a4a6..7ee6a39a7ff9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -833,7 +833,7 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
 
 bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
 			      unsigned int opt_size, unsigned int remaining,
-			      struct mptcp_addr_info *addr, bool *echo,
+			      struct mptcp_addr_info *addr,
 			      bool *drop_other_suboptions);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
-- 
2.34.1

[PATCH mptcp-next 4/4] mptcp: drop echo parameter of mptcp_pm_announce_addr

[Geliang Tang]

Since the echo bit is added in struct mptcp_addr_info, use it in
mptcp_pm_announce_addr() instead of passing the echo parameter.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c         | 13 ++++++-------
 net/mptcp/pm_netlink.c |  6 +++---
 net/mptcp/protocol.h   |  3 +--
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 8dd78424f86e..0bd8af167d65 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -15,22 +15,21 @@
 /* path manager command handlers */
 
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
-			   const struct mptcp_addr_info *addr,
-			   bool echo)
+			   const struct mptcp_addr_info *addr)
 {
 	u8 add_addr = READ_ONCE(msk->pm.addr_signal);
 
-	pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo);
+	pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, addr->echo);
 
 	lockdep_assert_held(&msk->pm.lock);
 
 	if (add_addr &
-	    (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
-		pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
+	    (addr->echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
+		pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, addr->echo);
 		return -EINVAL;
 	}
 
-	if (echo) {
+	if (addr->echo) {
 		msk->pm.remote = *addr;
 		add_addr |= BIT(MPTCP_ADD_ADDR_ECHO);
 	} else {
@@ -209,7 +208,7 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
 	spin_lock_bh(&pm->lock);
 
 	if (!READ_ONCE(pm->accept_addr) || mptcp_pm_is_userspace(msk)) {
-		mptcp_pm_announce_addr(msk, addr, true);
+		mptcp_pm_announce_addr(msk, addr);
 		mptcp_pm_add_addr_send_ack(msk);
 	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
 		pm->remote = *addr;
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 93800f32fcb6..8fcba8e92b5b 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -326,7 +326,7 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
 
 	if (!mptcp_pm_should_add_signal_addr(msk)) {
 		pr_debug("retransmit ADD_ADDR id=%d", entry->addr.id);
-		mptcp_pm_announce_addr(msk, &entry->addr, false);
+		mptcp_pm_announce_addr(msk, &entry->addr);
 		mptcp_pm_add_addr_send_ack(msk);
 		entry->retrans_times++;
 	}
@@ -550,7 +550,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 			if (mptcp_pm_alloc_anno_list(msk, local)) {
 				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
 				msk->pm.add_addr_signaled++;
-				mptcp_pm_announce_addr(msk, &local->addr, false);
+				mptcp_pm_announce_addr(msk, &local->addr);
 				mptcp_pm_nl_addr_send_ack(msk);
 			}
 		}
@@ -681,7 +681,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	spin_lock_bh(&msk->pm.lock);
 
 add_addr_echo:
-	mptcp_pm_announce_addr(msk, &msk->pm.remote, true);
+	mptcp_pm_announce_addr(msk, &msk->pm.remote);
 	mptcp_pm_nl_addr_send_ack(msk);
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7ee6a39a7ff9..29a8a0b2512b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -772,8 +772,7 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct net *net, unsigned int id,
 					 u8 *flags, int *ifindex);
 
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
-			   const struct mptcp_addr_info *addr,
-			   bool echo);
+			   const struct mptcp_addr_info *addr);
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 
-- 
2.34.1

Re: [PATCH mptcp-next 1/4] mptcp: drop port parameter of mptcp_pm_add_addr_signal

[Mat Martineau]

On Tue, 8 Feb 2022, Geliang Tang wrote:

> Drop the port parameter of mptcp_pm_add_addr_signal() and reflect it to
> avoid passing too many parameters.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>

For this patch, looks like a helpful bit of cleanup. Thanks!

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


> ---
> net/mptcp/options.c  | 5 ++---
> net/mptcp/pm.c       | 7 ++++---
> net/mptcp/protocol.h | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 7b615dc10897..4e516e88ab88 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -652,7 +652,6 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> 	bool drop_other_suboptions = false;
> 	unsigned int opt_size = *size;
> 	bool echo;
> -	bool port;
> 	int len;
>
> 	/* add addr will strip the existing options, be sure to avoid breaking
> @@ -661,12 +660,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> 	if (!mptcp_pm_should_add_signal(msk) ||
> 	    (opts->suboptions & (OPTION_MPTCP_MPJ_ACK | OPTION_MPTCP_MPC_ACK)) ||
> 	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
> -		    &echo, &port, &drop_other_suboptions))
> +		    &echo, &drop_other_suboptions))
> 		return false;
>
> 	if (drop_other_suboptions)
> 		remaining += opt_size;
> -	len = mptcp_add_addr_len(opts->addr.family, echo, port);
> +	len = mptcp_add_addr_len(opts->addr.family, echo, !!opts->addr.port);
> 	if (remaining < len)
> 		return false;
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 1f8878cc29e3..99db7270e461 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -284,11 +284,12 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> 			      unsigned int opt_size, unsigned int remaining,
> 			      struct mptcp_addr_info *addr, bool *echo,
> -			      bool *port, bool *drop_other_suboptions)
> +			      bool *drop_other_suboptions)
> {
> 	int ret = false;
> 	u8 add_addr;
> 	u8 family;
> +	bool port;
>
> 	spin_lock_bh(&msk->pm.lock);
>
> @@ -306,10 +307,10 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> 	}
>
> 	*echo = mptcp_pm_should_add_signal_echo(msk);
> -	*port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> +	port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>
> 	family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> -	if (remaining < mptcp_add_addr_len(family, *echo, *port))
> +	if (remaining < mptcp_add_addr_len(family, *echo, port))
> 		goto out_unlock;
>
> 	if (*echo) {
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f37f087caab3..0eebfc9f39bc 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -835,7 +835,7 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> 			      unsigned int opt_size, unsigned int remaining,
> 			      struct mptcp_addr_info *addr, bool *echo,
> -			      bool *port, bool *drop_other_suboptions);
> +			      bool *drop_other_suboptions);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> 			     struct mptcp_rm_list *rm_list);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next 2/4] mptcp: add echo bit in mptcp_addr_info

[Mat Martineau]

On Tue, 8 Feb 2022, Geliang Tang wrote:

> The echo bit is one of the important properties of a MPTCP address. It
> makes sense to put it into struct mptcp_addr_info with others properties
> like the address family, the id number and the port number too.
>

It seems to me that keeping 'echo' in struct mptcp_options_received or 
passed to the various functions is better. struct mptcp_addr_info is used 
in other places where 'echo' isn't needed, and it's a bit of information 
that's useful only at rx or tx time and not something to store 
permanently.

I'd prefer to drop patches 2-4.

-Mat


> This patch added the echo bit in struct mptcp_addr_info. Use this instead
> of using the struct member echo in struct mptcp_options_received.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> include/net/mptcp.h  |  3 ++-
> net/mptcp/options.c  | 16 +++++++++-------
> net/mptcp/protocol.h |  1 -
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 8b1afd6f5cc4..53f66e1ca4fc 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -48,7 +48,8 @@ struct mptcp_rm_list {
>
> struct mptcp_addr_info {
> 	u8			id;
> -	sa_family_t		family;
> +	u8			echo:1,
> +				family:4;
> 	__be16			port;
> 	union {
> 		struct in_addr	addr;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 4e516e88ab88..4070a9104386 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -234,8 +234,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> 		break;
>
> 	case MPTCPOPT_ADD_ADDR:
> -		mp_opt->echo = (*ptr++) & MPTCP_ADDR_ECHO;
> -		if (!mp_opt->echo) {
> +		mp_opt->addr.echo = (*ptr++) & MPTCP_ADDR_ECHO;
> +		if (!mp_opt->addr.echo) {
> 			if (opsize == TCPOLEN_MPTCP_ADD_ADDR ||
> 			    opsize == TCPOLEN_MPTCP_ADD_ADDR_PORT)
> 				mp_opt->addr.family = AF_INET;
> @@ -283,13 +283,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> 			}
> 		}
> #endif
> -		if (!mp_opt->echo) {
> +		if (!mp_opt->addr.echo) {
> 			mp_opt->ahmac = get_unaligned_be64(ptr);
> 			ptr += 8;
> 		}
> 		pr_debug("ADD_ADDR%s: id=%d, ahmac=%llu, echo=%d, port=%d",
> 			 (mp_opt->addr.family == AF_INET6) ? "6" : "",
> -			 mp_opt->addr.id, mp_opt->ahmac, mp_opt->echo, ntohs(mp_opt->addr.port));
> +			 mp_opt->addr.id, mp_opt->ahmac,
> +			 mp_opt->addr.echo, ntohs(mp_opt->addr.port));
> 		break;
>
> 	case MPTCPOPT_RM_ADDR:
> @@ -945,7 +946,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 	}
>
> 	if (((mp_opt->suboptions & OPTION_MPTCP_DSS) && mp_opt->use_ack) ||
> -	    ((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) && !mp_opt->echo)) {
> +	    ((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) && !mp_opt->addr.echo)) {
> 		/* subflows are fully established as soon as we get any
> 		 * additional ack, including ADD_ADDR.
> 		 */
> @@ -1076,7 +1077,7 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk,
> {
> 	u64 hmac = 0;
>
> -	if (mp_opt->echo)
> +	if (mp_opt->addr.echo)
> 		return true;
>
> 	hmac = add_addr_generate_hmac(msk->remote_key,
> @@ -1129,7 +1130,8 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>
> 		if ((mp_opt.suboptions & OPTION_MPTCP_ADD_ADDR) &&
> 		    add_addr_hmac_valid(msk, &mp_opt)) {
> -			if (!mp_opt.echo) {
> +			if (!mp_opt.addr.echo) {
> +				mp_opt.addr.echo = 1;
> 				mptcp_pm_add_addr_received(msk, &mp_opt.addr);
> 				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
> 			} else {
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 0eebfc9f39bc..4becac89a4a6 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -151,7 +151,6 @@ struct mptcp_options_received {
> 		mpc_map:1,
> 		reset_reason:4,
> 		reset_transient:1,
> -		echo:1,
> 		backup:1,
> 		deny_join_id0:1,
> 		__unused:2;
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next 1/4] mptcp: drop port parameter of mptcp_pm_add_addr_signal

[Matthieu Baerts]

Hi Geliang, Mat,

On 09/02/2022 02:20, Mat Martineau wrote:
> On Tue, 8 Feb 2022, Geliang Tang wrote:
> 
>> Drop the port parameter of mptcp_pm_add_addr_signal() and reflect it to
>> avoid passing too many parameters.
>>
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> 
> For this patch, looks like a helpful bit of cleanup. Thanks!
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the patch and the review.

I just applied this patch in our tree (not the rest of the series) with
Mat's RvB tag:

- 3afdb53fb2eb: mptcp: drop port parameter of mptcp_pm_add_addr_signal
- Results: e0f3a1a43be7..56333e0f13bc

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220209T113813
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

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