| Message ID | 1116d80f808ea870f3f77fe927dbd6c622d062ae.1708515908.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: fix another deadlock issue | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 41 lines checked |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join |
Hi Paolo, Thank you for your modifications, that's great! Our CI (GitHub Action) did some validations and here is its report: - KVM Validation: normal: - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join
On Wed, 2024-02-21 at 12:46 +0100, Paolo Abeni wrote: > The mptcp diag interface already experienced a few locking bugs > that lockdep and appropriate coverage have detected in advance. > > Let's add a test-case triggering the relevant code path, to prevent > similar issues in the future. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh > index 60a7009ce1b5..3ab584b38566 100755 > --- a/tools/testing/selftests/net/mptcp/diag.sh > +++ b/tools/testing/selftests/net/mptcp/diag.sh > @@ -81,6 +81,21 @@ chk_msk_nr() > __chk_msk_nr "grep -c token:" "$@" > } > > +chk_listener_nr() > +{ > + local expected=$1 > + local msg="$2" > + > + if [ $expected -gt 0 ] && \ > + ! mptcp_lib_kallsyms_has "mptcp_diag_dump_listeners"; then > + printf "%-50s%s\n" "$msg - mptcp" "[ skip ]" > + mptcp_lib_result_skip "many listener sockets" I was too hasty and this is too fragile/requires debug. A v2 with something more reliable is needed. Given this is not going to land into this week PR, we will probably send it with a burst with other fixes. Thanks, Paolo
On Wed, Feb 22, 2024 at 12:46:59PM +0100, Paolo Abeni wrote: >l The mptcp diag interface already experienced a few locking bugs > that lockdep and appropriate coverage have detected in advance. > > Let's add a test-case triggering the relevant code path, to prevent > similar issues in the future. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh > index 60a7009ce1b5..3ab584b38566 100755 > --- a/tools/testing/selftests/net/mptcp/diag.sh > +++ b/tools/testing/selftests/net/mptcp/diag.sh > @@ -81,6 +81,21 @@ chk_msk_nr() > __chk_msk_nr "grep -c token:" "$@" > } > > +chk_listener_nr() > +{ > + local expected=$1 > + local msg="$2" > + > + if [ $expected -gt 0 ] && \ > + ! mptcp_lib_kallsyms_has "mptcp_diag_dump_listeners"; then > + printf "%-50s%s\n" "$msg - mptcp" "[ skip ]" > + mptcp_lib_result_skip "many listener sockets" > + else > + __chk_nr "ss -inmlHMON $ns | wc -l" "$expected" "$msg - mptcp" > + fi > + __chk_nr "ss -inmlHtON $ns | wc -l" "$expected" "$msg - subflows" > +} > + > wait_msk_nr() > { > local condition="grep -c token:" > @@ -279,5 +294,20 @@ flush_pids > chk_msk_inuse 0 "many->0" > chk_msk_cestab 0 "many->0" > > +chk_listener_nr 0 "no listener sockets" > +NR_SERVERS=100 > +for I in $(seq 1 $NR_SERVERS); do > + ip netns exec $ns ./mptcp_connect -p $((I + 20001)) -l 0.0.0.0 2>&1 >/dev/null & > + mptcp_lib_wait_local_port_listen $ns $((I + 20001)) > +done > + > +chk_listener_nr $NR_SERVERS "many listener sockets" > + > +# gracefull termination nit, as you plan to re-spin anyway: graceful > +for I in $(seq 1 $NR_SERVERS); do > + echo a | ip netns exec $ns ./mptcp_connect -p $((I + 20001)) 127.0.0.1 2>&1 >/dev/null I'm not sure of the validity of this but shellcheck complains that: SC2069 (warning): To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify). Also for the same constriction slightly earlier in this patch. > +done > +flush_pids > + > mptcp_lib_result_print_all_tap > exit $ret > -- > 2.43.0 > >
Hi Simon, Thank you for the review! On 22/02/2024 2:13 pm, Simon Horman wrote: > On Wed, Feb 22, 2024 at 12:46:59PM +0100, Paolo Abeni wrote: >> l The mptcp diag interface already experienced a few locking bugs >> that lockdep and appropriate coverage have detected in advance. >> >> Let's add a test-case triggering the relevant code path, to prevent >> similar issues in the future. >> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> --- >> tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh >> index 60a7009ce1b5..3ab584b38566 100755 >> --- a/tools/testing/selftests/net/mptcp/diag.sh >> +++ b/tools/testing/selftests/net/mptcp/diag.sh >> @@ -81,6 +81,21 @@ chk_msk_nr() >> __chk_msk_nr "grep -c token:" "$@" >> } >> >> +chk_listener_nr() >> +{ >> + local expected=$1 >> + local msg="$2" >> + >> + if [ $expected -gt 0 ] && \ >> + ! mptcp_lib_kallsyms_has "mptcp_diag_dump_listeners"; then >> + printf "%-50s%s\n" "$msg - mptcp" "[ skip ]" >> + mptcp_lib_result_skip "many listener sockets" >> + else >> + __chk_nr "ss -inmlHMON $ns | wc -l" "$expected" "$msg - mptcp" >> + fi >> + __chk_nr "ss -inmlHtON $ns | wc -l" "$expected" "$msg - subflows" >> +} >> + >> wait_msk_nr() >> { >> local condition="grep -c token:" >> @@ -279,5 +294,20 @@ flush_pids >> chk_msk_inuse 0 "many->0" >> chk_msk_cestab 0 "many->0" >> >> +chk_listener_nr 0 "no listener sockets" >> +NR_SERVERS=100 >> +for I in $(seq 1 $NR_SERVERS); do >> + ip netns exec $ns ./mptcp_connect -p $((I + 20001)) -l 0.0.0.0 2>&1 >/dev/null & >> + mptcp_lib_wait_local_port_listen $ns $((I + 20001)) >> +done >> + >> +chk_listener_nr $NR_SERVERS "many listener sockets" >> + >> +# gracefull termination > > nit, as you plan to re-spin anyway: graceful Paolo sent a new version on MPTCP ML only. When applying the new version, I fixed this: https://lore.kernel.org/mptcp/e98b8b07-558c-498a-97fb-dd5cab11e439@kernel.org/ >> +for I in $(seq 1 $NR_SERVERS); do >> + echo a | ip netns exec $ns ./mptcp_connect -p $((I + 20001)) 127.0.0.1 2>&1 >/dev/null > > I'm not sure of the validity of this but shellcheck complains that: > > SC2069 (warning): To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify). > > Also for the same constriction slightly earlier in this patch. Good catch! I should add shellcheck support in our CI to catch that :) I just applied the suggested modification in our tree [1]. The new version that will be sent to netdev will no longer have this issue. https://github.com/multipath-tcp/mptcp_net-next/commit/31f1460d552c Cheers, Matt
On Thu, Feb 22, 2024 at 03:49:44PM +0100, Matthieu Baerts wrote: > Hi Simon, > > Thank you for the review! > > On 22/02/2024 2:13 pm, Simon Horman wrote: > > On Wed, Feb 22, 2024 at 12:46:59PM +0100, Paolo Abeni wrote: > >> l The mptcp diag interface already experienced a few locking bugs > >> that lockdep and appropriate coverage have detected in advance. > >> > >> Let's add a test-case triggering the relevant code path, to prevent > >> similar issues in the future. > >> > >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> > >> --- > >> tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++ > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh > >> index 60a7009ce1b5..3ab584b38566 100755 > >> --- a/tools/testing/selftests/net/mptcp/diag.sh > >> +++ b/tools/testing/selftests/net/mptcp/diag.sh > >> @@ -81,6 +81,21 @@ chk_msk_nr() > >> __chk_msk_nr "grep -c token:" "$@" > >> } > >> > >> +chk_listener_nr() > >> +{ > >> + local expected=$1 > >> + local msg="$2" > >> + > >> + if [ $expected -gt 0 ] && \ > >> + ! mptcp_lib_kallsyms_has "mptcp_diag_dump_listeners"; then > >> + printf "%-50s%s\n" "$msg - mptcp" "[ skip ]" > >> + mptcp_lib_result_skip "many listener sockets" > >> + else > >> + __chk_nr "ss -inmlHMON $ns | wc -l" "$expected" "$msg - mptcp" > >> + fi > >> + __chk_nr "ss -inmlHtON $ns | wc -l" "$expected" "$msg - subflows" > >> +} > >> + > >> wait_msk_nr() > >> { > >> local condition="grep -c token:" > >> @@ -279,5 +294,20 @@ flush_pids > >> chk_msk_inuse 0 "many->0" > >> chk_msk_cestab 0 "many->0" > >> > >> +chk_listener_nr 0 "no listener sockets" > >> +NR_SERVERS=100 > >> +for I in $(seq 1 $NR_SERVERS); do > >> + ip netns exec $ns ./mptcp_connect -p $((I + 20001)) -l 0.0.0.0 2>&1 >/dev/null & > >> + mptcp_lib_wait_local_port_listen $ns $((I + 20001)) > >> +done > >> + > >> +chk_listener_nr $NR_SERVERS "many listener sockets" > >> + > >> +# gracefull termination > > > > nit, as you plan to re-spin anyway: graceful > > Paolo sent a new version on MPTCP ML only. When applying the new > version, I fixed this: > > https://lore.kernel.org/mptcp/e98b8b07-558c-498a-97fb-dd5cab11e439@kernel.org/ > > >> +for I in $(seq 1 $NR_SERVERS); do > >> + echo a | ip netns exec $ns ./mptcp_connect -p $((I + 20001)) 127.0.0.1 2>&1 >/dev/null > > > > I'm not sure of the validity of this but shellcheck complains that: > > > > SC2069 (warning): To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify). > > > > Also for the same constriction slightly earlier in this patch. > > Good catch! > > I should add shellcheck support in our CI to catch that :) > > I just applied the suggested modification in our tree [1]. The new > version that will be sent to netdev will no longer have this issue. > > https://github.com/multipath-tcp/mptcp_net-next/commit/31f1460d552c Thanks Matt, much appreciated.
diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh index 60a7009ce1b5..3ab584b38566 100755 --- a/tools/testing/selftests/net/mptcp/diag.sh +++ b/tools/testing/selftests/net/mptcp/diag.sh @@ -81,6 +81,21 @@ chk_msk_nr() __chk_msk_nr "grep -c token:" "$@" } +chk_listener_nr() +{ + local expected=$1 + local msg="$2" + + if [ $expected -gt 0 ] && \ + ! mptcp_lib_kallsyms_has "mptcp_diag_dump_listeners"; then + printf "%-50s%s\n" "$msg - mptcp" "[ skip ]" + mptcp_lib_result_skip "many listener sockets" + else + __chk_nr "ss -inmlHMON $ns | wc -l" "$expected" "$msg - mptcp" + fi + __chk_nr "ss -inmlHtON $ns | wc -l" "$expected" "$msg - subflows" +} + wait_msk_nr() { local condition="grep -c token:" @@ -279,5 +294,20 @@ flush_pids chk_msk_inuse 0 "many->0" chk_msk_cestab 0 "many->0" +chk_listener_nr 0 "no listener sockets" +NR_SERVERS=100 +for I in $(seq 1 $NR_SERVERS); do + ip netns exec $ns ./mptcp_connect -p $((I + 20001)) -l 0.0.0.0 2>&1 >/dev/null & + mptcp_lib_wait_local_port_listen $ns $((I + 20001)) +done + +chk_listener_nr $NR_SERVERS "many listener sockets" + +# gracefull termination +for I in $(seq 1 $NR_SERVERS); do + echo a | ip netns exec $ns ./mptcp_connect -p $((I + 20001)) 127.0.0.1 2>&1 >/dev/null +done +flush_pids + mptcp_lib_result_print_all_tap exit $ret
The mptcp diag interface already experienced a few locking bugs that lockdep and appropriate coverage have detected in advance. Let's add a test-case triggering the relevant code path, to prevent similar issues in the future. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- tools/testing/selftests/net/mptcp/diag.sh | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+)