| Message ID | 20250217-mptcp-dss-drop-mpj-v1-2-d671d6b9a153@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | mptcp: reset when MPTCP opts are dropped after MPJ | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
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
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 */
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(-)