[MPTCP] Re: [MPTCP][PATCH mptcp-next] mptcp: send ack for rm_addr

[MPTCP] Re: [MPTCP][PATCH mptcp-next] mptcp: send ack for rm_addr

[Mat Martineau]

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

On Mon, 15 Mar 2021, Geliang Tang wrote:

> This patch changes the sending ACK conditions for the ADD_ADDR, send an
> ACK packet for RM_ADDR too.
>
> Note:
> - Insert this commit between "mptcp: move to next addr when subflow
> creation fail" and "selftests: mptcp: signal addresses testcases".
> - Apply the patch 'Squash to "mptcp: remove id 0 address"' first.
> - tag: export/20210315T064655.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/173
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm.c         |  1 +
> net/mptcp/pm_netlink.c | 15 +++++++++------
> net/mptcp/protocol.h   |  1 +
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 966942d1013f..efa7deb96139 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -53,6 +53,7 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
> 	msk->pm.rm_list_tx = *rm_list;
> 	rm_addr |= BIT(MPTCP_RM_ADDR_SIGNAL);
> 	WRITE_ONCE(msk->pm.addr_signal, rm_addr);
> +	mptcp_pm_nl_add_addr_send_ack(msk);
> 	return 0;
> }
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 6bb6117bf2f7..71ba203bfc5f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -56,8 +56,6 @@ struct pm_nl_pernet {
> #define MPTCP_PM_ADDR_MAX	8
> #define ADD_ADDR_RETRANS_MAX	3
>
> -static void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk);
> -
> static bool addresses_equal(const struct mptcp_addr_info *a,
> 			    struct mptcp_addr_info *b, bool use_port)
> {
> @@ -524,14 +522,15 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> 	mptcp_pm_nl_add_addr_send_ack(msk);
> }
>
> -static void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk)
> +void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk)

Like commit 13ad9f01a29e3f458fb3b319fb53323b2b0d1e68 I think it makes 
sense to remove the "add" from this function name, since this is now used 
for both add and rm cases. Ok to make that a separate commit.

> {
> 	struct mptcp_subflow_context *subflow;
>
> 	msk_owned_by_me(msk);
> 	lockdep_assert_held(&msk->pm.lock);
>
> -	if (!mptcp_pm_should_add_signal(msk))
> +	if (!mptcp_pm_should_add_signal(msk) &&
> +	    !mptcp_pm_should_rm_signal(msk))
> 		return;
>
> 	__mptcp_flush_join_list(msk);
> @@ -541,9 +540,11 @@ static void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk)
> 		u8 add_addr;
>
> 		spin_unlock_bh(&msk->pm.lock);
> -		pr_debug("send ack for add_addr%s%s",
> +		pr_debug("send ack for %s%s%s%s",
> +			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "",
> 			 mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
> -			 mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
> +			 mptcp_pm_should_add_signal_port(msk) ? " [port]" : "",
> +			 mptcp_pm_should_rm_signal(msk) ? "rm_addr" : "");

It would be better to add this before the ipv6 line above, so "add_addr" 
and "rm_addr" would appear in the same place in the debug output.

>
> 		lock_sock(ssk);
> 		tcp_send_ack(ssk);
> @@ -555,6 +556,8 @@ static void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk)
> 			add_addr &= ~BIT(MPTCP_ADD_ADDR_IPV6);
> 		if (mptcp_pm_should_add_signal_port(msk))
> 			add_addr &= ~BIT(MPTCP_ADD_ADDR_PORT);
> +		if (mptcp_pm_should_rm_signal(msk))
> +			add_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);

It looks like msk->pm.addr_signal is also cleared in 
mptcp_pm_rm_addr_signal() - is it necessary to clear it here? It seems 
like adding these lines could clear the bit even when the RM_ADDR hasn't 
been sent due to lack of space.

(Similarly, mptcp_pm_add_addr_signal() clears the pm.addr_signal bits, so 
I'm not sure why they are also cleared in this function. I'm either 
misunderstanding the use of the bits or something is buggy)

> 		WRITE_ONCE(msk->pm.addr_signal, add_addr);
> 	}
> }
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0e301ae56b0..34fddb1fcd1e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -667,6 +667,7 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
> void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
> 			      struct mptcp_addr_info *addr);
> void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk);
> +void mptcp_pm_nl_add_addr_send_ack(struct mptcp_sock *msk);
> void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
> 			       const struct mptcp_rm_list *rm_list);
> void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup);
>

--
Mat Martineau
Intel