Hi all,
On Thu, Aug 15, 2024 at 03:25:13PM +0200, Greg Kroah-Hartman wrote:
> 5.10-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
>
> commit 68cc924729ffcfe90d0383177192030a9aeb2ee4 upstream.
>
> 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 taking into account the skb consumed offset in
> mptcp_subflow_discard_data().
>
> Fixes: 04e4cd4f7ca4 ("mptcp: cleanup mptcp_subflow_discard_data()")
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/501
> Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> Reviewed-by: Mat Martineau <martineau@xxxxxxxxxx>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
> Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> net/mptcp/subflow.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -863,14 +863,22 @@ static void mptcp_subflow_discard_data(s
> {
> 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;
>
> - pr_debug("discarding=%d len=%d seq=%d", incr, skb->len,
> - subflow->map_subflow_seq);
> + avail_len = skb->len - offset;
> + incr = limit >= avail_len ? avail_len + fin : limit;
> +
> + 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)
>
>
This change in 5.10 appears to introduce an instance of
-Wsometimes-uninitialized because 5.10 does not include
commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling"), which
removed the use of incr in the error path added by this change:
$ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 LLVM_IAS=1 mrproper allmodconfig net/mptcp/subflow.o
net/mptcp/subflow.c:877:6: warning: variable 'incr' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
877 | if (WARN_ON_ONCE(offset > skb->len))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/bug.h:101:33: note: expanded from macro 'WARN_ON_ONCE'
101 | #define WARN_ON_ONCE(condition) ({ \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
102 | int __ret_warn_on = !!(condition); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
103 | if (unlikely(__ret_warn_on)) \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
104 | __WARN_FLAGS(BUGFLAG_ONCE | \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
105 | BUGFLAG_TAINT(TAINT_WARN)); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
106 | unlikely(__ret_warn_on); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
107 | })
| ~~
net/mptcp/subflow.c:893:6: note: uninitialized use occurs here
893 | if (incr)
| ^~~~
net/mptcp/subflow.c:877:2: note: remove the 'if' if its condition is always false
877 | if (WARN_ON_ONCE(offset > skb->len))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
878 | goto out;
| ~~~~~~~~
net/mptcp/subflow.c:874:18: note: initialize the variable 'incr' to silence this warning
874 | u32 offset, incr, avail_len;
| ^
| = 0
1 warning generated.
That change does not really look suitable for stable (unless folks feel
otherwise), so maybe a stable only patch to adddress this is in order?
Sorry for the delay in reporting this, I guess I do not build older
stable releases as often as I should.
Cheers,
Nathan
