[v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

[PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Davide Caratti 1 month, 3 weeks ago

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(-)

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;
 }
-- 
2.43.0

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Matthieu Baerts 1 month, 3 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Davide Caratti 1 month, 3 weeks ago

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.

-- 
davide

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Matthieu Baerts 1 month, 3 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Davide Caratti 1 month, 3 weeks ago

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?
-- 
davide

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Matthieu Baerts 1 month, 3 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Davide Caratti 1 month, 2 weeks ago

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?

-- 
davide

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Davide Caratti 1 month, 2 weeks ago

[...]

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!
-- 
davide

Re: [PATCH mptcp-net v2] mptcp: don't account accept() of non-MPC client as fallback to TCP

Posted by Matthieu Baerts 1 month, 3 weeks ago

On 07/03/2024 11:05, Matthieu Baerts wrote:
> Hi Davide,

I forgot to add: thank you for having worked on that :)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.