[PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[Matthieu Baerts]

Here is the implementation of an idea I suggested a few months ago: it
is a factoring of the sockopt code trying to uniform it, mainly to rely
on {ip,ipv6,tcp}_[gs]etsockopt() instead of copying what is done there
to get and set values.

That's a first step. More might come if we are OK with the approach,
e.g.

- For SOL_TCP socket options we cannot store info in the MPTCP socket so
  we still need to "manually" store it somewhere in the msk. Maybe we
  could store info in the different subflows (and create a first one if
  there is no subflow) and retrieve info from there?

- For SOL_SOCKET socket options, it looks like it should be feasible to
  do something similar to what I did here but I'm probably missing
  something. This could clean quite a lot of code, e.g. the last commit
  from:

    https://github.com/matttbe/mptcp_net-next/commits/b4/mptcp-unify-sockopt-issue-353

- To sync the socket options when a new subflow has been created, for
  the moment, we copy info from the msk to the subflow but maybe it
   would be fine to call some getsockopt() / setsockopt() to do the
  sync? If we take this approach, we might want to save a list of
  setsockopt() that have been done in the past on this socket and only
  re-do these ones? That should help with the maintenance of these
  socket options and ease the support of new ones.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
Matthieu Baerts (6):
      mptcp: sockopt: move tcp_inq code to a dedicated function
      mptcp: sockopt: update supported list
      mptcp: sockopt: get val in a generic way
      mptcp: sockopt: add missing getsockopt() options
      mptcp: sockopt: set val in a generic way
      mptcp: sockopt: support IP_TTL & IPV6_UNICAST_HOPS

 net/mptcp/sockopt.c | 241 ++++++++++++++++++++++++++--------------------------
 1 file changed, 121 insertions(+), 120 deletions(-)
---
base-commit: cde4fa6a0a5ed3effb75008ce99eb842bc448e5d
change-id: 20230221-mptcp-unify-sockopt-issue-353-1ad56ba6b04e

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

[PATCH mptcp-next 1/6] mptcp: sockopt: move tcp_inq code to a dedicated function

[Matthieu Baerts]

Similar to what we have with the other TCP socket options where each of
them are in a dedicated function.

It is clearer like that.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 63f7a09335c5..a46a00bf0f08 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -577,6 +577,25 @@ static bool mptcp_supported_sockopt(int level, int optname)
 	return false;
 }
 
