[PATCH mptcp-net 1/2] mptcp: fix bad RCVPRUNED mib accounting

[PATCH mptcp-net 1/2] mptcp: fix bad RCVPRUNED mib accounting

[Paolo Abeni]

Since its introduction, the mentioned MIB accounted for the wrong
event: wake-up being skipped as not-needed on some edge condition
instead of incoming skb being dropped after landing in the (subflow)
receive queue.

Move the increment in the correct location.

Fixes: ce599c516386 ("mptcp: properly account bulk freed memory")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b3a48d97f009..13777c35496c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -350,8 +350,10 @@ static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
 	skb_orphan(skb);
 
 	/* try to fetch required memory from subflow */
-	if (!mptcp_rmem_schedule(sk, ssk, skb->truesize))
+	if (!mptcp_rmem_schedule(sk, ssk, skb->truesize)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
 		goto drop;
+	}
 
 	has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
 
@@ -844,10 +846,8 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
 		sk_rbuf = ssk_rbuf;
 
 	/* over limit? can't append more skbs to msk, Also, no need to wake-up*/
-	if (__mptcp_rmem(sk) > sk_rbuf) {
-		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
+	if (__mptcp_rmem(sk) > sk_rbuf)
 		return;
-	}
 
 	/* Wake-up the reader only for in-sequence data */
 	mptcp_data_lock(sk);
-- 
2.45.2

[PATCH mptcp-net 2/2] mptcp: fix duplicate data handling

[Paolo Abeni]

When a subflow receives and discards duplicate data, the mptcp
stack assumes that the consumed offset inside the current skb is
zero.

With multiple subflows receiving data simultaneously such assertion
does not held true. As a result the subflow-level copied_seq will
be incorrectly increased and later on the same subflow will observe
a bad mapping, leading to subflow reset.

Address the issue tacking in account the skb consumed offset in
mptcp_subflow_discard_data().

Fixes: 04e4cd4f7ca4 ("mptcp: cleanup mptcp_subflow_discard_data()")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 0e4b5bfbeaa1..a21c712350c3 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1230,14 +1230,22 @@ static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb,
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	bool fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN;
-	u32 incr;
+	struct tcp_sock *tp = tcp_sk(ssk);
+	u32 offset, incr, avail_len;
 
-	incr = limit >= skb->len ? skb->len + fin : limit;
+	offset = tp->copied_seq - TCP_SKB_CB(skb)->seq;
+	if (WARN_ON_ONCE(offset > skb->len))
+		goto out;
+
+	avail_len = skb->len - offset;
+	incr = limit >= avail_len ? avail_len + fin : limit;
 
-	pr_debug("discarding=%d len=%d seq=%d", incr, skb->len,
-		 subflow->map_subflow_seq);
+	pr_debug("discarding=%d len=%d offset=%d seq=%d", incr, skb->len,
+		 offset, subflow->map_subflow_seq);
 	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DUPDATA);
 	tcp_sk(ssk)->copied_seq += incr;
+
+out:
 	if (!before(tcp_sk(ssk)->copied_seq, TCP_SKB_CB(skb)->end_seq))
 		sk_eat_skb(ssk, skb);
 	if (mptcp_subflow_get_map_offset(subflow) >= subflow->map_data_len)
-- 
2.45.2

Re: [PATCH mptcp-net 2/2] mptcp: fix duplicate data handling

[Mat Martineau]

On Thu, 25 Jul 2024, Paolo Abeni wrote:

> When a subflow receives and discards duplicate data, the mptcp
> stack assumes that the consumed offset inside the current skb is
> zero.
>
> With multiple subflows receiving data simultaneously such assertion
> does not held true. As a result the subflow-level copied_seq will
> be incorrectly increased and later on the same subflow will observe
> a bad mapping, leading to subflow reset.
>
> Address the issue tacking in account the skb consumed offset in

Just one fix but Matthieu can adjust: "Address the issue taking into 
account..."

Reviewed-by: Mat Martineau <martineau@kernel.org>

