include/net/ip.h | 3 +-- net/ipv4/ip_sockglue.c | 2 +- net/mptcp/sockopt.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-)
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
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
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
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
© 2016 - 2024 Red Hat, Inc.