+static int mptcp_setsockopt_sol_tcp_inq(struct mptcp_sock *msk, sockptr_t optval,
+					unsigned int optlen)
+{
+	struct sock *sk = (struct sock *)msk;
+	int ret, val;
+
+	ret = mptcp_get_int_option(msk, optval, optlen, &val);
+	if (ret)
+		return ret;
+	if (val < 0 || val > 1)
+		return -EINVAL;
+
+	lock_sock(sk);
+	msk->recvmsg_inq = !!val;
+	release_sock(sk);
+
+	return 0;
+}
+
 static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t optval,
 					       unsigned int optlen)
 {
@@ -784,21 +803,9 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 				    sockptr_t optval, unsigned int optlen)
 {
-	struct sock *sk = (void *)msk;
-	int ret, val;
-
 	switch (optname) {
 	case TCP_INQ:
-		ret = mptcp_get_int_option(msk, optval, optlen, &val);
-		if (ret)
-			return ret;
-		if (val < 0 || val > 1)
-			return -EINVAL;
-
-		lock_sock(sk);
-		msk->recvmsg_inq = !!val;
-		release_sock(sk);
-		return 0;
+		return mptcp_setsockopt_sol_tcp_inq(msk, optval, optlen);
 	case TCP_ULP:
 		return -EOPNOTSUPP;
 	case TCP_CONGESTION:

-- 
2.40.1

[PATCH mptcp-next 2/6] mptcp: sockopt: update supported list

[Matthieu Baerts]

IP_TOS, IPV6_TRANSPARENT and IPV6_FREEBIND are fully supported but we
forgot to move them to the "should work fine" section.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index a46a00bf0f08..af1d30c8832d 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -437,6 +437,7 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		/* should work fine */
 		case IP_FREEBIND:
 		case IP_TRANSPARENT:
+		case IP_TOS:
 
 		/* the following are control cmsg related */
 		case IP_PKTINFO:
@@ -450,7 +451,6 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case IP_RECVFRAGSIZE:
 
 		/* common stuff that need some love */
-		case IP_TOS:
 		case IP_TTL:
 		case IP_BIND_ADDRESS_NO_PORT:
 		case IP_MTU_DISCOVER:
@@ -479,7 +479,10 @@ static bool mptcp_supported_sockopt(int level, int optname)
 	}
 	if (level == SOL_IPV6) {
 		switch (optname) {
+		/* should work fine */
 		case IPV6_V6ONLY:
+		case IPV6_TRANSPARENT:
+		case IPV6_FREEBIND:
 
 		/* the following are control cmsg related */
 		case IPV6_RECVPKTINFO:
@@ -500,8 +503,6 @@ static bool mptcp_supported_sockopt(int level, int optname)
 
 		/* the following ones need some love but are quite common */
 		case IPV6_TCLASS:
-		case IPV6_TRANSPARENT:
-		case IPV6_FREEBIND:
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTOPTIONS:
 		case IPV6_UNICAST_HOPS:

-- 
2.40.1

[PATCH mptcp-next 3/6] mptcp: sockopt: get val in a generic way

[Matthieu Baerts]

Currently, to get socket option, we deal with them case by case by
looking at the field that has been set and copying what is done
elsewhere in SOL_IP(V6). That's probably why there is only one being
supported here with IP_TOS.

Instead, we can use ip(v6)_getsockopt() to retrieve the value. By doing
that, we can also go through the BPF et Netlink hooks if needed and we
can easily support new options later.

While at it, also fix the variable declaration order to respect the
reverse xmas tree convention.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index af1d30c8832d..673bcf8a97f0 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -868,13 +868,31 @@ int mptcp_setsockopt(struct sock *sk, int level, int optname,
 	return -EOPNOTSUPP;
 }
 
+static int mptcp_getsockopt_msk(struct mptcp_sock *msk, int level, int optname,
+			        char __user *optval, int __user *optlen)
+{
+	struct sock *sk = (struct sock *)msk;
+
+	/* We cannot use tcp_getsockopt() with the msk */
+	if (level == SOL_IP)
+		return ip_getsockopt(sk, level, optname, optval, optlen);
+
+	if (level == SOL_IPV6)
+		return ipv6_getsockopt(sk, level, optname, optval, optlen);
+
+	if (level == SOL_TCP)
+		return 0;
+
+	return -EOPNOTSUPP;
+}
+
 static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
 					  char __user *optval, int __user *optlen)
 {
 	struct sock *sk = (struct sock *)msk;
 	struct socket *ssock;
-	int ret;
 	struct sock *ssk;
+	int ret;
 
 	lock_sock(sk);
 	ssk = msk->first;
@@ -1348,11 +1366,9 @@ static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 static int mptcp_getsockopt_v4(struct mptcp_sock *msk, int optname,
 			       char __user *optval, int __user *optlen)
 {
-	struct sock *sk = (void *)msk;
-
 	switch (optname) {
 	case IP_TOS:
-		return mptcp_put_int_option(msk, optval, optlen, inet_sk(sk)->tos);
+		return mptcp_getsockopt_msk(msk, SOL_IP, optname, optval, optlen);
 	}
 
 	return -EOPNOTSUPP;

-- 
2.40.1

[PATCH mptcp-next 4/6] mptcp: sockopt: add missing getsockopt() options

[Matthieu Baerts]

Some socket options had a support for the "set" part but not the "get"
one:

 - IP_FREEBIND
 - IP_TRANSPARENT
 - IPV6_V6ONLY
 - IPV6_TRANSPARENT
 - IPV6_FREEBIND

Thanks to the previous commit, we can easily support them.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 673bcf8a97f0..f0007fa7a0a8 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1367,6 +1367,8 @@ static int mptcp_getsockopt_v4(struct mptcp_sock *msk, int optname,
 			       char __user *optval, int __user *optlen)
 {
 	switch (optname) {
+	case IP_FREEBIND:
+	case IP_TRANSPARENT:
 	case IP_TOS:
 		return mptcp_getsockopt_msk(msk, SOL_IP, optname, optval, optlen);
 	}
@@ -1374,6 +1376,19 @@ static int mptcp_getsockopt_v4(struct mptcp_sock *msk, int optname,
 	return -EOPNOTSUPP;
 }
 
+static int mptcp_getsockopt_v6(struct mptcp_sock *msk, int optname,
+			       char __user *optval, int __user *optlen)
+{
+	switch (optname) {
+	case IPV6_V6ONLY:
+	case IPV6_TRANSPARENT:
+	case IPV6_FREEBIND:
+		return mptcp_getsockopt_msk(msk, SOL_IPV6, optname, optval, optlen);
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int mptcp_getsockopt_sol_mptcp(struct mptcp_sock *msk, int optname,
 				      char __user *optval, int __user *optlen)
 {
@@ -1413,6 +1428,8 @@ int mptcp_getsockopt(struct sock *sk, int level, int optname,
 
 	if (level == SOL_IP)
 		return mptcp_getsockopt_v4(msk, optname, optval, option);
+	if (level == SOL_IPV6)
+		return mptcp_getsockopt_v6(msk, optname, optval, option);
 	if (level == SOL_TCP)
 		return mptcp_getsockopt_sol_tcp(msk, optname, optval, option);
 	if (level == SOL_MPTCP)

-- 
2.40.1

[PATCH mptcp-next 5/6] mptcp: sockopt: set val in a generic way

[Matthieu Baerts]

Currently, to set socket option, we mostly deal with them case by case
by setting the right field in the socket structure and in fact, copying
what is done elsewhere in SOL_IP(V6). Supporting a new option is then
more complex and we can see some disparities on how things are done
between the different options.

Instead, we can rely on ip(v6)_setsockopt() to set the different values.
By doing that, we uniform the way we deal with the different options and
we can also go through the BPF et Netlink hooks if needed. We can also
easily support new options later.

Note that there are still two cases where we still need to deal with the
end values directly, thus with something specific case by case:

- For SOL_TCP socket options because we cannot store them in the MPTCP
  socket. Note we can also store info in the different subflows (and
  create a first one if there is no subflow) and retrieve info from
  there.

- To sync the socket options when a new subflow has been created to
  minimise the work. But maybe it is fine to call some getsockopt() /
  setsockopt(). We could also save a list of setsockopt() that have been
  done in the past and only re-do these ones. That should help with the
  maintenance of these socket options and ease the support of new ones.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 148 ++++++++++++++++++----------------------------------
 1 file changed, 50 insertions(+), 98 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index f0007fa7a0a8..a17739683fdb 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -385,51 +385,6 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
 	return -EOPNOTSUPP;
 }
 
-static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
-			       sockptr_t optval, unsigned int optlen)
-{
-	struct sock *sk = (struct sock *)msk;
-	int ret = -EOPNOTSUPP;
-	struct socket *ssock;
-
-	switch (optname) {
-	case IPV6_V6ONLY:
-	case IPV6_TRANSPARENT:
-	case IPV6_FREEBIND:
-		lock_sock(sk);
-		ssock = __mptcp_nmpc_socket(msk);
-		if (IS_ERR(ssock)) {
-			release_sock(sk);
-			return PTR_ERR(ssock);
-		}
-
-		ret = tcp_setsockopt(ssock->sk, SOL_IPV6, optname, optval, optlen);
-		if (ret != 0) {
-			release_sock(sk);
-			return ret;
-		}
-
-		sockopt_seq_inc(msk);
-
-		switch (optname) {
-		case IPV6_V6ONLY:
-			sk->sk_ipv6only = ssock->sk->sk_ipv6only;
-			break;
-		case IPV6_TRANSPARENT:
-			inet_sk(sk)->transparent = inet_sk(ssock->sk)->transparent;
-			break;
-		case IPV6_FREEBIND:
-			inet_sk(sk)->freebind = inet_sk(ssock->sk)->freebind;
-			break;
-		}
-
-		release_sock(sk);
-		break;
-	}
-
-	return ret;
-}
-
 static bool mptcp_supported_sockopt(int level, int optname)
 {
 	if (level == SOL_IP) {
@@ -700,94 +655,65 @@ static int mptcp_setsockopt_sol_tcp_nodelay(struct mptcp_sock *msk, sockptr_t op
 	return 0;
 }
 
-static int mptcp_setsockopt_sol_ip_set_transparent(struct mptcp_sock *msk, int optname,
-						   sockptr_t optval, unsigned int optlen)
+static int mptcp_setsockopt_msk(struct mptcp_sock *msk, int level, int optname,
+				sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = (struct sock *)msk;
-	struct inet_sock *issk;
-	struct socket *ssock;
-	int err;
 
-	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
-	if (err != 0)
-		return err;
+	/* We cannot use tcp_setsockopt() with the msk */
+	if (level == SOL_IP)
+		return ip_setsockopt(sk, level, optname, optval, optlen);
 
-	lock_sock(sk);
+	if (level == SOL_IPV6)
+		return ipv6_setsockopt(sk, level, optname, optval, optlen);
 
-	ssock = __mptcp_nmpc_socket(msk);
-	if (IS_ERR(ssock)) {
-		release_sock(sk);
-		return PTR_ERR(ssock);
-	}
+	if (level == SOL_TCP)
+		return 0;
 
-	issk = inet_sk(ssock->sk);
-
-	switch (optname) {
-	case IP_FREEBIND:
-		issk->freebind = inet_sk(sk)->freebind;
-		break;
-	case IP_TRANSPARENT:
-		issk->transparent = inet_sk(sk)->transparent;
-		break;
-	default:
-		release_sock(sk);
-		WARN_ON_ONCE(1);
-		return -EOPNOTSUPP;
-	}
-
-	sockopt_seq_inc(msk);
-	release_sock(sk);
-	return 0;
+	return -EOPNOTSUPP;
 }
 
-static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname,
-				       sockptr_t optval, unsigned int optlen)
+static int mptcp_setsockopt_all_sf(struct mptcp_sock *msk, int level,
+				   int optname, sockptr_t optval,
+				   unsigned int optlen)
 {
 	struct mptcp_subflow_context *subflow;
 	struct sock *sk = (struct sock *)msk;
-	int err, val;
+	int err;
 
-	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
+	err = mptcp_setsockopt_msk(msk, level, optname, optval, optlen);
 
 	if (err != 0)
 		return err;
 
 	lock_sock(sk);
 	sockopt_seq_inc(msk);
-	val = inet_sk(sk)->tos;
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
-		__ip_sock_set_tos(ssk, val);
+		tcp_setsockopt(ssk, level, optname, optval, optlen);
 	}
 	release_sock(sk);
 
 	return 0;
 }
 
-static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
-			       sockptr_t optval, unsigned int optlen)
-{
-	switch (optname) {
-	case IP_FREEBIND:
-	case IP_TRANSPARENT:
-		return mptcp_setsockopt_sol_ip_set_transparent(msk, optname, optval, optlen);
-	case IP_TOS:
-		return mptcp_setsockopt_v4_set_tos(msk, optname, optval, optlen);
-	}
-
-	return -EOPNOTSUPP;
-}
-
 static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
 					  sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = (struct sock *)msk;
 	struct socket *sock;
+	struct sock *ssk;
 	int ret;
 
-	/* Limit to first subflow, before the connection establishment */
+	/* Limit to first subflow */
 	lock_sock(sk);
+	ssk = msk->first;
+	if (ssk) {
+		ret = tcp_setsockopt(ssk, level, optname, optval, optlen);
+		goto unlock;
+	}
+
 	sock = __mptcp_nmpc_socket(msk);
 	if (IS_ERR(sock)) {
 		ret = PTR_ERR(sock);
@@ -801,6 +727,32 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 	return ret;
 }
 
+static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
+			       sockptr_t optval, unsigned int optlen)
+{
+	switch (optname) {
+	case IP_FREEBIND:
+	case IP_TRANSPARENT:
+	case IP_TOS:
+		return mptcp_setsockopt_all_sf(msk, SOL_IP, optname, optval, optlen);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
+			       sockptr_t optval, unsigned int optlen)
+{
+	switch (optname) {
+	case IPV6_V6ONLY:
+	case IPV6_TRANSPARENT:
+	case IPV6_FREEBIND:
+		return mptcp_setsockopt_all_sf(msk, SOL_IPV6, optname, optval, optlen);
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 				    sockptr_t optval, unsigned int optlen)
 {

-- 
2.40.1

[PATCH mptcp-next 6/6] mptcp: sockopt: support IP_TTL & IPV6_UNICAST_HOPS

[Matthieu Baerts]

This adds support for IP_TTL and IPv6_UNICAST_HOPS, the IPv6 equivalent.

The socket option is propagated to all subflows by default.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/296
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index a17739683fdb..cf35775179ab 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -393,6 +393,7 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case IP_FREEBIND:
 		case IP_TRANSPARENT:
 		case IP_TOS:
+		case IP_TTL:
 
 		/* the following are control cmsg related */
 		case IP_PKTINFO:
@@ -406,7 +407,6 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case IP_RECVFRAGSIZE:
 
 		/* common stuff that need some love */
-		case IP_TTL:
 		case IP_BIND_ADDRESS_NO_PORT:
 		case IP_MTU_DISCOVER:
 		case IP_RECVERR:
@@ -438,6 +438,7 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case IPV6_V6ONLY:
 		case IPV6_TRANSPARENT:
 		case IPV6_FREEBIND:
+		case IPV6_UNICAST_HOPS:
 
 		/* the following are control cmsg related */
 		case IPV6_RECVPKTINFO:
@@ -460,7 +461,6 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case IPV6_TCLASS:
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTOPTIONS:
-		case IPV6_UNICAST_HOPS:
 		case IPV6_MTU_DISCOVER:
 		case IPV6_MTU:
 		case IPV6_RECVERR:
@@ -734,6 +734,7 @@ static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
 	case IP_FREEBIND:
 	case IP_TRANSPARENT:
 	case IP_TOS:
+	case IP_TTL:
 		return mptcp_setsockopt_all_sf(msk, SOL_IP, optname, optval, optlen);
 	}
 
@@ -747,6 +748,7 @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
 	case IPV6_V6ONLY:
 	case IPV6_TRANSPARENT:
 	case IPV6_FREEBIND:
+	case IPV6_UNICAST_HOPS:
 		return mptcp_setsockopt_all_sf(msk, SOL_IPV6, optname, optval, optlen);
 	}
 
@@ -1322,6 +1324,7 @@ static int mptcp_getsockopt_v4(struct mptcp_sock *msk, int optname,
 	case IP_FREEBIND:
 	case IP_TRANSPARENT:
 	case IP_TOS:
+	case IP_TTL:
 		return mptcp_getsockopt_msk(msk, SOL_IP, optname, optval, optlen);
 	}
 
@@ -1335,6 +1338,7 @@ static int mptcp_getsockopt_v6(struct mptcp_sock *msk, int optname,
 	case IPV6_V6ONLY:
 	case IPV6_TRANSPARENT:
 	case IPV6_FREEBIND:
+	case IPV6_UNICAST_HOPS:
 		return mptcp_getsockopt_msk(msk, SOL_IPV6, optname, optval, optlen);
 	}
 
@@ -1436,6 +1440,10 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
 
 	inet_sk(ssk)->transparent = inet_sk(sk)->transparent;
 	inet_sk(ssk)->freebind = inet_sk(sk)->freebind;
+	inet_sk(ssk)->uc_ttl = inet_sk(sk)->uc_ttl;
+
+	if (ssk->sk_family == AF_INET6)
+		inet6_sk(ssk)->hop_limit = inet6_sk(sk)->hop_limit;
 }
 
 static void __mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk)

-- 
2.40.1

Re: mptcp: sockopt: support IP_TTL & IPV6_UNICAST_HOPS: Build Failure

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/20230707-mptcp-unify-sockopt-issue-353-v1-6-693e15c06646@tessares.net/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/5488425457

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/25067ce9c170

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: mptcp: sockopt: support IP_TTL & IPV6_UNICAST_HOPS: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6207458729787392
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6207458729787392/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5363033799655424
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5363033799655424/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/5925983753076736
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5925983753076736/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4800083846234112
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4800083846234112/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/25067ce9c170


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[Paolo Abeni]

On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
> Here is the implementation of an idea I suggested a few months ago: it
> is a factoring of the sockopt code trying to uniform it, mainly to rely
> on {ip,ipv6,tcp}_[gs]etsockopt() instead of copying what is done there
> to get and set values.
> 
> That's a first step. More might come if we are OK with the approach,
> e.g.
> 
> - For SOL_TCP socket options we cannot store info in the MPTCP socket so
>   we still need to "manually" store it somewhere in the msk. Maybe we
>   could store info in the different subflows (and create a first one if
>   there is no subflow) and retrieve info from there?
> 
> - For SOL_SOCKET socket options, it looks like it should be feasible to
>   do something similar to what I did here but I'm probably missing
>   something. This could clean quite a lot of code, e.g. the last commit
>   from:
> 
>     https://github.com/matttbe/mptcp_net-next/commits/b4/mptcp-unify-sockopt-issue-353
> 
> - To sync the socket options when a new subflow has been created, for
>   the moment, we copy info from the msk to the subflow but maybe it
>    would be fine to call some getsockopt() / setsockopt() to do the
>   sync? If we take this approach, we might want to save a list of
>   setsockopt() that have been done in the past on this socket and only
>   re-do these ones? That should help with the maintenance of these
>   socket options and ease the support of new ones.

I would highly discourage this latest approach. We will have to limit
the list length, breaking the sync at some point. And there are few
quite gray area, e.g. how errors should be handled? I also fear the
"final" state of the subflow could be different from the
expected/correct one for non trivial socktopt. 


Cheers,

Paolo

Re: [PATCH mptcp-next 5/6] mptcp: sockopt: set val in a generic way

[Paolo Abeni]

On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
> Currently, to set socket option, we mostly deal with them case by case
> by setting the right field in the socket structure and in fact, copying
> what is done elsewhere in SOL_IP(V6). Supporting a new option is then
> more complex and we can see some disparities on how things are done
> between the different options.
> 
> Instead, we can rely on ip(v6)_setsockopt() to set the different values.
> By doing that, we uniform the way we deal with the different options and
> we can also go through the BPF et Netlink hooks if needed. We can also
> easily support new options later.
> 
> Note that there are still two cases where we still need to deal with the
> end values directly, thus with something specific case by case:
> 
> - For SOL_TCP socket options because we cannot store them in the MPTCP
>   socket. Note we can also store info in the different subflows (and
>   create a first one if there is no subflow) and retrieve info from
>   there.
> 
> - To sync the socket options when a new subflow has been created to
>   minimise the work. But maybe it is fine to call some getsockopt() /
>   setsockopt(). We could also save a list of setsockopt() that have been
>   done in the past and only re-do these ones. That should help with the
>   maintenance of these socket options and ease the support of new ones.
> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/353
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  net/mptcp/sockopt.c | 148 ++++++++++++++++++----------------------------------
>  1 file changed, 50 insertions(+), 98 deletions(-)
> 
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index f0007fa7a0a8..a17739683fdb 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -385,51 +385,6 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
>  	return -EOPNOTSUPP;
>  }
>  
> -static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
> -			       sockptr_t optval, unsigned int optlen)
> -{
> -	struct sock *sk = (struct sock *)msk;
> -	int ret = -EOPNOTSUPP;
> -	struct socket *ssock;
> -
> -	switch (optname) {
> -	case IPV6_V6ONLY:
> -	case IPV6_TRANSPARENT:
> -	case IPV6_FREEBIND:
> -		lock_sock(sk);
> -		ssock = __mptcp_nmpc_socket(msk);
> -		if (IS_ERR(ssock)) {
> -			release_sock(sk);
> -			return PTR_ERR(ssock);
> -		}
> -
> -		ret = tcp_setsockopt(ssock->sk, SOL_IPV6, optname, optval, optlen);
> -		if (ret != 0) {
> -			release_sock(sk);
> -			return ret;
> -		}
> -
> -		sockopt_seq_inc(msk);
> -
> -		switch (optname) {
> -		case IPV6_V6ONLY:
> -			sk->sk_ipv6only = ssock->sk->sk_ipv6only;
> -			break;
> -		case IPV6_TRANSPARENT:
> -			inet_sk(sk)->transparent = inet_sk(ssock->sk)->transparent;
> -			break;
> -		case IPV6_FREEBIND:
> -			inet_sk(sk)->freebind = inet_sk(ssock->sk)->freebind;
> -			break;
> -		}
> -
> -		release_sock(sk);
> -		break;
> -	}
> -
> -	return ret;
> -}
> -
>  static bool mptcp_supported_sockopt(int level, int optname)
>  {
>  	if (level == SOL_IP) {
> @@ -700,94 +655,65 @@ static int mptcp_setsockopt_sol_tcp_nodelay(struct mptcp_sock *msk, sockptr_t op
>  	return 0;
>  }
>  
> -static int mptcp_setsockopt_sol_ip_set_transparent(struct mptcp_sock *msk, int optname,
> -						   sockptr_t optval, unsigned int optlen)
> +static int mptcp_setsockopt_msk(struct mptcp_sock *msk, int level, int optname,
> +				sockptr_t optval, unsigned int optlen)
>  {
>  	struct sock *sk = (struct sock *)msk;
> -	struct inet_sock *issk;
> -	struct socket *ssock;
> -	int err;
>  
> -	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
> -	if (err != 0)
> -		return err;
> +	/* We cannot use tcp_setsockopt() with the msk */
> +	if (level == SOL_IP)
> +		return ip_setsockopt(sk, level, optname, optval, optlen);
>  
> -	lock_sock(sk);
> +	if (level == SOL_IPV6)
> +		return ipv6_setsockopt(sk, level, optname, optval, optlen);
>  
> -	ssock = __mptcp_nmpc_socket(msk);
> -	if (IS_ERR(ssock)) {
> -		release_sock(sk);
> -		return PTR_ERR(ssock);
> -	}
> +	if (level == SOL_TCP)
> +		return 0;
>  
> -	issk = inet_sk(ssock->sk);
> -
> -	switch (optname) {
> -	case IP_FREEBIND:
> -		issk->freebind = inet_sk(sk)->freebind;
> -		break;
> --	case IP_TRANSPARENT:
> -		issk->transparent = inet_sk(sk)->transparent;
> -		break;
> -	default:
> -		release_sock(sk);
> -		WARN_ON_ONCE(1);
> -		return -EOPNOTSUPP;
> -	}
> -
> -	sockopt_seq_inc(msk);
> -	release_sock(sk);
> -	return 0;
> +	return -EOPNOTSUPP;
>  }
>  
> -static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname,
> -				       sockptr_t optval, unsigned int optlen)
> +static int mptcp_setsockopt_all_sf(struct mptcp_sock *msk, int level,
> +				   int optname, sockptr_t optval,
> +				   unsigned int optlen)
>  {
>  	struct mptcp_subflow_context *subflow;
>  	struct sock *sk = (struct sock *)msk;
> -	int err, val;
> +	int err;
>  
> -	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
> +	err = mptcp_setsockopt_msk(msk, level, optname, optval, optlen);
>  
>  	if (err != 0)
>  		return err;
>  
>  	lock_sock(sk);
>  	sockopt_seq_inc(msk);
> -	val = inet_sk(sk)->tos;
>  	mptcp_for_each_subflow(msk, subflow) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  
> -		__ip_sock_set_tos(ssk, val);
> +		tcp_setsockopt(ssk, level, optname, optval, optlen);

The above ignores the tcp_setsockopt() return value. What if the
sockopt fails on a single subflow? e.g. the 2nd one?

Perhaps not possible with the currently supported sockopt, but if the
goal is to extend the support in a generic way, we will likely hit
that. Note that even saving and e.v. restoring the value on failure is
prone to the same problem (restoring could fail, too).

>  	}
>  	release_sock(sk);
>  
>  	return 0;
>  }
>  
> -static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
> -			       sockptr_t optval, unsigned int optlen)
> -{
> -	switch (optname) {
> -	case IP_FREEBIND:
> -	case IP_TRANSPARENT:
> -		return mptcp_setsockopt_sol_ip_set_transparent(msk, optname, optval, optlen);
> -	case IP_TOS:
> -		return mptcp_setsockopt_v4_set_tos(msk, optname, optval, optlen);
> -	}
> -
> -	return -EOPNOTSUPP;
> -}
> -
>  static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
>  					  sockptr_t optval, unsigned int optlen)
>  {
>  	struct sock *sk = (struct sock *)msk;
>  	struct socket *sock;
> +	struct sock *ssk;
>  	int ret;
>  
> -	/* Limit to first subflow, before the connection establishment */
> +	/* Limit to first subflow */
>  	lock_sock(sk);
> +	ssk = msk->first;
> +	if (ssk) {
> +		ret = tcp_setsockopt(ssk, level, optname, optval, optlen);
> +		goto unlock;
> +	}

Why do you need the above chunk? This change the behavior of existing
sockopt and possibly their return value in case of error?!?

> +
>  	sock = __mptcp_nmpc_socket(msk);
>  	if (IS_ERR(sock)) {
>  		ret = PTR_ERR(sock);
> @@ -801,6 +727,32 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  	return ret;
>  }
>  
> +static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
> +			       sockptr_t optval, unsigned int optlen)
> +{
> +	switch (optname) {
> +	case IP_FREEBIND:
> +	case IP_TRANSPARENT:
> +	case IP_TOS:
> +		return mptcp_setsockopt_all_sf(msk, SOL_IP, optname, optval, optlen);

This changes the sockopt semantic a bit, applying the
IP_FREEBIND/IP_TRANSPARENT to all the subflows, while before this
change, only the MPC subflow was rightfull affected. Likely in practice
there are no visible functional effect change, but it sounds confusing
to me.

Cheers,

Paolo

Re: mptcp: sockopt: support IP_TTL & IPV6_UNICAST_HOPS: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6207458729787392
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6207458729787392/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5363033799655424
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5363033799655424/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5364691875135488
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5364691875135488/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4800083846234112
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4800083846234112/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/25067ce9c170


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next 5/6] mptcp: sockopt: set val in a generic way

[Matthieu Baerts]

Hi Paolo,

Thank you for the reviews!

On 10/07/2023 19:05, Paolo Abeni wrote:
> On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
>> Currently, to set socket option, we mostly deal with them case by case
>> by setting the right field in the socket structure and in fact, copying
>> what is done elsewhere in SOL_IP(V6). Supporting a new option is then
>> more complex and we can see some disparities on how things are done
>> between the different options.
>>
>> Instead, we can rely on ip(v6)_setsockopt() to set the different values.
>> By doing that, we uniform the way we deal with the different options and
>> we can also go through the BPF et Netlink hooks if needed. We can also
>> easily support new options later.

(...)

>> -static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname,
>> -				       sockptr_t optval, unsigned int optlen)
>> +static int mptcp_setsockopt_all_sf(struct mptcp_sock *msk, int level,
>> +				   int optname, sockptr_t optval,
>> +				   unsigned int optlen)
>>  {
>>  	struct mptcp_subflow_context *subflow;
>>  	struct sock *sk = (struct sock *)msk;
>> -	int err, val;
>> +	int err;
>>  
>> -	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
>> +	err = mptcp_setsockopt_msk(msk, level, optname, optval, optlen);
>>  
>>  	if (err != 0)
>>  		return err;
>>  
>>  	lock_sock(sk);
>>  	sockopt_seq_inc(msk);
>> -	val = inet_sk(sk)->tos;
>>  	mptcp_for_each_subflow(msk, subflow) {
>>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>>  
>> -		__ip_sock_set_tos(ssk, val);
>> +		tcp_setsockopt(ssk, level, optname, optval, optlen);
> 
> The above ignores the tcp_setsockopt() return value. What if the
> sockopt fails on a single subflow? e.g. the 2nd one?
> 
> Perhaps not possible with the currently supported sockopt, but if the
> goal is to extend the support in a generic way, we will likely hit
> that. Note that even saving and e.v. restoring the value on failure is
> prone to the same problem (restoring could fail, too).

That was somehow the behaviour we had before: we use setsockopt() on the
MPTCP sock and we stop if there is an error. If not, it means the
parameters are fine and we can propagate the value to all subflows.
Before, we were not looking for errors when setting the value on the
different subflows.

Now we use the "normal" flow to set this parameter on the different
subflows and if the corresponding sockopt has additional checks before
setting the value (or if some are added later and we missed that),
that's even better because we are no longer forcing the modification of
a parameter if it was not supposed to be done :)

I'm not a big fan of ignoring errors but as you said, we cannot return
all the different errors with the current API. Or we would need
something like:
  > ret = setsockopt(fd, SOL_MPTCP, MPTCP_SETSOCKOPT, {level=X, name=Y,
val=&Z, err=&E}, len);

But that's a different topic and a new API specific to MPTCP.

>>  	}
>>  	release_sock(sk);
>>  
>>  	return 0;
>>  }
>>  
>> -static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
>> -			       sockptr_t optval, unsigned int optlen)
>> -{
>> -	switch (optname) {
>> -	case IP_FREEBIND:
>> -	case IP_TRANSPARENT:
>> -		return mptcp_setsockopt_sol_ip_set_transparent(msk, optname, optval, optlen);
>> -	case IP_TOS:
>> -		return mptcp_setsockopt_v4_set_tos(msk, optname, optval, optlen);
>> -	}
>> -
>> -	return -EOPNOTSUPP;
>> -}
>> -
>>  static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname,
>>  					  sockptr_t optval, unsigned int optlen)
>>  {
>>  	struct sock *sk = (struct sock *)msk;
>>  	struct socket *sock;
>> +	struct sock *ssk;
>>  	int ret;
>>  
>> -	/* Limit to first subflow, before the connection establishment */
>> +	/* Limit to first subflow */
>>  	lock_sock(sk);
>> +	ssk = msk->first;
>> +	if (ssk) {
>> +		ret = tcp_setsockopt(ssk, level, optname, optval, optlen);
>> +		goto unlock;
>> +	}
> 
> Why do you need the above chunk? This change the behavior of existing
> sockopt and possibly their return value in case of error?!?

I thought we could only call __mptcp_nmpc_socket() before the
establishment of the connection and I wanted to be able to call
setsockopt() on the first subflow even if it has been done after the
establishment of the connection. I'm maybe getting confused by the
modifications you did around __mptcp_nmpc_socket().

But now that I'm thinking about that, maybe it doesn't make sense what I
did: if we only want to act on the first subflow, it is certainly
because we want to do something before the establishment of the MPTCP
connection, no?

But then if the connection has been established before and if we return
before doing the call to tcp_setsockopt(), we might return a different
error with MPTCP than with TCP, no?

>> +
>>  	sock = __mptcp_nmpc_socket(msk);
>>  	if (IS_ERR(sock)) {
>>  		ret = PTR_ERR(sock);
>> @@ -801,6 +727,32 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>>  	return ret;
>>  }
>>  
>> +static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
>> +			       sockptr_t optval, unsigned int optlen)
>> +{
>> +	switch (optname) {
>> +	case IP_FREEBIND:
>> +	case IP_TRANSPARENT:
>> +	case IP_TOS:
>> +		return mptcp_setsockopt_all_sf(msk, SOL_IP, optname, optval, optlen);
> 
> This changes the sockopt semantic a bit, applying the
> IP_FREEBIND/IP_TRANSPARENT to all the subflows, while before this
> change, only the MPC subflow was rightfull affected. Likely in practice
> there are no visible functional effect change, but it sounds confusing
> to me.

Good point. Indeed, if there is no first subflow created before the
establishment of the connection, we will now only set the transparent
and freebind bits on the MPTCP socket. They will be set when a new
subflow has been created in the "sync()" part (which is maybe fine?)

But we can call mptcp_setsockopt_first_sf_only() here if you prefer.

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

Re: [PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[Matthieu Baerts]

Hi Paolo,

Thank you for your feedback!

On 10/07/2023 18:51, Paolo Abeni wrote:
> On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
>> Here is the implementation of an idea I suggested a few months ago: it
>> is a factoring of the sockopt code trying to uniform it, mainly to rely
>> on {ip,ipv6,tcp}_[gs]etsockopt() instead of copying what is done there
>> to get and set values.
>>
>> That's a first step. More might come if we are OK with the approach,
>> e.g.
>>
>> - For SOL_TCP socket options we cannot store info in the MPTCP socket so
>>   we still need to "manually" store it somewhere in the msk. Maybe we
>>   could store info in the different subflows (and create a first one if
>>   there is no subflow) and retrieve info from there?
>>
>> - For SOL_SOCKET socket options, it looks like it should be feasible to
>>   do something similar to what I did here but I'm probably missing
>>   something. This could clean quite a lot of code, e.g. the last commit
>>   from:
>>
>>     https://github.com/matttbe/mptcp_net-next/commits/b4/mptcp-unify-sockopt-issue-353
>>
>> - To sync the socket options when a new subflow has been created, for
>>   the moment, we copy info from the msk to the subflow but maybe it
>>    would be fine to call some getsockopt() / setsockopt() to do the
>>   sync? If we take this approach, we might want to save a list of
>>   setsockopt() that have been done in the past on this socket and only
>>   re-do these ones? That should help with the maintenance of these
>>   socket options and ease the support of new ones.
> 
> I would highly discourage this latest approach.

Before going further, I want to insist on the fact that it is just an
idea, I didn't look at the implementation (and to be honest, I was not
looking at doing that now :) )

> We will have to limit
> the list length, breaking the sync at some point.

Why would we have to limit the list length and break the sync?

I understand having a list might not feel good but I'm not sure what
else we can have.

> And there are few
> quite gray area, e.g. how errors should be handled?

Same as what we have now I guess: errors are ignored :)
But I don't see what else we can and it doesn't feel worst than before
(regarding how we handle errors during the sync)