> mptcp_subflow_discard_data().
>
> Fixes: 04e4cd4f7ca4 ("mptcp: cleanup mptcp_subflow_discard_data()")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/subflow.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 0e4b5bfbeaa1..a21c712350c3 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1230,14 +1230,22 @@ static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb,
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> 	bool fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN;
> -	u32 incr;
> +	struct tcp_sock *tp = tcp_sk(ssk);
> +	u32 offset, incr, avail_len;
>
> -	incr = limit >= skb->len ? skb->len + fin : limit;
> +	offset = tp->copied_seq - TCP_SKB_CB(skb)->seq;
> +	if (WARN_ON_ONCE(offset > skb->len))
> +		goto out;
> +
> +	avail_len = skb->len - offset;
> +	incr = limit >= avail_len ? avail_len + fin : limit;
>
> -	pr_debug("discarding=%d len=%d seq=%d", incr, skb->len,
> -		 subflow->map_subflow_seq);
> +	pr_debug("discarding=%d len=%d offset=%d seq=%d", incr, skb->len,
> +		 offset, subflow->map_subflow_seq);
> 	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DUPDATA);
> 	tcp_sk(ssk)->copied_seq += incr;
> +
> +out:
> 	if (!before(tcp_sk(ssk)->copied_seq, TCP_SKB_CB(skb)->end_seq))
> 		sk_eat_skb(ssk, skb);
> 	if (mptcp_subflow_get_map_offset(subflow) >= subflow->map_data_len)
> --

Re: [PATCH mptcp-net 2/2] mptcp: fix duplicate data handling

[Matthieu Baerts]

Hi Paolo, Mat,

On 26/07/2024 02:42, Mat Martineau wrote:
> On Thu, 25 Jul 2024, Paolo Abeni wrote:
> 
>> When a subflow receives and discards duplicate data, the mptcp
>> stack assumes that the consumed offset inside the current skb is
>> zero.
>>
>> With multiple subflows receiving data simultaneously such assertion
>> does not held true. As a result the subflow-level copied_seq will
>> be incorrectly increased and later on the same subflow will observe
>> a bad mapping, leading to subflow reset.

Thank you for the patches and the reviews!

>> Address the issue tacking in account the skb consumed offset in
> 
> Just one fix but Matthieu can adjust: "Address the issue taking into
> account..."

Just did, thanks!

Now in our tree (fixes for -net)

New patches for t/upstream-net and t/upstream:
- 6d60c2cf352d: mptcp: fix bad RCVPRUNED mib accounting
- 2e6def94f212: mptcp: fix duplicate data handling
- Results: a1f9b538328a..0e547869a341 (export-net)
- Results: 8149d851361f..9a749f3dd928 (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/865fc5f5109629347844c647fa71a50484d02dec/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/166ef45101617d160bacd4c3330e32f8f71fcf17/checks

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

Re: [PATCH mptcp-net 2/2] mptcp: fix duplicate data handling

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10108418141

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/9d038da097ea
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=873877


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)

Re: [PATCH mptcp-net 1/2] mptcp: fix bad RCVPRUNED mib accounting

[Mat Martineau]

On Thu, 25 Jul 2024, Paolo Abeni wrote:

> Since its introduction, the mentioned MIB accounted for the wrong
> event: wake-up being skipped as not-needed on some edge condition
> instead of incoming skb being dropped after landing in the (subflow)
> receive queue.
>
> Move the increment in the correct location.
>
> Fixes: ce599c516386 ("mptcp: properly account bulk freed memory")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b3a48d97f009..13777c35496c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -350,8 +350,10 @@ static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
> 	skb_orphan(skb);
>
> 	/* try to fetch required memory from subflow */
> -	if (!mptcp_rmem_schedule(sk, ssk, skb->truesize))
> +	if (!mptcp_rmem_schedule(sk, ssk, skb->truesize)) {
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);

Hi Paolo -

MIB change LGTM:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> 		goto drop;
> +	}
>
> 	has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
>
> @@ -844,10 +846,8 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
> 		sk_rbuf = ssk_rbuf;
>
> 	/* over limit? can't append more skbs to msk, Also, no need to wake-up*/
> -	if (__mptcp_rmem(sk) > sk_rbuf) {
> -		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
> +	if (__mptcp_rmem(sk) > sk_rbuf)
> 		return;
> -	}
>
> 	/* Wake-up the reader only for in-sequence data */
> 	mptcp_data_lock(sk);