[PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[Poorva Sonparote]

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

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/220

Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
---

Notes:
    - This patch addresses previous suggestions.
    - The TOS value is now being set successfully for subflows
      created after setsockopt().

 include/net/ip.h       |  3 +--
 net/ipv4/ip_sockglue.c |  2 +-
 net/mptcp/sockopt.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index cf229a531194..a40501fe89de 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -762,7 +762,6 @@ void ip_icmp_error(struct sock *sk, struct sk_buff *skb, int err, __be16 port,
 		   u32 info, u8 *payload);
 void ip_local_error(struct sock *sk, int err, __be32 daddr, __be16 dport,
 		    u32 info);
-
 static inline void ip_cmsg_recv(struct msghdr *msg, struct sk_buff *skb)
 {
 	ip_cmsg_recv_offset(msg, skb->sk, skb, 0, 0);
@@ -789,5 +788,5 @@ int ip_sock_set_mtu_discover(struct sock *sk, int val);
 void ip_sock_set_pktinfo(struct sock *sk);
 void ip_sock_set_recverr(struct sock *sk);
 void ip_sock_set_tos(struct sock *sk, int val);
-
+void  __ip_sock_set_tos(struct sock *sk, int val);
 #endif	/* _IP_H */
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index b297bb28556e..7fd83f14daae 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -576,7 +576,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
 	return err;
 }
 
-static void __ip_sock_set_tos(struct sock *sk, int val)
+void __ip_sock_set_tos(struct sock *sk, int val)
 {
 	if (sk->sk_type == SOCK_STREAM) {
 		val &= ~INET_ECN_MASK;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 8137cc3a4296..3967e1b8455c 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -598,6 +598,43 @@ 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, val = 0;
+	int err;
+
+	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
+	val = inet_sk(sk)->tos;
+
+	if (err != 0)
+		return err;
+
+	lock_sock(sk);
+	sockopt_seq_inc(msk);
+	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,6 +674,9 @@ 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);
 
@@ -1000,6 +1040,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 mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[Mat Martineau]

On Mon, 25 Oct 2021, Poorva Sonparote wrote:

> This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)
> https://github.com/multipath-tcp/mptcp_net-next/issues/220
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/220
>
> Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
> ---
>
> Notes:
>    - This patch addresses previous suggestions.
>    - The TOS value is now being set successfully for subflows
>      created after setsockopt().
>

Hi Poorva -

Thanks for the update. A few notes below.

> include/net/ip.h       |  3 +--
> net/ipv4/ip_sockglue.c |  2 +-
> net/mptcp/sockopt.c    | 41 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/ip.h b/include/net/ip.h
> index cf229a531194..a40501fe89de 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -762,7 +762,6 @@ void ip_icmp_error(struct sock *sk, struct sk_buff *skb, int err, __be16 port,
> 		   u32 info, u8 *payload);
> void ip_local_error(struct sock *sk, int err, __be32 daddr, __be16 dport,
> 		    u32 info);
> -

Looks like an accidentally deleted line here.

When making changes to the networking core, it's best to split that out in 
a separate commit (with a subject like "net: Expose __ip_sock_set_tos()"). 
MPTCP maintainers can accept changes to MPTCP files, but other maintainers 
are responsible for ip.h and ip_sockglue.c

> static inline void ip_cmsg_recv(struct msghdr *msg, struct sk_buff *skb)
> {
> 	ip_cmsg_recv_offset(msg, skb->sk, skb, 0, 0);
> @@ -789,5 +788,5 @@ int ip_sock_set_mtu_discover(struct sock *sk, int val);
> void ip_sock_set_pktinfo(struct sock *sk);
> void ip_sock_set_recverr(struct sock *sk);
> void ip_sock_set_tos(struct sock *sk, int val);
> -
> +void  __ip_sock_set_tos(struct sock *sk, int val);

Keep the empty line before the #endif

> #endif	/* _IP_H */
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index b297bb28556e..7fd83f14daae 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -576,7 +576,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
> 	return err;
> }
>
> -static void __ip_sock_set_tos(struct sock *sk, int val)
> +void __ip_sock_set_tos(struct sock *sk, int val)
> {
> 	if (sk->sk_type == SOCK_STREAM) {
> 		val &= ~INET_ECN_MASK;
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 8137cc3a4296..3967e1b8455c 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -598,6 +598,43 @@ 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, val = 0;

No need to initialize 'val' here, there's no code path where it will get 
used uninitialized.

> +	int err;
> +
> +	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
> +	val = inet_sk(sk)->tos;

This assignment should be done after the lock_sock() but before the loop.


The patch is close to being ready, thanks again.

Mat


> +
> +	if (err != 0)
> +		return err;
> +
> +	lock_sock(sk);
> +	sockopt_seq_inc(msk);
> +	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,6 +674,9 @@ 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);
>
> @@ -1000,6 +1040,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
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[Matthieu Baerts]

Hi Poorva,

In addition to Mat's review, here is a few notes from my side.

On 25/10/2021 23:01, Poorva Sonparote wrote:
> This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)

Small detail: in the subject, you mention 'SOL_IP'. Maybe clearer to set
'IP_TOS' like here because you don't add support for all SOL_IP options, no?

> https://github.com/multipath-tcp/mptcp_net-next/issues/220
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/220
> 
> Signed-off-by: Poorva Sonparote <psonparo@redhat.com>

Small detail for your next version: there should be no blank lines
between tags. In other words, we should have this without blank lines in
betweeen:

  Closes: (...)
  Signed-off-by: (...)


While at it, it is always nice to add some useful info in the commit
message:

- the reason: not always obvious when looking at the diff. Here it is
maybe clear but still good to give a short recap on what SOL_IP is.

- what you are doing: always useful for the reviewer to check if you did
what you said you wanted to do, e.g. here you accept a first SOL_IP
option, you set the same value to each subflow, you export a function
from ipv4 stack (for a dedicated commit as Mat said), etc..

- particularities: e.g. it seems you don't check if the subflows are in
v4 when you go through the list of subflows to set something v4 specific
(it is v4 specific, no?). Always good to mention that in the commit
message if it is OK to do that (and a comment in the code).

- Any other info that can be useful for other devs and reviewers, e.g.
how you tested it, if you had an interesting issue during the dev,
possible alternatives, etc.

Do not hesitate to look at other commit messages for inspiration ;-)

