[PATCH RFC] Support for SOL_IP for MPTCP setsockopt

[PATCH RFC] Support for SOL_IP for MPTCP setsockopt

[Poorva Sonparote]

Hello!

I have been working on adding support for IP_TOS for setsockopt(). I
am attaching the packetdrill test and the patch but it  still won't set the TOS value
for the subflows that are created after the setsockopt() call.

https://github.com/multipath-tcp/mptcp_net-next/issues/220#issuecomment-947863717

Thanks,
Poorva




This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)
https://github.com/multipath-tcp/mptcp_net-next/issues/220

Currently, any new subflows that are created after the setsockopt()
call are not being updated with the set IP_TOS value.

Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
---
 net/mptcp/sockopt.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 8137cc3a4296..f5ee49660902 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -598,6 +598,40 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
 	return ret;
 }
 
+static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
+					   sockptr_t optval, unsigned int optlen)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	int ret, err;
+	int val = 0;
+
+	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
+	val = inet_sk(sk)->tos;
+
+	if (err != 0)
+		return err;
+	lock_sock(sk);
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		ip_sock_set_tos(ssk, val);
+	}
+	release_sock(sk);
+	return ret;
+}
+
+static int mptcp_setsockopt_sol_ip(struct mptcp_sock *msk, int optname,
+				   sockptr_t optval, unsigned int optlen)
+{
+	switch (optname) {
+	case IP_TOS:
+		return  mptcp_setsockopt_sol_ip_set_tos(msk, optname, optval, optlen);
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
 				    sockptr_t optval, unsigned int optlen)
 {
@@ -637,12 +671,14 @@ int mptcp_setsockopt(struct sock *sk, int level, int optname,
 	if (ssk)
 		return tcp_setsockopt(ssk, level, optname, optval, optlen);
 
+	if (level == SOL_IP)
+		return mptcp_setsockopt_sol_ip(msk, optname, optval, optlen);
+
 	if (level == SOL_IPV6)
 		return mptcp_setsockopt_v6(msk, optname, optval, optlen);
 
 	if (level == SOL_TCP)
 		return mptcp_setsockopt_sol_tcp(msk, optname, optval, optlen);
-
 	return -EOPNOTSUPP;
 }
 
@@ -1000,6 +1036,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
 	ssk->sk_priority = sk->sk_priority;
 	ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
 	ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
+	ip_sock_set_tos(ssk, inet_sk(sk)->tos);
 
 	if (sk->sk_userlocks & tx_rx_locks) {
 		ssk->sk_userlocks |= sk->sk_userlocks & tx_rx_locks;
-- 
2.18.2

Re: [PATCH RFC] Support for SOL_IP for MPTCP setsockopt

[Mat Martineau]

On Wed, 20 Oct 2021, Poorva Sonparote wrote:

> Hello!
>
> I have been working on adding support for IP_TOS for setsockopt(). I
> am attaching the packetdrill test and the patch but it  still won't set the TOS value
> for the subflows that are created after the setsockopt() call.
>
> https://github.com/multipath-tcp/mptcp_net-next/issues/220#issuecomment-947863717
>
> Thanks,
> Poorva
>
>
>
>
> This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)
> https://github.com/multipath-tcp/mptcp_net-next/issues/220
>
> Currently, any new subflows that are created after the setsockopt()
> call are not being updated with the set IP_TOS value.
>

Hi Poorva,

Thanks for the patch!


A 'Closes:' tag with the github issue link is also helpful here.

> Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
> ---
> net/mptcp/sockopt.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>

If you have more commentary that isn't part of the commit message you want 
to be permanently stored in git, this space (after the '---' line above 
but before the 'diff --git ...' below) is a good for that! 'git am' will 
drop this text when applying. This is especially not a big deal with RFC 
patches like this one, since they don't end up getting merged anyway.


> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 8137cc3a4296..f5ee49660902 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -598,6 +598,40 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
> 	return ret;
> }
>
> +static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
> +					   sockptr_t optval, unsigned int optlen)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
> +	int ret, err;
> +	int val = 0;
> +
> +	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
> +	val = inet_sk(sk)->tos;
> +
> +	if (err != 0)
> +		return err;

Minor - need a blank line here.

> +	lock_sock(sk);
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		ip_sock_set_tos(ssk, val);
> +	}
> +	release_sock(sk);

and here

