[MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Davide Caratti]

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

add ulp-specific diagnostic functions, so that subflow information can be
dumped to userspace programs like 'ss'.

Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
---

Notes:
    this patch applies on top of "net/mptcp: rcu-ify the subflow context"

 include/uapi/linux/inet_diag.h |  1 +
 include/uapi/linux/mptcp.h     | 23 ++++++++++
 net/mptcp/Makefile             |  2 +-
 net/mptcp/diag.c               | 78 ++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.h           |  3 ++
 net/mptcp/subflow.c            |  2 +
 6 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/mptcp.h
 create mode 100644 net/mptcp/diag.c

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index a1ff345b3f33..13d16b887512 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -163,6 +163,7 @@ enum {
 	INET_ULP_INFO_UNSPEC,
 	INET_ULP_INFO_NAME,
 	INET_ULP_INFO_TLS,
+	INET_ULP_INFO_MPTCP,
 	__INET_ULP_INFO_MAX,
 };
 #define INET_ULP_INFO_MAX (__INET_ULP_INFO_MAX - 1)
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
new file mode 100644
index 000000000000..2e0cdc395dc3
--- /dev/null
+++ b/include/uapi/linux/mptcp.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef _UAPI_MPTCP_H
+#define _UAPI_MPTCP_H
+
+#include <linux/types.h>
+
+#define SUBFLOW_FLAGS_MP_REMOTE		BIT(0)
+#define SUBFLOW_FLAGS_MP_LOCAL		BIT(1)
+#define SUBFLOW_FLAGS_MP_JOIN		BIT(2)
+#define SUBFLOW_FLAGS_DSS_CSUM		BIT(3)
+#define SUBFLOW_FLAGS_BKUP_LOCAL	BIT(4)
+
+enum {
+	MPTCP_SUBFLOW_UNSPEC,
+	MPTCP_SUBFLOW_TOKEN,
+	MPTCP_SUBFLOW_LOCAL_KEY,
+	MPTCP_SUBFLOW_REMOTE_KEY,
+	MPTCP_SUBFLOW_FLAGS,
+	MPTCP_SUBFLOW_PAD,
+	__MPTCP_SUBFLOW_MAX
+};
+#define MPTCP_SUBFLOW_MAX (__MPTCP_SUBFLOW_MAX - 1)
+#endif /* _UAPI_MPTCP_H */
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index 289fdf4339c1..6b556e2995ec 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_MPTCP) += mptcp.o
 
-mptcp-y := protocol.o subflow.o options.o token.o crypto.o pm.o ctrl.o
+mptcp-y := protocol.o subflow.o options.o token.o crypto.o pm.o ctrl.o diag.o
diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
new file mode 100644
index 000000000000..789e9684d29a
--- /dev/null
+++ b/net/mptcp/diag.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/* MPTCP socket monitoring support
+ *
+ * Copyright (c) 2019 Red Hat
+ *
+ * Author: Davide Caratti <dcaratti(a)redhat.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/net.h>
+#include <linux/inet_diag.h>
+#include <net/netlink.h>
+#include <uapi/linux/mptcp.h>
+#include "protocol.h"
+
+int subflow_get_info(const struct sock *sk, struct sk_buff *skb)
+{
+	struct subflow_context *subflow;
+	struct nlattr *start;
+	u32 flags = 0;
+	int err;
+
+	start = nla_nest_start_noflag(skb, INET_ULP_INFO_MPTCP);
+	if (!start)
+		return -EMSGSIZE;
+
+	rcu_read_lock();
+	subflow = rcu_dereference(inet_csk(sk)->icsk_ulp_data);
+	if (!subflow) {
+		err = 0;
+		goto nla_failure;
+	}
+	if (nla_put_u32(skb, MPTCP_SUBFLOW_TOKEN, subflow->token)) {
+		err = -EMSGSIZE;
+		goto nla_failure;
+	}
+	if (subflow->mp_capable)
+		flags |= SUBFLOW_FLAGS_MP_REMOTE;
+	if (subflow->request_mptcp)
+		flags |= SUBFLOW_FLAGS_MP_LOCAL;
+	if (subflow->mp_join)
+		flags |= SUBFLOW_FLAGS_MP_JOIN;
+	if (subflow->request_cksum)
+		flags |= SUBFLOW_FLAGS_DSS_CSUM;
+	if (subflow->request_bkup)
+		flags |= SUBFLOW_FLAGS_BKUP_LOCAL;
+	if (nla_put_be64(skb, MPTCP_SUBFLOW_LOCAL_KEY,
+			 cpu_to_be64(subflow->local_key),
+			 MPTCP_SUBFLOW_PAD) ||
+	    nla_put_be64(skb, MPTCP_SUBFLOW_REMOTE_KEY,
+			 cpu_to_be64(subflow->remote_key),
+			 MPTCP_SUBFLOW_PAD) ||
+	    nla_put_u32(skb, MPTCP_SUBFLOW_FLAGS, flags)) {
+		err = -EMSGSIZE;
+		goto nla_failure;
+	}
+	rcu_read_unlock();
+	nla_nest_end(skb, start);
+	return 0;
+
+nla_failure:
+	rcu_read_unlock();
+	nla_nest_cancel(skb, start);
+	return err;
+}
+
+size_t subflow_get_info_size(const struct sock *sk)
+{
+	size_t size = 0;
+
+	size += nla_total_size(0) +		/* INET_ULP_INFO_MPTCP */
+		nla_total_size(4) +		/* MPTCP_SUBFLOW_TOKEN */
+		nla_total_size_64bit(8) +	/* MPTCP_SUBFLOW_LOCAL_KEY */
+		nla_total_size_64bit(8) +	/* MPTCP_SUBFLOW_REMOTE_KEY */
+		nla_total_size(4) +		/* MPTCP_SUBFLOW_FLAGS */
+		0;
+	return size;
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7a210593ca60..53c63e32431c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -273,4 +273,7 @@ static inline struct mptcp_ext *mptcp_get_ext(struct sk_buff *skb)
 	return (struct mptcp_ext *)skb_ext_find(skb, SKB_EXT_MPTCP);
 }
 