> I also fear the
> "final" state of the subflow could be different from the
> expected/correct one for non trivial socktopt.

I would expect it to be better because we would follow the "normal" flow
to set values. What bother me is the fact we bypass checks and directly
set values. If the code change on the different IP/TCP/SOCKET socket
options side, we might (very likely) miss the fact we also need to
update what we are doing on the MPTCP side, e.g. if new checks or extra
actions (BPF/Netfilter hooks) are added later, etc. So the idea of this
approach is to ease the maintenance (but maybe that's not needed and/or
not worth it) :)

Of course, this would work only if we can do something like *only* for
each setsockopt() that have been done before:

  void sync_sockopt(sock, level, optname)
  {
    socklen_t buflen;
    char val[1024]; // we can also record the max given before

    buflen = sizeof(val);

    if ((err = getsockopt(sock, level, optname, val, &buflen) < 0))
        return;

    setsockopt(sock, level, optname, val, buflen);
  }


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

Re: [PATCH mptcp-next 5/6] mptcp: sockopt: set val in a generic way

[Matthieu Baerts]

Hi Paolo,

On 11/07/2023 11:43, Matthieu Baerts wrote:
> On 10/07/2023 19:05, Paolo Abeni wrote:
>> On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:

(...)

>>> @@ -801,6 +727,32 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>>>  	return ret;
>>>  }
>>>  
>>> +static int mptcp_setsockopt_v4(struct mptcp_sock *msk, int optname,
>>> +			       sockptr_t optval, unsigned int optlen)
>>> +{
>>> +	switch (optname) {
>>> +	case IP_FREEBIND:
>>> +	case IP_TRANSPARENT:
>>> +	case IP_TOS:
>>> +		return mptcp_setsockopt_all_sf(msk, SOL_IP, optname, optval, optlen);
>>
>> This changes the sockopt semantic a bit, applying the
>> IP_FREEBIND/IP_TRANSPARENT to all the subflows, while before this
>> change, only the MPC subflow was rightfull affected. Likely in practice
>> there are no visible functional effect change, but it sounds confusing
>> to me.
> 
> Good point. Indeed, if there is no first subflow created before the
> establishment of the connection, we will now only set the transparent
> and freebind bits on the MPTCP socket. They will be set when a new
> subflow has been created in the "sync()" part (which is maybe fine?)

Thinking about that, with the same logic (the sync will be done later),
we could also only call mptcp_setsockopt_msk() and avoid a lock (on a
non established MPTCP connection).

> But we can call mptcp_setsockopt_first_sf_only() here if you prefer.

(We would also need to call mptcp_setsockopt_msk() as well)

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

Re: [PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[Paolo Abeni]

On Tue, 2023-07-11 at 12:22 +0200, Matthieu Baerts wrote:
> On 10/07/2023 18:51, Paolo Abeni wrote:
> > On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
> > > Here is the implementation of an idea I suggested a few months ago: it
> > > is a factoring of the sockopt code trying to uniform it, mainly to rely
> > > on {ip,ipv6,tcp}_[gs]etsockopt() instead of copying what is done there
> > > to get and set values.
> > > 
> > > That's a first step. More might come if we are OK with the approach,
> > > e.g.
> > > 
> > > - For SOL_TCP socket options we cannot store info in the MPTCP socket so
> > >   we still need to "manually" store it somewhere in the msk. Maybe we
> > >   could store info in the different subflows (and create a first one if
> > >   there is no subflow) and retrieve info from there?
> > > 
> > > - For SOL_SOCKET socket options, it looks like it should be feasible to
> > >   do something similar to what I did here but I'm probably missing
> > >   something. This could clean quite a lot of code, e.g. the last commit
> > >   from:
> > > 
> > >     https://github.com/matttbe/mptcp_net-next/commits/b4/mptcp-unify-sockopt-issue-353
> > > 
> > > - To sync the socket options when a new subflow has been created, for
> > >   the moment, we copy info from the msk to the subflow but maybe it
> > >    would be fine to call some getsockopt() / setsockopt() to do the
> > >   sync? If we take this approach, we might want to save a list of
> > >   setsockopt() that have been done in the past on this socket and only
> > >   re-do these ones? That should help with the maintenance of these
> > >   socket options and ease the support of new ones.
> > 
> > I would highly discourage this latest approach.
> 
> Before going further, I want to insist on the fact that it is just an
> idea, I didn't look at the implementation (and to be honest, I was not
> looking at doing that now :) )
> 
> > We will have to limit
> > the list length, breaking the sync at some point.
> 
> Why would we have to limit the list length and break the sync?
> 
> I understand having a list might not feel good but I'm not sure what
> else we can have.

Because otherwise syzkaller (or evil unprivileged user-space program)
can easily exhaust the kernel memory doing a lot of setsockopt: we need
to allocate the list entry, and we can't never flush the list.

> > And there are few
> > quite gray area, e.g. how errors should be handled?
> 
> Same as what we have now I guess: errors are ignored :)
> But I don't see what else we can and it doesn't feel worst than before
> (regarding how we handle errors during the sync)

