From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1618172 for ; Wed, 18 Aug 2021 00:31:21 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10079"; a="301808892" X-IronPort-AV: E=Sophos;i="5.84,330,1620716400"; d="scan'208";a="301808892" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Aug 2021 17:31:20 -0700 X-IronPort-AV: E=Sophos;i="5.84,330,1620716400"; d="scan'208";a="520687735" Received: from sreddy3-mobl.amr.corp.intel.com ([10.251.13.114]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Aug 2021 17:31:20 -0700 Date: Tue, 17 Aug 2021 17:31:19 -0700 (PDT) From: Mat Martineau To: Florian Westphal cc: mptcp@lists.linux.dev Subject: Re: [PATCH mptcp-next v2 4/5] mptcp: add MPTCP_SUBFLOW_ADDRS getsockopt support In-Reply-To: <20210816162636.25611-5-fw@strlen.de> Message-ID: <98a76020-a5b2-63c8-b44d-a5131e84bc26@linux.intel.com> References: <20210816162636.25611-1-fw@strlen.de> <20210816162636.25611-5-fw@strlen.de> Precedence: bulk X-Mailing-List: mptcp@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII On Mon, 16 Aug 2021, Florian Westphal wrote: > This retrieves the address pairs of all subflows currently > active for a given mptcp connection. > > It re-uses the same meta-header as for MPTCP_TCPINFO. > > A new structure is provided to hold the subflow > address data: > > struct mptcp_subflow_addrs { > union { > sa_family_t sa_family; Looks like the update to __kernel_sa_family_t was in the code but not the comment. > struct sockaddr sa_local; > struct sockaddr_in sin_local; > struct sockaddr_in6 sin6_local; > struct sockaddr_storage ss_local; > }; > union { > struct sockaddr sa_remote; > struct sockaddr_in sin_remote; > struct sockaddr_in6 sin6_remote; > struct sockaddr_storage ss_remote; > }; > }; > > Usage of the new getsockopt is very similar to > MPTCP_TCPINFO one. > > Userspace allocates a > 'struct mptcp_subflow_data', followed by one or > more 'struct mptcp_subflow_addrs', then inits the > mptcp_subflow_data structure as follows: > > struct mptcp_subflow_addrs *sf_addr; > struct mptcp_subflow_data *addr; > socklen_t olen = sizeof(*addr) + (8 * sizeof(*sf_addr)); > > addr = malloc(olen); > addr->size_subflow_data = sizeof(*addr); > addr->num_subflows = 0; > addr->size_kernel = 0; > addr->size_user = sizeof(struct mptcp_subflow_addrs); > > sf_addr = (struct mptcp_subflow_addrs *)(addr + 1); > > and then retrieves the endpoint addresses via: > ret = getsockopt(fd, SOL_MPTCP, MPTCP_SUBFLOW_ADDRS, > addr, &olen); > > If the call succeeds, kernel will have added up to 8 > endpoint addresses after the 'mptcp_subflow_data' header. > > Userspace needs to re-check 'olen' value to detect how > many bytes have been filled in by the kernel. > > Userspace can check addr->num_subflows to discover when > there were more subflows that available data space. ^^^^ than? > > Signed-off-by: Florian Westphal > --- > include/uapi/linux/mptcp.h | 17 +++++++ > net/mptcp/sockopt.c | 91 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 108 insertions(+) > > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h > index 7c368fccd83e..c368c11a0ed9 100644 > --- a/include/uapi/linux/mptcp.h > +++ b/include/uapi/linux/mptcp.h > @@ -201,8 +201,25 @@ struct mptcp_subflow_data { > __u32 size_user; /* size of one element in data[] */ > } __attribute__((aligned(8))); > > +struct mptcp_subflow_addrs { > + union { > + __kernel_sa_family_t sa_family; > + struct sockaddr sa_local; > + struct sockaddr_in sin_local; > + struct sockaddr_in6 sin6_local; > + struct sockaddr_storage ss_local; > + }; > + union { > + struct sockaddr sa_remote; > + struct sockaddr_in sin_remote; > + struct sockaddr_in6 sin6_remote; > + struct sockaddr_storage ss_remote; > + }; > +}; > + > /* MPTCP socket options */ > #define MPTCP_INFO 1 > #define MPTCP_TCPINFO 2 > +#define MPTCP_SUBFLOW_ADDRS 3 > > #endif /* _UAPI_MPTCP_H */ > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index 77838933760c..51c488108b80 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -840,6 +840,95 @@ static int mptcp_getsockopt_tcpinfo(struct mptcp_sock *msk, char __user *optval, > return 0; > } > > +static void mptcp_get_sub_addrs(const struct sock *sk, struct mptcp_subflow_addrs *a) > +{ > + struct inet_sock *inet = inet_sk(sk); > + > + memset(a, 0, sizeof(*a)); > + > + if (sk->sk_family == AF_INET) { > + a->sin_local.sin_family = AF_INET; > + a->sin_local.sin_port = inet->inet_sport; > + a->sin_local.sin_addr.s_addr = inet->inet_rcv_saddr; > + > + if (!a->sin_local.sin_addr.s_addr) > + a->sin_local.sin_addr.s_addr = inet->inet_saddr; > + > + a->sin_remote.sin_family = AF_INET; > + a->sin_remote.sin_port = inet->inet_dport; > + a->sin_remote.sin_addr.s_addr = inet->inet_daddr; > +#if IS_ENABLED(CONFIG_IPV6) > + } else if (sk->sk_family == AF_INET6) { > + const struct ipv6_pinfo *np = inet6_sk(sk); > + > + a->sin6_local.sin6_family = AF_INET6; > + a->sin6_local.sin6_port = inet->inet_sport; > + > + if (ipv6_addr_any(&sk->sk_v6_rcv_saddr)) > + a->sin6_local.sin6_addr = np->saddr; > + else > + a->sin6_local.sin6_addr = sk->sk_v6_rcv_saddr; > + > + a->sin6_remote.sin6_family = AF_INET6; > + a->sin6_remote.sin6_port = inet->inet_dport; > + a->sin6_remote.sin6_addr = sk->sk_v6_daddr; > +#endif > + } > +} > + > +static int mptcp_getsockopt_subflow_addrs(struct mptcp_sock *msk, char __user *optval, > + int __user *optlen) > +{ > + struct mptcp_subflow_context *subflow; > + struct sock *sk = &msk->sk.icsk_inet.sk; Reverse-xmas... Other than these minor things, the kernel code looks good in patches 1-4. -Mat > + unsigned int sfcount = 0, copied = 0; > + struct mptcp_subflow_data sfd; > + char __user *addrptr; > + int len; > + > + len = mptcp_get_subflow_data(&sfd, optval, optlen); > + if (len < 0) > + return len; > + > + sfd.size_kernel = sizeof(struct mptcp_subflow_addrs); > + sfd.size_user = min_t(unsigned int, sfd.size_user, > + sizeof(struct mptcp_subflow_addrs)); > + > + addrptr = optval + sfd.size_subflow_data; > + > + lock_sock(sk); > + > + mptcp_for_each_subflow(msk, subflow) { > + struct sock *ssk = mptcp_subflow_tcp_sock(subflow); > + > + ++sfcount; > + > + if (len && len >= sfd.size_user) { > + struct mptcp_subflow_addrs a; > + > + mptcp_get_sub_addrs(ssk, &a); > + > + if (copy_to_user(addrptr, &a, sfd.size_user)) { > + release_sock(sk); > + return -EFAULT; > + } > + > + addrptr += sfd.size_user; > + copied += sfd.size_user; > + len -= sfd.size_user; > + } > + } > + > + release_sock(sk); > + > + sfd.num_subflows = sfcount; > + > + if (mptcp_put_subflow_data(&sfd, optval, copied, optlen)) > + return -EFAULT; > + > + return 0; > +} > + > static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname, > char __user *optval, int __user *optlen) > { > @@ -862,6 +951,8 @@ static int mptcp_getsockopt_sol_mptcp(struct mptcp_sock *msk, int optname, > return mptcp_getsockopt_info(msk, optval, optlen); > case MPTCP_TCPINFO: > return mptcp_getsockopt_tcpinfo(msk, optval, optlen); > + case MPTCP_SUBFLOW_ADDRS: > + return mptcp_getsockopt_subflow_addrs(msk, optval, optlen); > } > > return -EOPNOTSUPP; > -- > 2.31.1 > > > -- Mat Martineau Intel