+size_t subflow_get_info_size(const struct sock *sk);
+int subflow_get_info(const struct sock *sk, struct sk_buff *skb);
+
 #endif /* __MPTCP_PROTOCOL_H */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3d4a388147bc..7f9fec9e1514 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -498,6 +498,8 @@ static struct tcp_ulp_ops subflow_ulp_ops __read_mostly = {
 	.init		= subflow_ulp_init,
 	.release	= subflow_ulp_release,
 	.clone		= subflow_ulp_clone,
+	.get_info	= subflow_get_info,
+	.get_info_size	= subflow_get_info_size,
 };
 
 static int subflow_ops_init(struct request_sock_ops *subflow_ops)
-- 
2.20.1

Re: [MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Davide Caratti]

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

On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
> add ulp-specific diagnostic functions, so that subflow information can be
> dumped to userspace programs like 'ss'.

for the record: iproute2 support is also on the way, but also in this case
the first step will be in ktls. This patch exposes token, local/remote
keys, and some of the bitfield members. What do you think it's reasonable
exposing to users of 'ss -i' ?

any feedback welcome.
thanks,
-- 
davide

Re: [MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Matthieu Baerts]

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

Hi Davide,

Thank you for sharing this patch and thank you for looking at that!

On 12/09/2019 18:57, Davide Caratti wrote:
> On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
>> add ulp-specific diagnostic functions, so that subflow information can be
>> dumped to userspace programs like 'ss'.
> 
> for the record: iproute2 support is also on the way, but also in this case
> the first step will be in ktls. This patch exposes token, local/remote
> keys, and some of the bitfield members. What do you think it's reasonable
> exposing to users of 'ss -i' ?

That looks good to start! I guess later we can expose more info like the
number of subflows, if there was a fallback to TCP (or maybe non
relevant?), the version of MPTCP and other info we store in the MPTCP
socket structure, etc.

Also, do you think it is possible to restrict the informations that are
shared depending on the capabilities of the requester?

A bit like what happened on the out-of-tree implementation with this
famous (for us) commit:

	https://github.com/multipath-tcp/mptcp/commit/e993b775d8abda23

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] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Florian Westphal]

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

Davide Caratti <dcaratti(a)redhat.com> wrote:
> On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
> > add ulp-specific diagnostic functions, so that subflow information can be
> > dumped to userspace programs like 'ss'.
> 
> for the record: iproute2 support is also on the way, but also in this case
> the first step will be in ktls. This patch exposes token, local/remote
> keys, and some of the bitfield members. What do you think it's reasonable
> exposing to users of 'ss -i' ?

The MPTCP (parent/meta) socket data, e.g. the DSS would be good.

Once Paolo retrans is in, we could also extend that to expose
information about MPTCP-Level events (rather than subflow stats).

This is a great start though.  I guess it might be good to expose
how much more data the current mapping expects.

Wrt. local/remote keys, what is the use case?

I'm asking because some people get nervous when you add an api to
expose 'secret keys'.

