Any fallback should happen only if allowed, so only if this variable is
set: msk->allow_infinite_fallback. This boolean will be set to false
once MPTCP-specific operations acting on the whole MPTCP connection vs
the initial path have been done, e.g. a second path has been created, or
an MPTCP re-injection -- yes, possible even with a single subflow.

In other words, the !msk->allow_infinite_fallback condition should be
placed first. If it is no longer possible to do a fallback, there should
not be any bypass, e.g. when receiving an infinite mapping.

Now, regarding the conditions to do a fallback, the RFC mentions [1]
that if the checksum is used, a fallback is only possible when "it is
known that all unacknowledged data in flight is contiguous (which will
usually be the case with a single subflow)". In other words, if the
checksum is used, a fallback is possible when the other peer sent an
infinite mapping indicating the flow has been altered.

Note that the issue is present since a merge commit, where both
subflow_can_fallback() and the previous extra condition with
'subflow->map_data_len' got introduced.

Fixes: d7e6f5836038 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.7-11 [1]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6ec03580ccec12fb50fc3aacf3d22413647b32b5..a8ad16db1ecd08ef7155a38161555253e4c72399 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1298,16 +1298,20 @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss
 		mptcp_schedule_work(sk);
 }
 
-static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
+static bool subflow_can_fallback(struct mptcp_subflow_context *subflow,
+				 enum mapping_status status)
 {
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 
-	if (subflow->mp_join)
+	/* If not allowed (additional paths, MPTCP reinjections): no fallback */
+	if (!READ_ONCE(msk->allow_infinite_fallback))
 		return false;
-	else if (READ_ONCE(msk->csum_enabled))
+
+	/* More strict with csum: fallback in 2 cases: inf map or never valid */
+	if (status != MAPPING_INFINITE && READ_ONCE(msk->csum_enabled))
 		return !subflow->valid_csum_seen;
-	else
-		return READ_ONCE(msk->allow_infinite_fallback);
+
+	return true;
 }
 
 static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
@@ -1406,7 +1410,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			return true;
 		}
 
