| Message ID | f2cf8a00fdf178f314b92c8809ca9deed98464fa.1699466615.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | c69c00a783ed281382d869122b5105e38e902387 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net] mptcp: fix setsockopt(IP_TOS) subflow locking | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | success | Success! ✅ |
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6206547924090880
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6206547924090880/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5925072947380224
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5925072947380224/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4799173040537600
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4799173040537600/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5362122993958912
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5362122993958912/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/76fad834de5b
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
On Wed, 8 Nov 2023, Paolo Abeni wrote: > The mptcp implementation of the IP_TOS socket option uses the lockless > variant of the TOS manipulation helper and does not hold suck lock at (matttbe: please fix "such" typo when applying) > the helper invocation time. > > Add the required locking. > > Fixes: ffcacff87cd6 ("mptcp: Support for IP_TOS for MPTCP setsockopt()") > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/457 > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/sockopt.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index 8d485c40585a..cabe856b2a45 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -738,8 +738,11 @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, > val = READ_ONCE(inet_sk(sk)->tos); > mptcp_for_each_subflow(msk, subflow) { > struct sock *ssk = mptcp_subflow_tcp_sock(subflow); > + bool slow; > > + slow = lock_sock_fast(ssk); > __ip_sock_set_tos(ssk, val); > + unlock_sock_fast(ssk, slow); > } > release_sock(sk); > Looks good to me with commit message typo fix: Reviewed-by: Mat Martineau <martineau@kernel.org>
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6216342865444864
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6216342865444864/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4808967981891584
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4808967981891584/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5371917935312896
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5371917935312896/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5934867888734208
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5934867888734208/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f0a09d7ba823
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
Hi Paolo, Mat, On 08/11/2023 19:26, Paolo Abeni wrote: > The mptcp implementation of the IP_TOS socket option uses the lockless > variant of the TOS manipulation helper and does not hold suck lock at > the helper invocation time. > > Add the required locking. Thank you for the patch and the review! Now in our tree (fixes for -net), with 's/suck/such/', because typos suck...: New patches for t/upstream-net and t/upstream: - c69c00a783ed: mptcp: fix setsockopt(IP_TOS) subflow locking - Results: 688025cbce11..52a2364fffc9 (export-net) - Results: 4d89b70d7306..351099187dd4 (export) Tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20231109T161702 https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231109T161702 Cheers, Matt
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 8d485c40585a..cabe856b2a45 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -738,8 +738,11 @@ static int mptcp_setsockopt_v4_set_tos(struct mptcp_sock *msk, int optname, val = READ_ONCE(inet_sk(sk)->tos); mptcp_for_each_subflow(msk, subflow) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); + bool slow; + slow = lock_sock_fast(ssk); __ip_sock_set_tos(ssk, val); + unlock_sock_fast(ssk, slow); } release_sock(sk);
The mptcp implementation of the IP_TOS socket option uses the lockless variant of the TOS manipulation helper and does not hold suck lock at the helper invocation time. Add the required locking. Fixes: ffcacff87cd6 ("mptcp: Support for IP_TOS for MPTCP setsockopt()") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/457 Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/sockopt.c | 3 +++ 1 file changed, 3 insertions(+)