[MPTCP] Re: [PATCH] mptcp: mptcp reset option

[MPTCP] Re: [PATCH] mptcp: mptcp reset option

[Mat Martineau]

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

On Mon, 22 Feb 2021, Florian Westphal wrote:

> The MPTCP reset option allows to carry a mptcp-specific error code that
> provides more information on the nature of a connection reset.
>
> Reset option data received gets stored in the subflow context so it can
> be sent to userspace via the 'subflow closed' netlink event.
>
> When a subflow is closed, the desired error code that should be sent to
> the peer is also placed in the subflow context structure.
>
> If a reset is sent before subflow establishment could complete, e.g. on
> HMAC failure during an MP_JOIN operation, the mptcp skb extension is
> used to store the reset information.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> include/net/mptcp.h        | 18 +++++++++--
> include/uapi/linux/mptcp.h |  2 ++
> net/ipv4/tcp_ipv4.c        | 21 ++++++++++--
> net/ipv6/tcp_ipv6.c        | 14 +++++++-
> net/mptcp/options.c        | 66 +++++++++++++++++++++++++++++++++++---
> net/mptcp/pm_netlink.c     | 12 +++++++
> net/mptcp/protocol.c       | 12 +++++--
> net/mptcp/protocol.h       | 22 ++++++++++++-
> net/mptcp/subflow.c        | 37 ++++++++++++++++++---
> 9 files changed, 185 insertions(+), 19 deletions(-)
>

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 59ea64e5e914..e95bd8fa5e62 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1648,9 +1648,21 @@ static int mptcp_event_sub_closed(struct sk_buff *skb,
> 				  const struct mptcp_sock *msk,
> 				  const struct sock *ssk)
> {
> +	const struct mptcp_subflow_context *sf;
> +
> 	if (mptcp_event_put_token_and_ssk(skb, msk, ssk))
> 		return -EMSGSIZE;
>
> +	sf = mptcp_subflow_ctx(ssk);
> +	if (sf->reset_reason == 0)
> +		return 0;

From what I see in the RFC, the T flag can be set even if the reason code 
is 'unspecified', so I don't think MPTCP_ATTR_RESET_FLAGS should be 
skipped if reset_reason == 0.

I would suggest removing the above two lines and always including the 
reason & flags, but if it's worthwhile to optimize out either attribute 
then please explain.

> +
> +	if (nla_put_u32(skb, MPTCP_ATTR_RESET_REASON, sf->reset_reason))
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u32(skb, MPTCP_ATTR_RESET_FLAGS, sf->reset_transient))
> +		return -EMSGSIZE;
> +
> 	return 0;
> }
>

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 593085610971..64ac8db5b673 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -26,6 +26,7 @@
> #define OPTION_MPTCP_RM_ADDR	BIT(8)
> #define OPTION_MPTCP_FASTCLOSE	BIT(9)
> #define OPTION_MPTCP_PRIO	BIT(10)
> +#define OPTION_MPTCP_RST	BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE	0
> @@ -36,6 +37,7 @@
> #define MPTCPOPT_MP_PRIO	5
> #define MPTCPOPT_MP_FAIL	6
> #define MPTCPOPT_MP_FASTCLOSE	7
> +#define MPTCPOPT_RST		8
>
> /* MPTCP suboption lengths */
> #define TCPOLEN_MPTCP_MPC_SYN		4
> @@ -64,6 +66,7 @@
> #define TCPOLEN_MPTCP_PRIO		3
> #define TCPOLEN_MPTCP_PRIO_ALIGN	4
> #define TCPOLEN_MPTCP_FASTCLOSE		12
> +#define TCPOLEN_MPTCP_RST		4
>
> /* MPTCP MP_JOIN flags */
> #define MPTCPOPT_BACKUP		BIT(0)
> @@ -93,6 +96,18 @@
> /* MPTCP MP_PRIO flags */
> #define MPTCP_PRIO_BKUP		BIT(0)
>
> +/* MPTCP TCPRST flags */
> +#define MPTCP_RST_TRANSIENT	BIT(0)
> +
> +/* MPTCP Reset reason codes, rfc8684 */
> +#define MPTCP_RST_EUNSPEC	0
> +#define MPTCP_RST_EMPTCP	1
> +#define MPTCP_RST_ERESOURCE	2
> +#define MPTCP_RST_EPROHIBIT	3
> +#define MPTCP_RST_EWQ2BIG	4
> +#define MPTCP_RST_EBADPERF	5
> +#define MPTCP_RST_EMIDDLEBOX	6
> +

The MSP_RST_* defines could be useful in include/uapi/linux/mptcp.h


> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index e411be079c44..a59dd71deb3e 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -187,8 +187,16 @@ static int subflow_check_req(struct request_sock *req,
> 		subflow_req->msk = subflow_token_join_request(req);
>
> 		/* Can't fall back to TCP in this case. */
> -		if (!subflow_req->msk)
> +		if (!subflow_req->msk) {


> +			struct mptcp_ext *mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
> +
> +			if (mpext) {
> +				memset(mpext, 0, sizeof(*mpext));
> +				mpext->reset_reason = MPTCP_RST_EMPTCP;
> +			}

This chunk of code (with slight variation in the first line) is used in 3 
places, worth adding a helper function in subflow.c?


Thanks,

Mat


> +
> 			return -EPERM;
> +		}
>
> 		if (subflow_use_different_sport(subflow_req->msk, sk_listener)) {
> 			pr_debug("syn inet_sport=%d %d",
> @@ -396,8 +404,10 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> 	} else if (subflow->request_join) {
> 		u8 hmac[SHA256_DIGEST_SIZE];
>
> -		if (!mp_opt.mp_join)
> +		if (!mp_opt.mp_join) {
> +			subflow->reset_reason = MPTCP_RST_EMPTCP;
> 			goto do_reset;
> +		}
>
> 		subflow->thmac = mp_opt.thmac;
> 		subflow->remote_nonce = mp_opt.nonce;
> @@ -406,6 +416,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>
> 		if (!subflow_thmac_valid(subflow)) {
> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINACKMAC);
> +			subflow->reset_reason = MPTCP_RST_EMPTCP;
> 			goto do_reset;
> 		}
>
> @@ -434,6 +445,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> 	return;
>
> do_reset:
> +	subflow->reset_transient = 0;
> 	mptcp_subflow_reset(sk);
> }
>
> @@ -586,6 +598,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 	struct mptcp_options_received mp_opt;
> 	bool fallback, fallback_is_fatal;
> 	struct sock *new_msk = NULL;
> +	struct mptcp_ext *mpext;
> 	struct sock *child;
>
> 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> @@ -645,8 +658,15 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 		 * to reset the context to non MPTCP status.
> 		 */
> 		if (!ctx || fallback) {
> -			if (fallback_is_fatal)
> +			if (fallback_is_fatal) {
> +				mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
> +				if (mpext) {
> +					memset(mpext, 0, sizeof(*mpext));
> +					mpext->reset_reason = MPTCP_RST_EMPTCP;
> +				}
> +
> 				goto dispose_child;
> +			}
>
> 			subflow_drop_ctx(child);
> 			goto out;
> @@ -681,8 +701,15 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 			struct mptcp_sock *owner;
>
> 			owner = subflow_req->msk;
> -			if (!owner)
> +			if (!owner) {
> +				mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
> +				if (mpext) {
> +					memset(mpext, 0, sizeof(*mpext));
> +					mpext->reset_reason = MPTCP_RST_EPROHIBIT;
> +				}
> +
> 				goto dispose_child;
> +			}
>
> 			/* move the msk reference ownership to the subflow */
> 			subflow_req->msk = NULL;
> @@ -1046,6 +1073,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 	smp_wmb();
> 	ssk->sk_error_report(ssk);
> 	tcp_set_state(ssk, TCP_CLOSE);
> +	subflow->reset_transient = 0;
> +	subflow->reset_reason = MPTCP_RST_EMPTCP;
> 	tcp_send_active_reset(ssk, GFP_ATOMIC);
> 	subflow->data_avail = 0;
> 	return false;
> -- 
> 2.26.2

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] mptcp: mptcp reset option

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> On Mon, 22 Feb 2021, Florian Westphal wrote:
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 59ea64e5e914..e95bd8fa5e62 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1648,9 +1648,21 @@ static int mptcp_event_sub_closed(struct sk_buff *skb,
> > 				  const struct mptcp_sock *msk,
> > 				  const struct sock *ssk)
> > {
> > +	const struct mptcp_subflow_context *sf;
> > +
> > 	if (mptcp_event_put_token_and_ssk(skb, msk, ssk))
> > 		return -EMSGSIZE;
> > 
> > +	sf = mptcp_subflow_ctx(ssk);
> > +	if (sf->reset_reason == 0)
> > +		return 0;
> 
> From what I see in the RFC, the T flag can be set even if the reason code is
> 'unspecified', so I don't think MPTCP_ATTR_RESET_FLAGS should be skipped if
> reset_reason == 0.

OK.

> I would suggest removing the above two lines and always including the reason
> & flags, but if it's worthwhile to optimize out either attribute then please
> explain.

Hmpf, yes, this isn't sufficient to do what I wanted.
The intention was to ONLY set any of these attributes in the nlmsg if
the subflow had been zapped via tcprst option.

as reset_reason can be 0 in the wire format (and hacks like 'lets store
v+1' are error prone), i will add another bit to signal the field is
valid instead.

> > +/* MPTCP Reset reason codes, rfc8684 */
> > +#define MPTCP_RST_EUNSPEC	0
> > +#define MPTCP_RST_EMPTCP	1
> > +#define MPTCP_RST_ERESOURCE	2
> > +#define MPTCP_RST_EPROHIBIT	3
> > +#define MPTCP_RST_EWQ2BIG	4
> > +#define MPTCP_RST_EBADPERF	5
> > +#define MPTCP_RST_EMIDDLEBOX	6
> > +
> 
> The MSP_RST_* defines could be useful in include/uapi/linux/mptcp.h

Okay, no problem. I was reluctant to place them there but
given those values are from the rfc i think it makes sense to have this
in a uapi header.

> > +			struct mptcp_ext *mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
> > +
> > +			if (mpext) {
> > +				memset(mpext, 0, sizeof(*mpext));
> > +				mpext->reset_reason = MPTCP_RST_EMPTCP;
> > +			}
> 
> This chunk of code (with slight variation in the first line) is used in 3
> places, worth adding a helper function in subflow.c?

Sure, will add one.