Re: [MPTCP] [PATCH v2] mptcp: sockopt: pass whitelisted setsockopt to subflows

Re: [MPTCP] [PATCH v2] mptcp: sockopt: pass whitelisted setsockopt to subflows

[Matthieu Baerts]

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

Hi Florian, Mat,

On 30/08/2019 00:44, Florian Westphal wrote:
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
>> On Fri, 16 Aug 2019, Florian Westphal wrote:
>>
>>> This allows to set tcp congestion control algorithm on all subflows.
>>>
>>> In the future we could keep a record of sockopts so we can "replay"
>>> them when another subflow is added at a later point in time.
>>>
>>> Only TCP_CONGESTION is allowed at the moment, as some available
>>> TCP knobs either have undesireable behaviour or should be later
>>> implemented at the mptcp level rather than passing them through to
>>> the subflows (TCP_CORK for example).
>>>
>>> getsockopt is disabled for the time being to not expose a particular
>>> behaviour ("first subflow on list") at this time.
>>>
>>> v2: disable getsockopt, restrict setsockopt via whitelist.
>>
>> As Florian mentioned on the call today, we definitely want to merge the
>> getsockopt fix.

Thank you for the patch and the review!

>> As for the setsockopt part, if TCP_CONGESTION is sufficiently useful to
>> enough users, we could consider merging it. My slight hesitation is that the
>> potential "replay" behavior change in the future could come as a surprise,
>> and if we start out with this ability to set options on all subflows then we
>> have to be careful and consistent about how it interacts with per-subflow
>> options later. Anyone know if TCP_CONGESTION is a priority?
> 
> Its not.
> 
> Lets drop this, I will respin with the getsockopt fix only.
> 
> Alternatively, Matthieu -- if you can just apply the getsockopt part
> of this patch that works for me too.

Sure, just did:
- 8d057956303c: "squashed" in "mptcp: Make connection_list a real list
of subflows"
- 6090eb26ae22: signed-off
- 72da334cdf7c..732bbb964f61: result

Just exported, tests in progress.

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 v2] mptcp: sockopt: pass whitelisted setsockopt to subflows

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> On Fri, 16 Aug 2019, Florian Westphal wrote:
> 
> > This allows to set tcp congestion control algorithm on all subflows.
> > 
> > In the future we could keep a record of sockopts so we can "replay"
> > them when another subflow is added at a later point in time.
> > 
> > Only TCP_CONGESTION is allowed at the moment, as some available
> > TCP knobs either have undesireable behaviour or should be later
> > implemented at the mptcp level rather than passing them through to
> > the subflows (TCP_CORK for example).
> > 
> > getsockopt is disabled for the time being to not expose a particular
> > behaviour ("first subflow on list") at this time.
> > 
> > v2: disable getsockopt, restrict setsockopt via whitelist.
> 
> As Florian mentioned on the call today, we definitely want to merge the
> getsockopt fix.
> 
> As for the setsockopt part, if TCP_CONGESTION is sufficiently useful to
> enough users, we could consider merging it. My slight hesitation is that the
> potential "replay" behavior change in the future could come as a surprise,
> and if we start out with this ability to set options on all subflows then we
> have to be careful and consistent about how it interacts with per-subflow
> options later. Anyone know if TCP_CONGESTION is a priority?

Its not.

Lets drop this, I will respin with the getsockopt fix only.

Alternatively, Matthieu -- if you can just apply the getsockopt part
of this patch that works for me too.

Re: [MPTCP] [PATCH v2] mptcp: sockopt: pass whitelisted setsockopt to subflows

[Mat Martineau]

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

On Fri, 16 Aug 2019, Florian Westphal wrote:

> This allows to set tcp congestion control algorithm on all subflows.
>
> In the future we could keep a record of sockopts so we can "replay"
> them when another subflow is added at a later point in time.
>
> Only TCP_CONGESTION is allowed at the moment, as some available
> TCP knobs either have undesireable behaviour or should be later
> implemented at the mptcp level rather than passing them through to
> the subflows (TCP_CORK for example).
>
> getsockopt is disabled for the time being to not expose a particular
> behaviour ("first subflow on list") at this time.
>
> v2: disable getsockopt, restrict setsockopt via whitelist.

As Florian mentioned on the call today, we definitely want to merge the 
getsockopt fix.

As for the setsockopt part, if TCP_CONGESTION is sufficiently useful to 
enough users, we could consider merging it. My slight hesitation is that 
the potential "replay" behavior change in the future could come as a 
surprise, and if we start out with this ability to set options on all 
subflows then we have to be careful and consistent about how it interacts 
with per-subflow options later. Anyone know if TCP_CONGESTION is a 
priority?

Mat


