Add set MPTFO socket option for MPTCP.

Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
---
 net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
 net/mptcp/protocol.h |  2 ++
 net/mptcp/sockopt.c  |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 50b5c3376672..436e773d798a 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 	ret = -EFAULT;
 	return ret;
 }
+
+int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
+				      unsigned int optlen)
+{
+	struct sock *sk = (struct sock *)msk;
+	struct net *net = sock_net(sk);
+	int val;
+	int ret;
+
+	ret = 0;
+
+	if (copy_from_sockptr(&val, optval, sizeof(val)))
+		return -EFAULT;
+
+	lock_sock(sk);
+
+	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
+	    TCPF_LISTEN))) {
+		tcp_fastopen_init_key_once(net);
+		fastopen_queue_tune(sk, val);
+	} else {
+		ret = -EINVAL;
+	}
+
+	release_sock(sk);
+
+	return ret;
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 5cd14eacd1d6..8caaeeedb9da 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -843,6 +843,8 @@ void mptcp_rfree(struct sk_buff *skb);
 int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 			   size_t len, struct mptcp_sock *msk,
 			   size_t *copied);
+int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
+				      unsigned int optlen);
 // Fast Open Mechanism functions end
 
 static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 423d3826ca1e..f62f0d63b8e6 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -559,6 +559,7 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case TCP_NOTSENT_LOWAT:
 		case TCP_TX_DELAY:
 		case TCP_INQ:
+		case TCP_FASTOPEN:
 			return true;
 		}
 
@@ -796,6 +797,8 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 		return mptcp_setsockopt_sol_tcp_nodelay(msk, optval, optlen);
 	case TCP_DEFER_ACCEPT:
 		return mptcp_setsockopt_sol_tcp_defer(msk, optval, optlen);
+	case TCP_FASTOPEN:
+		return mptcp_setsockopt_sol_tcp_fastopen(msk, optval, optlen);
 	}
 
 	return -EOPNOTSUPP;
-- 
2.25.1

On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> Add set MPTFO socket option for MPTCP.
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/sockopt.c  |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 50b5c3376672..436e773d798a 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  	ret = -EFAULT;
>  	return ret;
>  }
> +
> +int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
> +				      unsigned int optlen)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +	struct net *net = sock_net(sk);
> +	int val;
> +	int ret;
> +
> +	ret = 0;
> +
> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
> +		return -EFAULT;
> +
> +	lock_sock(sk);
> +
> +	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
> +	    TCPF_LISTEN))) {

The above can fit a single line:

	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {

Otherwise this patch LGTM.


Cheers,

Paolo

Fixed in v8

Best,

Dmytro

On 9/19/2022 12:48 PM, Paolo Abeni wrote:
> On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
>> Add set MPTFO socket option for MPTCP.
>>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>   net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
>>   net/mptcp/protocol.h |  2 ++
>>   net/mptcp/sockopt.c  |  3 +++
>>   3 files changed, 33 insertions(+)
>>
>> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
>> index 50b5c3376672..436e773d798a 100644
>> --- a/net/mptcp/fastopen.c
>> +++ b/net/mptcp/fastopen.c
>> @@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>>   	ret = -EFAULT;
>>   	return ret;
>>   }
>> +
>> +int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>> +				      unsigned int optlen)
>> +{
>> +	struct sock *sk = (struct sock *)msk;
>> +	struct net *net = sock_net(sk);
>> +	int val;
>> +	int ret;
>> +
>> +	ret = 0;
>> +
>> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
>> +		return -EFAULT;
>> +
>> +	lock_sock(sk);
>> +
>> +	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
>> +	    TCPF_LISTEN))) {
> The above can fit a single line:
>
> 	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {
>
> Otherwise this patch LGTM.
>
>
> Cheers,
>
> Paolo
>

Hi Dmytro,

On 18/09/2022 00:28, Dmytro Shytyi wrote:
> Add set MPTFO socket option for MPTCP.

It is often very helpful to not just mention what you are doing here
(e.g. add XXX) but explain the reason why you are doing this. Getting
the reason is often not clear when looking at a commit diff so it is a
very valuable info to add in a commit description.

For example here, it could be nice to explain this option is needed for
the listen socket only to accept TFO. And also why you are changing the
values directly, what it is similar to, etc. etc.

> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/sockopt.c  |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 50b5c3376672..436e773d798a 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  	ret = -EFAULT;
>  	return ret;
>  }
> +
> +int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
> +				      unsigned int optlen)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +	struct net *net = sock_net(sk);
> +	int val;
> +	int ret;
> +
> +	ret = 0;
> +
> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
> +		return -EFAULT;
> +
> +	lock_sock(sk);
> +
> +	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
> +	    TCPF_LISTEN))) {
> +		tcp_fastopen_init_key_once(net);
> +		fastopen_queue_tune(sk, val);

Can you not call tcp_setsockopt() here instead of copying what is done
in do_tcp_setsockopt()?

It would be simpler and easier to maintain I think.


Also, in general for the series, may you add 'mptcp:' prefix in the git
commit titles?

What is important too is to add comments if what you are doing is
similar to what is done in TCP, e.g. similar to XXX() in tcp_XXX.c? You
can add a short comment in the code and explain why you are doing that
and not call TCP functions in the git commit description for example.
That would help a lot reviewers and maintainers later.

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

Hi Matthieu

Thank you for comments,

I will improve this in v9.

Best,

Dmytro

On 9/20/2022 11:37 AM, Matthieu Baerts wrote:
> Hi Dmytro,
>
> On 18/09/2022 00:28, Dmytro Shytyi wrote:
>> Add set MPTFO socket option for MPTCP.
> It is often very helpful to not just mention what you are doing here
> (e.g. add XXX) but explain the reason why you are doing this. Getting
> the reason is often not clear when looking at a commit diff so it is a
> very valuable info to add in a commit description.
>
> For example here, it could be nice to explain this option is needed for
> the listen socket only to accept TFO. And also why you are changing the
> values directly, what it is similar to, etc. etc.
>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>   net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
>>   net/mptcp/protocol.h |  2 ++
>>   net/mptcp/sockopt.c  |  3 +++
>>   3 files changed, 33 insertions(+)
>>
>> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
>> index 50b5c3376672..436e773d798a 100644
>> --- a/net/mptcp/fastopen.c
>> +++ b/net/mptcp/fastopen.c
>> @@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>>   	ret = -EFAULT;
>>   	return ret;
>>   }
>> +
>> +int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>> +				      unsigned int optlen)
>> +{
>> +	struct sock *sk = (struct sock *)msk;
>> +	struct net *net = sock_net(sk);
>> +	int val;
>> +	int ret;
>> +
>> +	ret = 0;
>> +
>> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
>> +		return -EFAULT;
>> +
>> +	lock_sock(sk);
>> +
>> +	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
>> +	    TCPF_LISTEN))) {
>> +		tcp_fastopen_init_key_once(net);
>> +		fastopen_queue_tune(sk, val);
> Can you not call tcp_setsockopt() here instead of copying what is done
> in do_tcp_setsockopt()?
>
> It would be simpler and easier to maintain I think.
>
>
> Also, in general for the series, may you add 'mptcp:' prefix in the git
> commit titles?
>
> What is important too is to add comments if what you are doing is
> similar to what is done in TCP, e.g. similar to XXX() in tcp_XXX.c? You
> can add a short comment in the code and explain why you are doing that
> and not call TCP functions in the git commit description for example.
> That would help a lot reviewers and maintainers later.
>
> Cheers,
> Matt