If there is no clear use-case i'd rather not expose them for now,
it can be added later if needed.

Re: [MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Davide Caratti]

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

hi Florian,

this is the kind of feedback I needed, thanks a lot for looking at this.

On Sat, 2019-09-14 at 12:09 +0200, Florian Westphal wrote:
> Davide Caratti <dcaratti(a)redhat.com> wrote:
> > On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
[...]
> > This patch exposes token, local/remote
> > keys, and some of the bitfield members. What do you think it's reasonable
> > exposing to users of 'ss -i' ?
> 
> The MPTCP (parent/meta) socket data, e.g. the DSS would be good.
> Once Paolo retrans is in, we could also extend that to expose
> information about MPTCP-Level events (rather than subflow stats).
> This is a great start though.  I guess it might be good to expose
> how much more data the current mapping expects.

I'm a bit reluctant on using ulp_diags to dump data that are specific to
the "master socket": when the number of subflows becomes high, this
results in unneeded copies of the same information that are passed from
kernel to userspace.

It would be much better if msk-related information are obtained by another
piece of code (that does not exist yet) that walks the token tree,
extracts msk-related info and dumps it to userspace, e.g. when 'ss' is
called with a new command line argument (something like 'ss -M').

For subflows, the only msk-specific information that's worth being part of
dump is the value of 'token', so we can use it on a live system to know
which MPTCP connection is using a given TCP socket, or if two (or more)
sockets belong to the same MPTCP connection.

> Wrt. local/remote keys, what is the use case?

my use case was assessing the correct exchange for the first 3WHS. But,
good point, also local/remote key are specific to the MSK: we don't need
them in the subflow dump.

> I'm asking because some people get nervous when you add an api to
> expose 'secret keys'.
> 
> If there is no clear use-case i'd rather not expose them for now,
> it can be added later if needed.

since local/rempte keys travelled in cleartext over the internet during
the first 3WHS, I wouldn't say they are a real secret. And then someone of
those "nervous" guys would argue anyway why we are storing them at memory
locations that have a constant offset from the subflow (and master
socket), and why we don't explicitly clear them when a subflow ends(*). 
But, I agree with you we don't need to dump these two numbers here, they
are more part of the "MPTCP control block" (**).

back to the topic, please find below a proposal of subflow-related infos:

- token
- rel_write_seq
- idsn
- map_seq
- map_subflow_seq (*)
- ssn_offset
- map_data_len (*)
- flags (all fields except 'request_version')
- local_id
- remote_id

WDYT?

-- 
davide