>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/mptcp/protocol.c | 51 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index aaa71b161c1a..a8cbd9741c32 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -707,13 +707,31 @@ static void mptcp_destroy(struct sock *sk)
> 	token_destroy(msk->token);
> }
>
> +
> +/* Only pass a small subset of level/optnames that are considered safe.
> + * We e.g. don't want MD5SIG (option space too limited) or TCP_CORK
> + * (meaning not entirely clear from mptcp point of view).
> + */
> +static bool pass_to_subflows(int level, int optname)
> +{
> +	if (level != SOL_TCP)
> +		return false;
> +
> +	switch (optname) {
> +	case TCP_CONGESTION: return true;
> +	}
> +
> +	return false;
> +}
> +
> static int mptcp_setsockopt(struct sock *sk, int level, int optname,
> 			    char __user *uoptval, unsigned int optlen)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> +	const struct subflow_context *subflow;
> 	char __kernel *optval;
> 	struct socket *ssock;
> -	int ret;
> +	int ret, err, subflows;
>
> 	/* will be treated as __user in tcp_setsockopt */
> 	optval = (char __kernel __force *)uoptval;
> @@ -729,8 +747,33 @@ static int mptcp_setsockopt(struct sock *sk, int level, int optname,
>
> 	/* @@ the meaning of setsockopt() when the socket is connected and
> 	 * there are multiple subflows is not defined.
> +	 *
> +	 * We attempt to perfom the operation on all current subflows, but
> +	 * only return error if all subflow setsockopt failed.
> 	 */
> -	return 0;
> +	if (!pass_to_subflows(level, optname))
> +		return -EOPNOTSUPP;
> +
> +	ret = 0;
> +	err = 0;
> +	subflows = 0;
> +	lock_sock(sk);
> +	list_for_each_entry(subflow, &msk->conn_list, node) {
> +		struct socket *tcp_sk = mptcp_subflow_tcp_socket(subflow);
> +		int tmp;
> +
> +		tmp = kernel_setsockopt(tcp_sk, level, optname, optval, optlen);
> +		if (tmp == 0)
> +			subflows++;
> +		else if (err == 0)
> +			err = tmp;
> +	}
> +	release_sock(sk);
> +
> +	if (subflows == 0 && err)
> +		ret = err;
> +
> +	return ret;
> }
>
> static int mptcp_getsockopt(struct sock *sk, int level, int optname,
> @@ -755,10 +798,10 @@ static int mptcp_getsockopt(struct sock *sk, int level, int optname,
> 		return ret;
> 	}
>
> -	/* @@ the meaning of setsockopt() when the socket is connected and
> +	/* @@ the meaning of getsockopt() when the socket is connected and
> 	 * there are multiple subflows is not defined.
> 	 */
> -	return 0;
> +	return -EOPNOTSUPP;
> }
>
> static int mptcp_get_port(struct sock *sk, unsigned short snum)
> -- 
> 2.21.0
>
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp
>

--
Mat Martineau
Intel

[MPTCP] [PATCH v2] mptcp: sockopt: pass whitelisted setsockopt to subflows

[Florian Westphal]

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

This allows to set tcp congestion control algorithm on all subflows.

In the future we could keep a record of sockopts so we can "replay"
them when another subflow is added at a later point in time.

Only TCP_CONGESTION is allowed at the moment, as some available
TCP knobs either have undesireable behaviour or should be later
implemented at the mptcp level rather than passing them through to
the subflows (TCP_CORK for example).

getsockopt is disabled for the time being to not expose a particular
behaviour ("first subflow on list") at this time.

v2: disable getsockopt, restrict setsockopt via whitelist.

Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 net/mptcp/protocol.c | 51 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index aaa71b161c1a..a8cbd9741c32 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -707,13 +707,31 @@ static void mptcp_destroy(struct sock *sk)
 	token_destroy(msk->token);
 }
 
+
+/* Only pass a small subset of level/optnames that are considered safe.
+ * We e.g. don't want MD5SIG (option space too limited) or TCP_CORK
+ * (meaning not entirely clear from mptcp point of view).
+ */
+static bool pass_to_subflows(int level, int optname)
+{
+	if (level != SOL_TCP)
+		return false;
+
+	switch (optname) {
+	case TCP_CONGESTION: return true;
+	}
+
+	return false;
+}
+
 static int mptcp_setsockopt(struct sock *sk, int level, int optname,
 			    char __user *uoptval, unsigned int optlen)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
+	const struct subflow_context *subflow;
 	char __kernel *optval;
 	struct socket *ssock;
-	int ret;
+	int ret, err, subflows;
 
 	/* will be treated as __user in tcp_setsockopt */
 	optval = (char __kernel __force *)uoptval;
@@ -729,8 +747,33 @@ static int mptcp_setsockopt(struct sock *sk, int level, int optname,
 
 	/* @@ the meaning of setsockopt() when the socket is connected and
 	 * there are multiple subflows is not defined.
+	 *
+	 * We attempt to perfom the operation on all current subflows, but
+	 * only return error if all subflow setsockopt failed.
 	 */
-	return 0;
+	if (!pass_to_subflows(level, optname))
+		return -EOPNOTSUPP;
+
+	ret = 0;
+	err = 0;
+	subflows = 0;
+	lock_sock(sk);
+	list_for_each_entry(subflow, &msk->conn_list, node) {
+		struct socket *tcp_sk = mptcp_subflow_tcp_socket(subflow);
+		int tmp;
+
+		tmp = kernel_setsockopt(tcp_sk, level, optname, optval, optlen);
+		if (tmp == 0)
+			subflows++;
+		else if (err == 0)
+			err = tmp;
+	}
+	release_sock(sk);
+
+	if (subflows == 0 && err)
+		ret = err;
+
+	return ret;
 }
 
 static int mptcp_getsockopt(struct sock *sk, int level, int optname,
@@ -755,10 +798,10 @@ static int mptcp_getsockopt(struct sock *sk, int level, int optname,
 		return ret;
 	}
 
-	/* @@ the meaning of setsockopt() when the socket is connected and
+	/* @@ the meaning of getsockopt() when the socket is connected and
 	 * there are multiple subflows is not defined.
 	 */
-	return 0;
+	return -EOPNOTSUPP;
 }
 
 static int mptcp_get_port(struct sock *sk, unsigned short snum)
-- 
2.21.0