Note the with the current schema sync can fail only while setting the
CA, and even that requires a quite non trivial setup. If we start
replying arbitrary complex sockopt sequence, I fear the we can hit
failures much more frequently.

> 
> > I also fear the
> > "final" state of the subflow could be different from the
> > expected/correct one for non trivial socktopt.
> 
> I would expect it to be better because we would follow the "normal" flow
> to set values. What bother me is the fact we bypass checks and directly
> set values. 

Actually we don't! Such checks are performed once for the sockopt on
the main/initial subflow (or by the mptcp impl).

IMHO it's safer then the list alternative. For passive socket the
latter will do the full sockopt checking using a different initial sock
internal state: the sockopt will be applied after cloning and the
related initialization.

For plain TCP sockopt value are inherited from the parent via the clone
itself, which is quite near to what the current sync implementation is
doing.

> If the code change on the different IP/TCP/SOCKET socket
> options side, we might (very likely) miss the fact we also need to
> update what we are doing on the MPTCP side, e.g. if new checks or extra
> actions (BPF/Netfilter hooks) are added later, etc. So the idea of this
> approach is to ease the maintenance (but maybe that's not needed and/or
> not worth it) :)

The eBPF maintainer mentioned that new hooks are discouraged, IIRC ;)


Cheers,

Paolo