> ---
> 
> Notes:
>     - This patch addresses previous suggestions.
>     - The TOS value is now being set successfully for subflows
>       created after setsockopt().

Is it enough to cover this only in packetdrill and not in selftests? I
think it is but just raising the point just in case ;-)

(...)

> +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);

detail: please avoid double spaces after "return"

> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  				    sockptr_t optval, unsigned int optlen)
>  {
> @@ -637,6 +674,9 @@ 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);

Maybe clearer to use mptcp_setsockopt_v4() for the name to imitate the
_v6 version below:

> +
>  	if (level == SOL_IPV6)
>  		return mptcp_setsockopt_v6(msk, optname, optval, optlen);
>  

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

Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[kernel test robot]

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

Hi Poorva,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Poorva-Sonparote/Support-for-SOL_IP-for-MPTCP-setsockopt/20211026-050443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3fb59a5de5cbb04de76915d9f5bff01d16aa1fc4
config: i386-buildonly-randconfig-r005-20211027 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ed825e662db596df8097ed4c2963fb72f7f9d214
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Poorva-Sonparote/Support-for-SOL_IP-for-MPTCP-setsockopt/20211026-050443
        git checkout ed825e662db596df8097ed4c2963fb72f7f9d214
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/mptcp/sockopt.c:624:9: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
           return ret;
                  ^~~
   net/mptcp/sockopt.c:606:9: note: initialize the variable 'ret' to silence this warning
           int ret, val = 0;
                  ^
                   = 0
   1 error generated.


vim +/ret +624 net/mptcp/sockopt.c

   600	
   601	static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
   602						   sockptr_t optval, unsigned int optlen)
   603	{
   604		struct mptcp_subflow_context *subflow;
   605		struct sock *sk = (struct sock *)msk;
   606		int ret, val = 0;
   607		int err;
   608	
   609		err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
   610		val = inet_sk(sk)->tos;
   611	
   612		if (err != 0)
   613			return err;
   614	
   615		lock_sock(sk);
   616		sockopt_seq_inc(msk);
   617		mptcp_for_each_subflow(msk, subflow) {
   618			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
   619	
   620			__ip_sock_set_tos(ssk, val);
   621		}
   622		release_sock(sk);
   623	
 > 624		return ret;
   625	}
   626	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40132 bytes --]

Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt

[kernel test robot]

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

Hi Poorva,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Poorva-Sonparote/Support-for-SOL_IP-for-MPTCP-setsockopt/20211026-050443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3fb59a5de5cbb04de76915d9f5bff01d16aa1fc4
config: i386-buildonly-randconfig-r005-20211027 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ed825e662db596df8097ed4c2963fb72f7f9d214
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Poorva-Sonparote/Support-for-SOL_IP-for-MPTCP-setsockopt/20211026-050443
        git checkout ed825e662db596df8097ed4c2963fb72f7f9d214
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/mptcp/sockopt.c:624:9: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
           return ret;
                  ^~~
   net/mptcp/sockopt.c:606:9: note: initialize the variable 'ret' to silence this warning
           int ret, val = 0;
                  ^
                   = 0
   1 error generated.


vim +/ret +624 net/mptcp/sockopt.c

   600	
   601	static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
   602						   sockptr_t optval, unsigned int optlen)
   603	{
   604		struct mptcp_subflow_context *subflow;
   605		struct sock *sk = (struct sock *)msk;
   606		int ret, val = 0;
   607		int err;
   608	
   609		err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
   610		val = inet_sk(sk)->tos;
   611	
   612		if (err != 0)
   613			return err;
   614	
   615		lock_sock(sk);
   616		sockopt_seq_inc(msk);
   617		mptcp_for_each_subflow(msk, subflow) {
   618			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
   619	
   620			__ip_sock_set_tos(ssk, val);
   621		}
   622		release_sock(sk);
   623	
 > 624		return ret;
   625	}
   626	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40132 bytes --]