[MPTCP] Re: [MPTCP][PATCH mptcp-next 2/4] mptcp: add port number check for MP_JOIN

[MPTCP] Re: [MPTCP][PATCH mptcp-next 2/4] mptcp: add port number check for MP_JOIN

[Mat Martineau]

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

On Sun, 8 Nov 2020, Geliang Tang wrote:

> This patch added two new helpers, subflow_use_different_sport and
> subflow_use_different_dport, to check whether the subflow's source or
> destination port number is different from the msk's port number.
>
> When we received the MP_JOIN's SYN/SYNACK/ACK, we do these port number
> checks. If the subflow's source port number is different, we use another
> new helper mptcp_pm_sport_in_anno_list to check whether this port number
> is announced. If it isn't, we need to abort this connection.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> This patch should be inserted into the "ADD_ADDR: ports support" patchset,
> after the 8th patch, "mptcp: print out port and ahmac when receiving
> ADD_ADDR".
> ---
> net/mptcp/pm_netlink.c | 12 ++++++++++++
> net/mptcp/protocol.h   |  1 +
> net/mptcp/subflow.c    | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 47 insertions(+)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 069447424ddb..00996f410e1c 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -201,6 +201,18 @@ lookup_anno_list_by_saddr(struct mptcp_sock *msk,
> 	return NULL;
> }
>
> +bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk)
> +{
> +	struct mptcp_pm_add_entry *entry;
> +
> +	list_for_each_entry(entry, &msk->pm.anno_list, list) {
> +		if (entry->addr.port == inet_sk(sk)->inet_sport)
> +			return true;
> +	}

Should msk->pm.lock should be held here? It looks like the pm.lock is held 
in other places the anno_list is iterated over, for example when 
lookup_anno_list_by_saddr() is called.

Thanks,

Mat


> +
> +	return false;
> +}
> +
> static void mptcp_pm_add_timer(struct timer_list *timer)
> {
> 	struct mptcp_pm_add_entry *entry = from_timer(entry, timer, add_timer);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index b01c1fae9d8f..e575823c3077 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -516,6 +516,7 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
> void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk);
> void mptcp_pm_rm_addr_received(struct mptcp_sock *msk, u8 rm_id);
> void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
> +bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
> struct mptcp_pm_add_entry *
> mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> 		       struct mptcp_addr_info *addr);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1e9a72af67dc..32e4be3a7d11 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -112,6 +112,11 @@ static int __subflow_init_req(struct request_sock *req, const struct sock *sk_li
> 	return 0;
> }
>
> +static bool subflow_use_different_sport(struct mptcp_sock *msk, const struct sock *sk)
> +{
> +	return inet_sk(sk)->inet_sport != inet_sk((struct sock *)msk)->inet_sport;
> +}
> +
> static void subflow_init_req(struct request_sock *req,
> 			     const struct sock *sk_listener,
> 			     struct sk_buff *skb)
> @@ -182,6 +187,16 @@ static void subflow_init_req(struct request_sock *req,
>
> 		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
> 			 subflow_req->remote_nonce, subflow_req->msk);
> +
> +		if (subflow_use_different_sport(subflow_req->msk, sk_listener)) {
> +			pr_debug("syn inet_sport=%d %d",
> +				 ntohs(inet_sk(sk_listener)->inet_sport),
> +				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
> +			if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
> +				subflow_req->mp_join = 0;
> +				return;
> +			}
> +		}
> 	}
> }
>
> @@ -284,6 +299,11 @@ void mptcp_subflow_reset(struct sock *ssk)
> 		sock_hold(sk);
> }
>
> +static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct sock *sk)
> +{
> +	return inet_sk(sk)->inet_dport != inet_sk((struct sock *)msk)->inet_dport;
> +}
> +
> static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> @@ -349,6 +369,12 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>
> 		subflow->mp_join = 1;
> 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNACKRX);
> +
> +		if (subflow_use_different_dport(mptcp_sk(parent), sk)) {
> +			pr_debug("synack inet_dport=%d %d",
> +				 ntohs(inet_sk(sk)->inet_dport),
> +				 ntohs(inet_sk(parent)->inet_dport));
> +		}
> 	} else if (mptcp_check_fallback(sk)) {
> fallback:
> 		mptcp_rcv_space_init(mptcp_sk(parent), sk);
> @@ -611,6 +637,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>
> 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
> 			tcp_rsk(req)->drop_req = true;
> +
> +			if (subflow_use_different_sport(owner, sk)) {
> +				pr_debug("ack inet_sport=%d %d",
> +					 ntohs(inet_sk(sk)->inet_sport),
> +					 ntohs(inet_sk((struct sock *)owner)->inet_sport));
> +				if (!mptcp_pm_sport_in_anno_list(owner, sk))
> +					goto out;
> +			}
> 		}
> 	}
>
> -- 
> 2.26.2

--
Mat Martineau
Intel