* [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt
@ 2021-10-25 21:01 Poorva Sonparote
2021-10-25 21:55 ` Mat Martineau
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Poorva Sonparote @ 2021-10-25 21:01 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt
2021-10-25 21:01 [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt Poorva Sonparote
@ 2021-10-25 21:55 ` Mat Martineau
2021-10-26 10:33 ` Matthieu Baerts
2021-10-27 10:06 ` kernel test robot
2 siblings, 0 replies; 5+ messages in thread
From: Mat Martineau @ 2021-10-25 21:55 UTC (permalink / raw)
To: Poorva Sonparote; +Cc: mptcp
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt
2021-10-25 21:01 [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt Poorva Sonparote
2021-10-25 21:55 ` Mat Martineau
@ 2021-10-26 10:33 ` Matthieu Baerts
2021-10-27 10:06 ` kernel test robot
2 siblings, 0 replies; 5+ messages in thread
From: Matthieu Baerts @ 2021-10-26 10:33 UTC (permalink / raw)
To: Poorva Sonparote; +Cc: mathew.j.martineau, mptcp
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt
2021-10-25 21:01 [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt Poorva Sonparote
@ 2021-10-27 10:06 ` kernel test robot
2021-10-26 10:33 ` Matthieu Baerts
2021-10-27 10:06 ` kernel test robot
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-10-27 10:06 UTC (permalink / raw)
To: Poorva Sonparote, mptcp; +Cc: llvm, kbuild-all, mathew.j.martineau
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt
@ 2021-10-27 10:06 ` kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-10-27 10:06 UTC (permalink / raw)
To: kbuild-all
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-27 10:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 21:01 [PATCH mptcp net-next] Support for SOL_IP for MPTCP setsockopt Poorva Sonparote
2021-10-25 21:55 ` Mat Martineau
2021-10-26 10:33 ` Matthieu Baerts
2021-10-27 10:06 ` kernel test robot
2021-10-27 10:06 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.