-		if (!subflow_can_fallback(subflow) && status != MAPPING_INFINITE) {
+		if (!subflow_can_fallback(subflow, status)) {
 			/* fatal protocol error, close the socket.
 			 * subflow_error_report() will introduce the appropriate barriers
 			 */

-- 
2.47.1

On 2/17/25 9:37 AM, Matthieu Baerts (NGI0) wrote:
> Any fallback should happen only if allowed, so only if this variable is
> set: msk->allow_infinite_fallback. This boolean will be set to false
> once MPTCP-specific operations acting on the whole MPTCP connection vs
> the initial path have been done, e.g. a second path has been created, or
> an MPTCP re-injection -- yes, possible even with a single subflow.
> 
> In other words, the !msk->allow_infinite_fallback condition should be
> placed first. If it is no longer possible to do a fallback, there should
> not be any bypass, e.g. when receiving an infinite mapping.
> 
> Now, regarding the conditions to do a fallback, the RFC mentions [1]
> that if the checksum is used, a fallback is only possible when "it is
> known that all unacknowledged data in flight is contiguous (which will
> usually be the case with a single subflow)". In other words, if the
> checksum is used, a fallback is possible when the other peer sent an
> infinite mapping indicating the flow has been altered.
> 
> Note that the issue is present since a merge commit, where both
> subflow_can_fallback() and the previous extra condition with
> 'subflow->map_data_len' got introduced.
> 
> Fixes: d7e6f5836038 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
> Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.7-11 [1]
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I have the feeling this patch could/should be squashed into the previous
one.

> ---
>  net/mptcp/subflow.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6ec03580ccec12fb50fc3aacf3d22413647b32b5..a8ad16db1ecd08ef7155a38161555253e4c72399 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1298,16 +1298,20 @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss
>  		mptcp_schedule_work(sk);
>  }
>  
> -static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
> +static bool subflow_can_fallback(struct mptcp_subflow_context *subflow,
> +				 enum mapping_status status)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>  
> -	if (subflow->mp_join)
> +	/* If not allowed (additional paths, MPTCP reinjections): no fallback */
> +	if (!READ_ONCE(msk->allow_infinite_fallback))
>  		return false;
> -	else if (READ_ONCE(msk->csum_enabled))
> +
> +	/* More strict with csum: fallback in 2 cases: inf map or never valid */
> +	if (status != MAPPING_INFINITE && READ_ONCE(msk->csum_enabled))
>  		return !subflow->valid_csum_seen;

Is the above branch (csum_enabled) still needed? AFAICS csum fallback is
handled by the previous test:

	if (status == MAPPING_BAD_CSUM &&
	    (subflow->mp_join || subflow->valid_csum_seen)) {

Thanks!

Paolo

On 17/02/2025 19:21, Paolo Abeni wrote:
> 
> 
> On 2/17/25 9:37 AM, Matthieu Baerts (NGI0) wrote:
>> Any fallback should happen only if allowed, so only if this variable is
>> set: msk->allow_infinite_fallback. This boolean will be set to false
>> once MPTCP-specific operations acting on the whole MPTCP connection vs
>> the initial path have been done, e.g. a second path has been created, or
>> an MPTCP re-injection -- yes, possible even with a single subflow.
>>
>> In other words, the !msk->allow_infinite_fallback condition should be
>> placed first. If it is no longer possible to do a fallback, there should
>> not be any bypass, e.g. when receiving an infinite mapping.
>>
>> Now, regarding the conditions to do a fallback, the RFC mentions [1]
>> that if the checksum is used, a fallback is only possible when "it is
>> known that all unacknowledged data in flight is contiguous (which will
>> usually be the case with a single subflow)". In other words, if the
>> checksum is used, a fallback is possible when the other peer sent an
>> infinite mapping indicating the flow has been altered.
>>
>> Note that the issue is present since a merge commit, where both
>> subflow_can_fallback() and the previous extra condition with
>> 'subflow->map_data_len' got introduced.
>>
>> Fixes: d7e6f5836038 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
>> Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.7-11 [1]
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> I have the feeling this patch could/should be squashed into the previous
> one.

When I created the commit, I struggled a bit to describe this change as
it was not directly related to my issue (subflow not reset when MPTCP
options are dropped on the second subflow), then when I saw the origin
(Fixes tags) were different, and I did the split. But I can squash them
if you prefer.

>> ---
>>  net/mptcp/subflow.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 6ec03580ccec12fb50fc3aacf3d22413647b32b5..a8ad16db1ecd08ef7155a38161555253e4c72399 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1298,16 +1298,20 @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss
>>  		mptcp_schedule_work(sk);
>>  }
>>  
>> -static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
>> +static bool subflow_can_fallback(struct mptcp_subflow_context *subflow,
>> +				 enum mapping_status status)
>>  {
>>  	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>  
>> -	if (subflow->mp_join)
>> +	/* If not allowed (additional paths, MPTCP reinjections): no fallback */
>> +	if (!READ_ONCE(msk->allow_infinite_fallback))
>>  		return false;
>> -	else if (READ_ONCE(msk->csum_enabled))
>> +
>> +	/* More strict with csum: fallback in 2 cases: inf map or never valid */
>> +	if (status != MAPPING_INFINITE && READ_ONCE(msk->csum_enabled))
>>  		return !subflow->valid_csum_seen;
> 
> Is the above branch (csum_enabled) still needed? AFAICS csum fallback is
> handled by the previous test:
> 
> 	if (status == MAPPING_BAD_CSUM &&
> 	    (subflow->mp_join || subflow->valid_csum_seen)) {

Good point. Then I guess we don't need the new status for infinite
mapping, and we can even drop subflow_can_fallback() and only use
msk->allow_infinite_fallback.

Thank you for the review, I will prepare a v2 squashing the two first
patches and simplifying them.

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