| Message ID | be9176dac04b292242d6afc2eefc0c931d21b0cd.1709651670.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net,v2] mptcp: don't account accept() of non-MPC client as fallback to TCP | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): selftest_mptcp_join |
Hi Davide,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8158923073
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e5de41746751
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-normal
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 (NGI0 Core)
Hi Davide, Thank you for your modifications, that's great! Our CI (Cirrus) did some validations with a debug kernel and here is its report: - KVM Validation: debug (except selftest_mptcp_join): - Success! ✅: - Task: https://cirrus-ci.com/task/6707619503538176 - Summary: https://api.cirrus-ci.com/v1/artifact/task/6707619503538176/summary/summary.txt - KVM Validation: debug (only selftest_mptcp_join): - Unstable: 1 failed test(s): selftest_mptcp_join
Hi Davide, On 05/03/2024 16:19, Davide Caratti wrote: > currently, inbound TCP connections increment MPTcpExtMPCapableFallbackACK Maybe clearer with 'inbound "plain" TCP connections'? > when the server accepts them. As Christoph reported, this is "surprising" > because the counter becomes greater than MPTcpExtMPCapableSYNRX when many > non-MPC TCP connections are accepted. (detail: maybe add an empty line here, and start the sentence above with a capital letter :) ) > Change the semantic of MPTcpExtMPCapableFallbackACK to increment when the > subflow context of an inbound MPC connection attempt is dropped. Detail: I don't know if 'change the semantic' would be OK for a patch for -net. Maybe more: MPTcpExtMPCapableFallbackACK counter's name suggests it should only be incremented when a connection was seen using MPTCP options, then a fallback to TCP has been done. Let's do that by incrementing it when the subflow context of an inbound MPC connection attempt is dropped. > v2: fix reporter name Maybe best to put that in the "comment" section, after a line with 3 dashes ('---', or by using Git notes), not to include that in the patch we will send to netdev. > Reported-by: Christoph Paasch <cpaasch@apple.com> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/449 > Signed-off-by: Davide Caratti <dcaratti@redhat.com> Can you add a Fixes tag please? > --- > net/mptcp/protocol.c | 2 -- > net/mptcp/subflow.c | 2 ++ > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index cdf9ec67795e..556b3b95c537 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -3937,8 +3937,6 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock, > mptcp_set_state(newsk, TCP_CLOSE); > } > } else { > - MPTCP_INC_STATS(sock_net(ssk), > - MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); > tcpfallback: > newsk->sk_kern_sock = kern; > lock_sock(newsk); > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 1626dd20c68f..6e3fe38f057d 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > return child; > > fallback: > + if (child) Why do you need to check that? It is not supposed to be NULL if you are here, no? > + SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); > mptcp_subflow_drop_ctx(child); > return child; > } Cheers, Matt
On 07/03/2024 11:05, Matthieu Baerts wrote:
> Hi Davide,
I forgot to add: thank you for having worked on that :)
Cheers,
Matt
Hi Matthieu! On Thu, Mar 07, 2024 at 11:05:10AM +0100, Matthieu Baerts wrote: > Hi Davide, > > On 05/03/2024 16:19, Davide Caratti wrote: > > currently, inbound TCP connections increment MPTcpExtMPCapableFallbackACK > > Maybe clearer with 'inbound "plain" TCP connections'? maybe it's better to say "non-MPC": see below > > > when the server accepts them. As Christoph reported, this is "surprising" > > because the counter becomes greater than MPTcpExtMPCapableSYNRX when many > > non-MPC TCP connections are accepted. > > (detail: maybe add an empty line here, and start the sentence above with > a capital letter :) ) right, I always forget the capital letter. > > > Change the semantic of MPTcpExtMPCapableFallbackACK to increment when the > > subflow context of an inbound MPC connection attempt is dropped. > > Detail: I don't know if 'change the semantic' would be OK for a patch > for -net. Maybe more: > > MPTcpExtMPCapableFallbackACK counter's name suggests it should only be > incremented when a connection was seen using MPTCP options, then a > fallback to TCP has been done. Let's do that by incrementing it when > the subflow context of an inbound MPC connection attempt is dropped. > > > v2: fix reporter name this was meant for net-next, but I can find a suitable Fixes tag. [...] > > index 1626dd20c68f..6e3fe38f057d 100644 > > --- a/net/mptcp/subflow.c > > +++ b/net/mptcp/subflow.c > > @@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > > return child; > > > > fallback: > > + if (child) > > Why do you need to check that? It is not supposed to be NULL if you are > here, no? Ouch, this is wrong and unintentional. The correct test is if (fallback) SUBFLOW_REQ_INC_STATS(blah); and I mixed variable names while cleaning up pr_debug()s. Non-MPC connection attempts (pure TCP, but also MPTCP with pathological MP_CAPABLE sub-options) have fallback equal to false here. Will fix this in v3.
Hi Davide, On 07/03/2024 16:38, Davide Caratti wrote: > Hi Matthieu! > > On Thu, Mar 07, 2024 at 11:05:10AM +0100, Matthieu Baerts wrote: >> Hi Davide, >> >> On 05/03/2024 16:19, Davide Caratti wrote: >>> currently, inbound TCP connections increment MPTcpExtMPCapableFallbackACK >> >> Maybe clearer with 'inbound "plain" TCP connections'? > > maybe it's better to say "non-MPC": see below Yes, good point. >>> when the server accepts them. As Christoph reported, this is "surprising" >>> because the counter becomes greater than MPTcpExtMPCapableSYNRX when many >>> non-MPC TCP connections are accepted. >> >> (detail: maybe add an empty line here, and start the sentence above with >> a capital letter :) ) > > right, I always forget the capital letter. > >> >>> Change the semantic of MPTcpExtMPCapableFallbackACK to increment when the >>> subflow context of an inbound MPC connection attempt is dropped. >> >> Detail: I don't know if 'change the semantic' would be OK for a patch >> for -net. Maybe more: >> >> MPTcpExtMPCapableFallbackACK counter's name suggests it should only be >> incremented when a connection was seen using MPTCP options, then a >> fallback to TCP has been done. Let's do that by incrementing it when >> the subflow context of an inbound MPC connection attempt is dropped. >> >>> v2: fix reporter name > > this was meant for net-next, but I can find a suitable Fixes tag. I think it is good to consider this as a fix for -net. > > [...] > >>> index 1626dd20c68f..6e3fe38f057d 100644 >>> --- a/net/mptcp/subflow.c >>> +++ b/net/mptcp/subflow.c >>> @@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, >>> return child; >>> >>> fallback: >>> + if (child) >> >> Why do you need to check that? It is not supposed to be NULL if you are >> here, no? > > Ouch, this is wrong and unintentional. The correct test is > if (fallback) > SUBFLOW_REQ_INC_STATS(blah); > > and I mixed variable names while cleaning up pr_debug()s. Non-MPC connection > attempts (pure TCP, but also MPTCP with pathological MP_CAPABLE > sub-options) have fallback equal to false here. Will fix this in v3. Maybe it also means it would be good to add a check in the selftests (in mptcp_connect.sh?) to make sure it is doing the right thing (and there is no regressions later) :) Cheers, Matt
On Thu, Mar 7, 2024 at 5:33 PM Matthieu Baerts <matttbe@kernel.org> wrote: > > Hi Davide, > > > > > and I mixed variable names while cleaning up pr_debug()s. Non-MPC connection > > attempts (pure TCP, but also MPTCP with pathological MP_CAPABLE > > sub-options) have fallback equal to false here. Will fix this in v3. > > Maybe it also means it would be good to add a check in the selftests (in > mptcp_connect.sh?) to make sure it is doing the right thing (and there > is no regressions later) :) I can push a PR for packetdrill's 'mp_capable' v1_bind_*.pkt scenarios, is that ok?
Hi Davide, On 08/03/2024 10:14, Davide Caratti wrote: > On Thu, Mar 7, 2024 at 5:33 PM Matthieu Baerts <matttbe@kernel.org> wrote: >> >> Hi Davide, >> >>> >>> and I mixed variable names while cleaning up pr_debug()s. Non-MPC connection >>> attempts (pure TCP, but also MPTCP with pathological MP_CAPABLE >>> sub-options) have fallback equal to false here. Will fix this in v3. >> >> Maybe it also means it would be good to add a check in the selftests (in >> mptcp_connect.sh?) to make sure it is doing the right thing (and there >> is no regressions later) :) > > > I can push a PR for packetdrill's 'mp_capable' v1_bind_*.pkt > scenarios, is that ok? Yes, I guess it will be easier to check that the counter has been incremented when needed. What about also checking in mptcp_connect.sh that the counter is no longer incremented? I guess it should always be set to 0 after the patch, but it was not the case before, no? Cheers, Matt
hello, On Fri, Mar 08, 2024 at 10:54:31AM +0100, Matthieu Baerts wrote: > Hi Davide, > > On 08/03/2024 10:14, Davide Caratti wrote: > > On Thu, Mar 7, 2024 at 5:33 PM Matthieu Baerts <matttbe@kernel.org> wrote: > >> > >> Hi Davide, > What about also checking in mptcp_connect.sh that the counter is no > longer incremented? I guess it should always be set to 0 after the > patch, but it was not the case before, no? correct. that would result in checking that $expect_ackrx didn't increment when $cl_proto is "TCP" and $srv_proto is "MPTCP", right?
[...] On Mon, Mar 11, 2024 at 1:20 PM Davide Caratti <dcaratti@redhat.com> wrote: > > hello, > > On Fri, Mar 08, 2024 at 10:54:31AM +0100, Matthieu Baerts wrote: > > Hi Davide, > > > > On 08/03/2024 10:14, Davide Caratti wrote: > > > On Thu, Mar 7, 2024 at 5:33 PM Matthieu Baerts <matttbe@kernel.org> wrote: > > >> > > >> Hi Davide, > > > What about also checking in mptcp_connect.sh that the counter is no > > longer incremented? I guess it should always be set to 0 after the > > patch, but it was not the case before, no? > > correct. that would result in checking that $expect_ackrx didn't > increment when $cl_proto is "TCP" and $srv_proto is "MPTCP", right? ^^ scratch that, it's the wrong counter :) we need to ensure that neither MPTcpExtMPCapableSYNRX nor MPTcpExtMPCapableFallbackACK had an increase after ./mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} <...> ./mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} <...> when client is TCP and server is MPTCP. Let me try a patch for that, then will post v3. Thanks!
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index cdf9ec67795e..556b3b95c537 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3937,8 +3937,6 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock, mptcp_set_state(newsk, TCP_CLOSE); } } else { - MPTCP_INC_STATS(sock_net(ssk), - MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); tcpfallback: newsk->sk_kern_sock = kern; lock_sock(newsk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 1626dd20c68f..6e3fe38f057d 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -905,6 +905,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, return child; fallback: + if (child) + SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); mptcp_subflow_drop_ctx(child); return child; }
currently, inbound TCP connections increment MPTcpExtMPCapableFallbackACK when the server accepts them. As Christoph reported, this is "surprising" because the counter becomes greater than MPTcpExtMPCapableSYNRX when many non-MPC TCP connections are accepted. Change the semantic of MPTcpExtMPCapableFallbackACK to increment when the subflow context of an inbound MPC connection attempt is dropped. v2: fix reporter name Reported-by: Christoph Paasch <cpaasch@apple.com> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/449 Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/protocol.c | 2 -- net/mptcp/subflow.c | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)