> +	return ret;
> +}
> +
> +static int mptcp_setsockopt_sol_ip(struct mptcp_sock *msk, int optname,
> +				   sockptr_t optval, unsigned int optlen)
> +{
> +	switch (optname) {
> +	case IP_TOS:
> +		return  mptcp_setsockopt_sol_ip_set_tos(msk, optname, optval, optlen);
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
> 				    sockptr_t optval, unsigned int optlen)
> {
> @@ -637,12 +671,14 @@ int mptcp_setsockopt(struct sock *sk, int level, int optname,
> 	if (ssk)
> 		return tcp_setsockopt(ssk, level, optname, optval, optlen);
>
> +	if (level == SOL_IP)
> +		return mptcp_setsockopt_sol_ip(msk, optname, optval, optlen);
> +
> 	if (level == SOL_IPV6)
> 		return mptcp_setsockopt_v6(msk, optname, optval, optlen);
>
> 	if (level == SOL_TCP)
> 		return mptcp_setsockopt_sol_tcp(msk, optname, optval, optlen);
> -

Looks like an accidental line deletion here.

> 	return -EOPNOTSUPP;
> }
>
> @@ -1000,6 +1036,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
> 	ssk->sk_priority = sk->sk_priority;
> 	ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
> 	ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
> +	ip_sock_set_tos(ssk, inet_sk(sk)->tos);

I think this does sync the tos value to new subflows, right? Since the ssk 
is already locked, use __ip_sock_set_tos(). Maybe you ran in to a deadlock 
here that interfered with the syncing.

>
> 	if (sk->sk_userlocks & tx_rx_locks) {
> 		ssk->sk_userlocks |= sk->sk_userlocks & tx_rx_locks;
> -- 
> 2.18.2
>
>
>

--
Mat Martineau
Intel

Re: [PATCH RFC] Support for SOL_IP for MPTCP setsockopt

[Poorva Sonparote]

Hi Mat,
>
> On Fri, Oct 22, 2021 at 4:50 PM Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Wed, 20 Oct 2021, Poorva Sonparote wrote:
>>
>> > Hello!
>> >
>> > I have been working on adding support for IP_TOS for setsockopt(). I
>> > am attaching the packetdrill test and the patch but it  still won't set the TOS value
>> > for the subflows that are created after the setsockopt() call.
>> >
>> > https://github.com/multipath-tcp/mptcp_net-next/issues/220#issuecomment-947863717
>> >
>> > Thanks,
>> > Poorva
>> >
>> >
>> >
>> >
>> > This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)
>> > https://github.com/multipath-tcp/mptcp_net-next/issues/220
>> >
>> > Currently, any new subflows that are created after the setsockopt()
>> > call are not being updated with the set IP_TOS value.
>> >
>>
>> Hi Poorva,
>>
>>> Thanks for the patch!
>>>
>>>
>>> A 'Closes:' tag with the github issue link is also helpful here.
>>
>>
>
Thank you for looking into this. I'll incorporate all the changes and
send out an updated version.
>
>>
>> > Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
>> > ---
>> > net/mptcp/sockopt.c | 39 ++++++++++++++++++++++++++++++++++++++-
>> > 1 file changed, 38 insertions(+), 1 deletion(-)
>> >
>>
>> If you have more commentary that isn't part of the commit message you want
>> to be permanently stored in git, this space (after the '---' line above
>> but before the 'diff --git ...' below) is a good for that! 'git am' will
>> drop this text when applying. This is especially not a big deal with RFC
>> patches like this one, since they don't end up getting merged anyway.
>>
>>
>> > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
>> > index 8137cc3a4296..f5ee49660902 100644
>> > --- a/net/mptcp/sockopt.c
>> > +++ b/net/mptcp/sockopt.c
>> > @@ -598,6 +598,40 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
>> >       return ret;
>> > }
>> >
>> > +static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
>> > +                                        sockptr_t optval, unsigned int optlen)
>> > +{
>> > +     struct mptcp_subflow_context *subflow;
>> > +     struct sock *sk = (struct sock *)msk;
>> > +     int ret, err;
>> > +     int val = 0;
>> > +
>> > +     err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
>> > +     val = inet_sk(sk)->tos;
>> > +
>> > +     if (err != 0)
>> > +             return err;
>>
>> Minor - need a blank line here.
>>
>> > +     lock_sock(sk);
>> > +     mptcp_for_each_subflow(msk, subflow) {
>> > +             struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>> > +
>> > +             ip_sock_set_tos(ssk, val);
>> > +     }
>> > +     release_sock(sk);
>>
>> and here
>>
>> > +     return ret;
>> > +}
>> > +
>> > +static int mptcp_setsockopt_sol_ip(struct mptcp_sock *msk, int optname,
>> > +                                sockptr_t optval, unsigned int optlen)
>> > +{
>> > +     switch (optname) {
>> > +     case IP_TOS:
>> > +             return  mptcp_setsockopt_sol_ip_set_tos(msk, optname, optval, optlen);
>> > +     }
>> > +
>> > +     return -EOPNOTSUPP;
>> > +}
>> > +
>> > static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>> >                                   sockptr_t optval, unsigned int optlen)
>> > {
>> > @@ -637,12 +671,14 @@ int mptcp_setsockopt(struct sock *sk, int level, int optname,
>> >       if (ssk)
>> >               return tcp_setsockopt(ssk, level, optname, optval, optlen);
>> >
>> > +     if (level == SOL_IP)
>> > +             return mptcp_setsockopt_sol_ip(msk, optname, optval, optlen);
>> > +
>> >       if (level == SOL_IPV6)
>> >               return mptcp_setsockopt_v6(msk, optname, optval, optlen);
>> >
>> >       if (level == SOL_TCP)
>> >               return mptcp_setsockopt_sol_tcp(msk, optname, optval, optlen);
>> > -
>>
>> Looks like an accidental line deletion here.
>>
>> >       return -EOPNOTSUPP;
>> > }
>> >
>> > @@ -1000,6 +1036,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>> >       ssk->sk_priority = sk->sk_priority;
>> >       ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
>> >       ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
>> > +     ip_sock_set_tos(ssk, inet_sk(sk)->tos);
>>
>>> > I think this does sync the tos value to new subflows, right? Since the ssk
>>> > is already locked, use __ip_sock_set_tos(). Maybe you ran in to a deadlock
>>> > here that interfered with the syncing.
>
>
Yes, I did. Thanks for the suggestion!
>>
>>
>> >
>> >       if (sk->sk_userlocks & tx_rx_locks) {
>> >               ssk->sk_userlocks |= sk->sk_userlocks & tx_rx_locks;
>> > --
>> > 2.18.2
>> >
>> >
>> >
>>
>> --
>> Mat Martineau
>> Intel
>>