Re: [PATCH mptcp-next 0/6] mptcp: sockopt: uniform code to get/set values

[Matthieu Baerts]

On 11/07/2023 14:50, Paolo Abeni wrote:
> On Tue, 2023-07-11 at 12:22 +0200, Matthieu Baerts wrote:
>> On 10/07/2023 18:51, Paolo Abeni wrote:
>>> On Fri, 2023-07-07 at 18:00 +0200, Matthieu Baerts wrote:
>>>> Here is the implementation of an idea I suggested a few months ago: it
>>>> is a factoring of the sockopt code trying to uniform it, mainly to rely
>>>> on {ip,ipv6,tcp}_[gs]etsockopt() instead of copying what is done there
>>>> to get and set values.
>>>>
>>>> That's a first step. More might come if we are OK with the approach,
>>>> e.g.
>>>>
>>>> - For SOL_TCP socket options we cannot store info in the MPTCP socket so
>>>>   we still need to "manually" store it somewhere in the msk. Maybe we
>>>>   could store info in the different subflows (and create a first one if
>>>>   there is no subflow) and retrieve info from there?
>>>>
>>>> - For SOL_SOCKET socket options, it looks like it should be feasible to
>>>>   do something similar to what I did here but I'm probably missing
>>>>   something. This could clean quite a lot of code, e.g. the last commit
>>>>   from:
>>>>
>>>>     https://github.com/matttbe/mptcp_net-next/commits/b4/mptcp-unify-sockopt-issue-353
>>>>
>>>> - To sync the socket options when a new subflow has been created, for
>>>>   the moment, we copy info from the msk to the subflow but maybe it
>>>>    would be fine to call some getsockopt() / setsockopt() to do the
>>>>   sync? If we take this approach, we might want to save a list of
>>>>   setsockopt() that have been done in the past on this socket and only
>>>>   re-do these ones? That should help with the maintenance of these
>>>>   socket options and ease the support of new ones.
>>>
>>> I would highly discourage this latest approach.
>>
>> Before going further, I want to insist on the fact that it is just an
>> idea, I didn't look at the implementation (and to be honest, I was not
>> looking at doing that now :) )
>>
>>> We will have to limit
>>> the list length, breaking the sync at some point.
>>
>> Why would we have to limit the list length and break the sync?
>>
>> I understand having a list might not feel good but I'm not sure what
>> else we can have.
> 
> Because otherwise syzkaller (or evil unprivileged user-space program)
> can easily exhaust the kernel memory doing a lot of setsockopt: we need
> to allocate the list entry, and we can't never flush the list.