(*) e.g. what was done in kTLS with commits c844eb46b7d ("tls: clear key
material from kernel memory when do_tls_setsockopt_conf fails") and
86029d10af18 ("tls: zero the crypto information from tls_context before
freeing"). 
(**) like defined in https://tools.ietf.org/html/rfc6824#appendix-B

Re: [MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Florian Westphal]

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

Davide Caratti <dcaratti(a)redhat.com> wrote:
> On Sat, 2019-09-14 at 12:09 +0200, Florian Westphal wrote:
> > Davide Caratti <dcaratti(a)redhat.com> wrote:
> > > On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
> [...]
> > > This patch exposes token, local/remote
> > > keys, and some of the bitfield members. What do you think it's reasonable
> > > exposing to users of 'ss -i' ?
> > 
> > The MPTCP (parent/meta) socket data, e.g. the DSS would be good.
> > Once Paolo retrans is in, we could also extend that to expose
> > information about MPTCP-Level events (rather than subflow stats).
> > This is a great start though.  I guess it might be good to expose
> > how much more data the current mapping expects.
> 
> I'm a bit reluctant on using ulp_diags to dump data that are specific to
> the "master socket": when the number of subflows becomes high, this
> results in unneeded copies of the same information that are passed from
> kernel to userspace.

I agree that MPTCP / parent socket data should not be exposed via ulp diag.

> It would be much better if msk-related information are obtained by another
> piece of code (that does not exist yet) that walks the token tree,

Right.  If that is too problematic due to parallel updates we could
consider adding them to a different data structure that is only used for
dumping as well.

If token tree walk works out, all the better.

> extracts msk-related info and dumps it to userspace, e.g. when 'ss' is
> called with a new command line argument (something like 'ss -M').

Yes, something like that.

> For subflows, the only msk-specific information that's worth being part of
> dump is the value of 'token', so we can use it on a live system to know
> which MPTCP connection is using a given TCP socket, or if two (or more)
> sockets belong to the same MPTCP connection.

Right.

> > Wrt. local/remote keys, what is the use case?
> 
> my use case was assessing the correct exchange for the first 3WHS. But,
> good point, also local/remote key are specific to the MSK: we don't need
> them in the subflow dump.

Ok.
> > I'm asking because some people get nervous when you add an api to
> > expose 'secret keys'.
> > 
> > If there is no clear use-case i'd rather not expose them for now,
> > it can be added later if needed.
> 
> since local/rempte keys travelled in cleartext over the internet during
> the first 3WHS, I wouldn't say they are a real secret. And then someone of
> those "nervous" guys would argue anyway why we are storing them at memory
> locations that have a constant offset from the subflow (and master
> socket), and why we don't explicitly clear them when a subflow ends(*).

[..]

Alright, fair enough.

> back to the topic, please find below a proposal of subflow-related infos:
> 
> - token
> - rel_write_seq
> - idsn
> - map_seq
> - map_subflow_seq (*)
> - ssn_offset
> - map_data_len (*)
> - flags (all fields except 'request_version')
> - local_id
> - remote_id
> 
> WDYT?

Looks good to me.

Re: [MPTCP] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Matthieu Baerts]

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

Hi Davide, Florian,

On 16/09/2019 15:00, Davide Caratti wrote:
> On Sat, 2019-09-14 at 12:09 +0200, Florian Westphal wrote:
>> Davide Caratti <dcaratti(a)redhat.com> wrote:
>>> On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:

[...]

>> I'm asking because some people get nervous when you add an api to
>> expose 'secret keys'.
>>
>> If there is no clear use-case i'd rather not expose them for now,
>> it can be added later if needed.
> 
> since local/rempte keys travelled in cleartext over the internet during
> the first 3WHS, I wouldn't say they are a real secret. And then someone of
> those "nervous" guys would argue anyway why we are storing them at memory
> locations that have a constant offset from the subflow (and master
> socket), and why we don't explicitly clear them when a subflow ends(*). 
> But, I agree with you we don't need to dump these two numbers here, they
> are more part of the "MPTCP control block" (**).
For me, the token is not a password. As you said, it is visible on the wire.

But what we maybe don't want is to allow the user/app X, without any
privilege, to see the token of the connections generated by the user/app
Y  on the same machine. Because the user/app X could report this
information to another peer to help tracing someone or help to establish
a new subflow if the keys also have leaked.

I agree that it does not seem to be a big issue to expose this token but
maybe safer to only expose it to "privileged" users/apps, no?

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] [PATCH RFC] mptcp: allow dumping subflow context to userspace

[Davide Caratti]

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

On Thu, 2019-09-12 at 19:34 +0200, Matthieu Baerts wrote:
> Hi Davide,
> 
> Thank you for sharing this patch and thank you for looking at that!
> 
> On 12/09/2019 18:57, Davide Caratti wrote:
> > On Thu, 2019-09-12 at 18:50 +0200, Davide Caratti wrote:
> > > add ulp-specific diagnostic functions, so that subflow information can be
> > > dumped to userspace programs like 'ss'.
> > 
> > for the record: iproute2 support is also on the way, but also in this case
> > the first step will be in ktls. This patch exposes token, local/remote
> > keys, and some of the bitfield members. What do you think it's reasonable
> > exposing to users of 'ss -i' ?
> 
> That looks good to start! I guess later we can expose more info like the
> number of subflows, 

probably the information on 'token' should be sufficient, even though it
needs to query the kernel for TCP_DIAG_INFO on all TCP sockets matching a
particular filter (e.g, given a destination address + port, the number of
subflows is the sum of items that have equal value of 'token').

Another possibility, not necessarily alternative, is to enumerate master
sockets instead of subflows (this needs to walk the radix tree instead of
TCP hashes).

> if there was a fallback to TCP (or maybe non
> relevant?), 

I think this is relevant, and it should also be feasible because the
ulp_ops  shouldstay attached to the subflow socket also after the
fallback.

I will try to include this in the next patch, thanks Matthieu.

> the version of MPTCP and other info we store in the MPTCP
> socket structure, etc.

this is more relevant to the msk IMO.

> Also, do you think it is possible to restrict the informations that are
> shared depending on the capabilities of the requester?
> 
> A bit like what happened on the out-of-tree implementation with this
> famous (for us) commit:
> 
> 	https://github.com/multipath-tcp/mptcp/commit/e993b775d8abda23

currently these sock_diags are available only to users having
CAP_NET_ADMIN, that can obtain the same infos attaching a packet socket on
the host. Do you think we should restrict more than this?

-- 
davide