linux-kernel - Re: [linux-next] [6f5b24e] WARNING: CPU: 0 PID: 10288 at net/ipv4/tcp_input.c:889 tcp_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACSApva6SkJe8kj+3TKyD+67dJ8qX-HHRDpxRBQskFCqzd-Mfg@mail.gmail.com>
Date:   Wed, 11 Oct 2017 10:08:07 -0400
From:   Soheil Hassas Yeganeh <soheil.kdev@...il.com>
To:     Abdul Haleem <abdhalee@...ux.vnet.ibm.com>
Cc:     linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-next <linux-next@...r.kernel.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        mpe <mpe@...erman.id.au>, sachinp <sachinp@...ux.vnet.ibm.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [linux-next] [6f5b24e] WARNING: CPU: 0 PID: 10288 at
 net/ipv4/tcp_input.c:889 tcp_update_reordering+0x1c4/0x1e0

On Wed, Oct 11, 2017 at 6:02 AM, Abdul Haleem
<abdhalee@...ux.vnet.ibm.com> wrote:
> Hi,
>
> CPU OFF & ON in a loop on next kernel results in WARNING in dmesg.
>
> Machine Type: Power 8 PowerVM LPAR
> Kernel : 4.14.0-rc4-next-20171009
> Gcc : 6.3.1
> config : attached
> Test: CPU toggle
>
> WARN_ON was first introduced with the commit:
>
> commit 6f5b24eed0278136c29c27f2a7b3a2b6a202ac68
> Author: Soheil Hassas Yeganeh <soheil@...gle.com>
> Date:   Tue May 16 17:39:02 2017 -0400
>
>     tcp: warn on negative reordering values
>
>     Commit bafbb9c73241 ("tcp: eliminate negative reordering
>     in tcp_clean_rtx_queue") fixes an issue for negative
>     reordering metrics.
>
>     To be resilient to such errors, warn and return
>     when a negative metric is passed to tcp_update_reordering().
>
>     Signed-off-by: Soheil Hassas Yeganeh <soheil@...gle.com>
>     Signed-off-by: Neal Cardwell <ncardwell@...gle.com>
>     Signed-off-by: Yuchung Cheng <ycheng@...gle.com>
>     Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index bbadd79..2fa55f5 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -887,6 +887,9 @@ static void tcp_update_reordering(struct sock *sk,
> const int metric,
>         struct tcp_sock *tp = tcp_sk(sk);
>         int mib_idx;
>
> +       if (WARN_ON_ONCE(metric < 0))
> +               return;
> +

Thank you for your report, Abdul! This warning was added to find the
root cause of insanely high reordering in TCP, which luckily I believe
you have reproduced.

Yuchung had pointed out offline that we are using inconsistent types
in using in tcp_sacktag_state vs tcp_sock: int for reord and
fack_count in tcp_sacktag_state and u32 for their related fields in
tcp_sock. This is likely to be the culprit here.

Would you mind giving the following patch a try please? If that fixes
the issue for you, we will mail that as a fix. Thanks!

From: Soheil Hassas Yeganeh <soheil@...gle.com>
Date: Wed, 11 Oct 2017 09:52:49 -0400
Subject: [PATCH net] tcp: fix type of fack_count and reord in sack_tag_state

This bug is found by Yuchung.

Signed-off-by: Soheil Hassas Yeganeh <soheil@...gle.com>
---
 net/ipv4/tcp_input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d0682ce2a5d6..2fada7acbdc5 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1132,8 +1132,8 @@ static bool tcp_check_dsack(struct sock *sk,
const struct sk_buff *ack_skb,
 }

 struct tcp_sacktag_state {
-       int reord;
-       int fack_count;
+       u32 reord;
+       u32 fack_count;
        /* Timestamps for earliest and latest never-retransmitted segment
         * that was SACKed. RTO needs the earliest RTT to stay conservative,
         * but congestion control should still get an accurate delay signal.
@@ -1209,7 +1209,7 @@ static u8 tcp_sacktag_one(struct sock *sk,
                        u64 xmit_time)
 {
        struct tcp_sock *tp = tcp_sk(sk);
-       int fack_count = state->fack_count;
+       u32 fack_count = state->fack_count;

        /* Account D-SACK for retransmitted packet. */
        if (dup_sack && (sacked & TCPCB_RETRANS)) {
-- 
2.15.0.rc0.271.g36b669edcc-goog