I see, thank you. We could then add items (with just "level" and
"optname") in the list only if the initial setsockopt() operation was a
success and if there was no previous entry with the same info.

But I see the risk.

>>> And there are few
>>> quite gray area, e.g. how errors should be handled?
>>
>> Same as what we have now I guess: errors are ignored :)
>> But I don't see what else we can and it doesn't feel worst than before
>> (regarding how we handle errors during the sync)
> 
> Note the with the current schema sync can fail only while setting the
> CA, and even that requires a quite non trivial setup. If we start
> replying arbitrary complex sockopt sequence, I fear the we can hit
> failures much more frequently.

Maybe. We can also start with an "allow list".

>>> I also fear the
>>> "final" state of the subflow could be different from the
>>> expected/correct one for non trivial socktopt.
>>
>> I would expect it to be better because we would follow the "normal" flow
>> to set values. What bother me is the fact we bypass checks and directly
>> set values. 
> 
> Actually we don't! Such checks are performed once for the sockopt on
> the main/initial subflow (or by the mptcp impl).
> 
> IMHO it's safer then the list alternative. For passive socket the
> latter will do the full sockopt checking using a different initial sock
> internal state: the sockopt will be applied after cloning and the
> related initialization.
> 
> For plain TCP sockopt value are inherited from the parent via the clone
> itself, which is quite near to what the current sync implementation is
> doing.

Yes but that's only good for passive sockets. It would be nice if we
could clone all the attributes of the first subflow when new ones are
created :)

It might be safer now but if we don't follow the code we duplicated from
the other layers (i.e. what is exactly done with a TCP socket when a
setsockopt() is done), it might be less safe later than changing the
different values using the sockopt API.

>> If the code change on the different IP/TCP/SOCKET socket
>> options side, we might (very likely) miss the fact we also need to
>> update what we are doing on the MPTCP side, e.g. if new checks or extra
>> actions (BPF/Netfilter hooks) are added later, etc. So the idea of this
>> approach is to ease the maintenance (but maybe that's not needed and/or
>> not worth it) :)
> 
> The eBPF maintainer mentioned that new hooks are discouraged, IIRC ;)

Until someone comes with the new extended eBPF technology :-P

No but what is done by the kernel when a "setsockopt()" is emitted could
be modified, e.g. if the value is no longer a bit but an enum or if a
security check needs to be done before, etc. (Or maybe I'm thinking
about unrealistic/very unlikely/forbidden modifications)?

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