The user-space need to preperly account the data received/sent by
individual subflows. When additional subflows are created and/or
closed during the MPTCP socket lifetime, the information currently
exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
by the sequential position inside the info dumps, and that will change
with the above mentioned events.

To solve the above problem, this patch introduces a new subflow identifier
that is unique inside the given mptcp socket scope.

The initial subflow get the id 1 and the other subflows get incremental
values at join time.

The identifier is exposed to user-space overloading tcpi_fackets in
MPTCP_TCPINFO.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/uapi/linux/mptcp.h | 3 +++
 net/mptcp/protocol.c       | 3 +++
 net/mptcp/protocol.h       | 5 ++++-
 net/mptcp/sockopt.c        | 1 +
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 32af2d278cb4..49bfbf85cb80 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -249,4 +249,7 @@ struct mptcp_subflow_addrs {
 #define MPTCP_TCPINFO		2
 #define MPTCP_SUBFLOW_ADDRS	3
 
+/* MPTCP_TCPINFO overload obsoleted tcp fields with MPTCP-specific info */
+#define tcpi_subflow_id		tcpi_fackets
+
 #endif /* _UAPI_MPTCP_H */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2d331b2d62b7..7f5c89315aad 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -96,6 +96,7 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
 	list_add(&subflow->node, &msk->conn_list);
 	sock_hold(ssock->sk);
 	subflow->request_mptcp = 1;
+	subflow->subflow_id = msk->subflow_id++;
 
 	/* This is the first subflow, always with id 0 */
 	subflow->local_id_valid = 1;
@@ -838,6 +839,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
 	if (sk->sk_socket && !ssk->sk_socket)
 		mptcp_sock_graft(ssk, sk->sk_socket);
 
+	mptcp_subflow_ctx(ssk)->subflow_id = msk->subflow_id++;
 	mptcp_sockopt_sync_locked(msk, ssk);
 	return true;
 }
@@ -2726,6 +2728,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
+	msk->subflow_id = 1;
 
 	mptcp_pm_data_init(msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 2d7b2c80a164..babf1230c84d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -316,7 +316,8 @@ struct mptcp_sock {
 		u64	rtt_us; /* last maximum rtt of subflows */
 	} rcvq_space;
 
-	u32 setsockopt_seq;
+	u32		subflow_id;
+	u32		setsockopt_seq;
 	char		ca_name[TCP_CA_NAME_MAX];
 	struct mptcp_sock	*dl_next;
 };
@@ -497,6 +498,8 @@ struct mptcp_subflow_context {
 	u8	reset_reason:4;
 	u8	stale_count;
 
+	u32	subflow_id;
+
 	long	delegated_status;
 	unsigned long	fail_tout;
 
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index d4258869ac48..d2aa02e28917 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1032,6 +1032,7 @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval,
 			struct tcp_info info;
 
 			tcp_get_info(ssk, &info);
+			info.tcpi_subflow_id = subflow->subflow_id;
 
 			if (copy_to_user(infoptr, &info, sfd.size_user)) {
 				release_sock(sk);
-- 
2.40.0

Hi Paolo,

On 04/05/2023 18:39, Paolo Abeni wrote:
> The user-space need to preperly account the data received/sent by
> individual subflows. When additional subflows are created and/or
> closed during the MPTCP socket lifetime, the information currently
> exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
> by the sequential position inside the info dumps, and that will change
> with the above mentioned events.
> 
> To solve the above problem, this patch introduces a new subflow identifier
> that is unique inside the given mptcp socket scope.
> 
> The initial subflow get the id 1 and the other subflows get incremental
> values at join time.
> 
> The identifier is exposed to user-space overloading tcpi_fackets in
> MPTCP_TCPINFO.

This smart idea solves the issue with MPTCP_TCPINFO but not with
MPTCP_SUBFLOW_ADDRS. But I guess for the second one, we can easily add a
new field in "struct mptcp_subflow_addrs", right? If yes, we can do that
in a new dedicated commit, that's not blocking this series I think.

We can add:

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/388

(But not the "Closes" tag then)

> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/uapi/linux/mptcp.h | 3 +++
>  net/mptcp/protocol.c       | 3 +++
>  net/mptcp/protocol.h       | 5 ++++-
>  net/mptcp/sockopt.c        | 1 +
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 32af2d278cb4..49bfbf85cb80 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -249,4 +249,7 @@ struct mptcp_subflow_addrs {
>  #define MPTCP_TCPINFO		2
>  #define MPTCP_SUBFLOW_ADDRS	3
>  
> +/* MPTCP_TCPINFO overload obsoleted tcp fields with MPTCP-specific info */
> +#define tcpi_subflow_id		tcpi_fackets

Should we eventually add a comment next to the declaration of
"tcpi_fackets" in include/uapi/linux/tcp.h to say that this field is
being used by MPTCP_TCPINFO? Or this is not needed because this is field
is only overridden when using MPTCP_TCPINFO?

I guess it will never be removed anyway. But could it be overridden by
another field at some points later?

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

On Wed, 2023-05-17 at 16:25 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 04/05/2023 18:39, Paolo Abeni wrote:
> > The user-space need to preperly account the data received/sent by
> > individual subflows. When additional subflows are created and/or
> > closed during the MPTCP socket lifetime, the information currently
> > exposed via MPTCP_TCPINFO are not enough: subflows are identifed only
> > by the sequential position inside the info dumps, and that will change
> > with the above mentioned events.
> > 
> > To solve the above problem, this patch introduces a new subflow identifier
> > that is unique inside the given mptcp socket scope.
> > 
> > The initial subflow get the id 1 and the other subflows get incremental
> > values at join time.
> > 
> > The identifier is exposed to user-space overloading tcpi_fackets in
> > MPTCP_TCPINFO.
> 
> This smart idea solves the issue with MPTCP_TCPINFO but not with
> MPTCP_SUBFLOW_ADDRS. But I guess for the second one, we can easily add a
> new field in "struct mptcp_subflow_addrs", right? If yes, we can do that
> in a new dedicated commit, that's not blocking this series I think.
> 
> We can add:
> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/388

Right, the above is not completed yet.

Possibly the right solution would be implementing a new SOCKOPT dumping
all the info alltogether.

BTW I just noticed this patch has a bug: we need to explicitly set the
subflow_id for active MPJ subflow - that will not land into
__mptcp_finish_join as they are already present into the conn_list when
mptcp_finish_join() is invoked.

> 
> (But not the "Closes" tag then)
> 
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/uapi/linux/mptcp.h | 3 +++
> >  net/mptcp/protocol.c       | 3 +++
> >  net/mptcp/protocol.h       | 5 ++++-
> >  net/mptcp/sockopt.c        | 1 +
> >  4 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> > index 32af2d278cb4..49bfbf85cb80 100644
> > --- a/include/uapi/linux/mptcp.h
> > +++ b/include/uapi/linux/mptcp.h
> > @@ -249,4 +249,7 @@ struct mptcp_subflow_addrs {
> >  #define MPTCP_TCPINFO		2
> >  #define MPTCP_SUBFLOW_ADDRS	3
> >  
> > +/* MPTCP_TCPINFO overload obsoleted tcp fields with MPTCP-specific info */
> > +#define tcpi_subflow_id		tcpi_fackets
> 
> Should we eventually add a comment next to the declaration of
> "tcpi_fackets" in include/uapi/linux/tcp.h to say that this field is
> being used by MPTCP_TCPINFO? Or this is not needed because this is field
> is only overridden when using MPTCP_TCPINFO?
> 
> I guess it will never be removed anyway. But could it be overridden by
> another field at some points later?
> 
> Cheers,
> Matt