Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302893 > unrolled thread

Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-06 18:00 +0100
    Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Yuchung Cheng <ycheng@google.com> - 2016-01-06 19:30 +0100
      Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-06 19:50 +0100
      Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Yuchung Cheng <ycheng@google.com> - 2016-01-06 19:50 +0100
        Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-09 18:40 +0100
        Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-10 11:30 +0100
          Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Neal Cardwell <ncardwell@google.com> - 2016-01-10 15:50 +0100
            Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Neal Cardwell <ncardwell@google.com> - 2016-01-10 16:00 +0100
              Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-10 16:00 +0100
            Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-10 16:00 +0100
              Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Neal Cardwell <ncardwell@google.com> - 2016-01-10 18:40 +0100
                Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-10 19:00 +0100
                  Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Neal Cardwell <ncardwell@google.com> - 2016-01-10 19:10 +0100
                Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-10 23:00 +0100
                  Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible)  division by zero Neal Cardwell <ncardwell@google.com> - 2016-01-11 19:50 +0100
                    Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-01-12 00:30 +0100

#1302893 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

Unfortunately, the patch didn't help -- I've got the same stacktrace with slightly different offset (+3) within the function.

Now trying to get full stacktrace via netconsole. Need more time.

Meanwhile, any other ideas on what went wrong?


On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> wrote:
>On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
><oleksandr@natalenko.name> wrote:
>> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
>mode by
>> default and SS mode conditionally) introduced changes to
>net/ipv4/tcp_input.c
>> tcp_cwnd_reduction() that, possibly, cause division by zero, and
>therefore,
>> kernel panic in interrupt handler [1].
>>
>> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
>issue.
>>
>> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
>> (occasionally).
>>
>> What could be done to help in debugging this issue?
>Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
>
>If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
>state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
>you try this debug / quick-fix patch and send me the error message if
>any?
>
>
>>
>> Regards,
>>   Oleksandr.
>>
>> [1] http://i.piccy.info/
>>
>i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302958 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Wed, Jan 6, 2016 at 8:50 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
>
> Unfortunately, the patch didn't help -- I've got the same stacktrace with slightly different offset (+3) within the function.
>
> Now trying to get full stacktrace via netconsole. Need more time.
>
> Meanwhile, any other ideas on what went wrong?

That's odd b/c the patch already checks and avoids div0. Can post me
the stacktrace and kernel warnings if any ...

One possibility is that tcp_cwnd_reduction() may set a cwnd of 0,
which then gets used to start another recovery phase. This may or may
not be the culprit of this div0 issue because I wasn't able to
reproduce exactly your issue on our servers. But I will post the fix
today and CC you.

>
>
> On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> wrote:
> >On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
> ><oleksandr@natalenko.name> wrote:
> >> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
> >mode by
> >> default and SS mode conditionally) introduced changes to
> >net/ipv4/tcp_input.c
> >> tcp_cwnd_reduction() that, possibly, cause division by zero, and
> >therefore,
> >> kernel panic in interrupt handler [1].
> >>
> >> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
> >issue.
> >>
> >> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
> >> (occasionally).
> >>
> >> What could be done to help in debugging this issue?
> >Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
> >
> >If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
> >state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
> >you try this debug / quick-fix patch and send me the error message if
> >any?
> >
> >
> >>
> >> Regards,
> >>   Oleksandr.
> >>
> >> [1] http://i.piccy.info/
> >>
> >i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302978

Sure, but after catching the stacktrace.

On середа, 6 січня 2016 р. 10:43:45 EET Yuchung Cheng wrote:
> Could you turn off ecn (sysctl net.ipv4.tcp_ecn=0) to see if this still
> happen?


> >> On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> 
wrote:
> >> >On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
> >> >
> >> ><oleksandr@natalenko.name> wrote:
> >> >> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
> >> >
> >> >mode by
> >> >
> >> >> default and SS mode conditionally) introduced changes to
> >> >
> >> >net/ipv4/tcp_input.c
> >> >
> >> >> tcp_cwnd_reduction() that, possibly, cause division by zero, and
> >> >
> >> >therefore,
> >> >
> >> >> kernel panic in interrupt handler [1].
> >> >> 
> >> >> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
> >> >
> >> >issue.
> >> >
> >> >> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
> >> >> (occasionally).
> >> >> 
> >> >> What could be done to help in debugging this issue?
> >> >
> >> >Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
> >> >
> >> >If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
> >> >state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
> >> >you try this debug / quick-fix patch and send me the error message if
> >> >any?
> >> >
> >> >> Regards,
> >> >> 
> >> >>   Oleksandr.
> >> >> 
> >> >> [1] http://i.piccy.info/
> >> >
> >> >i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg
> >> 
> >> --
> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302982 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Wed, Jan 6, 2016 at 10:19 AM, Yuchung Cheng <ycheng@google.com> wrote:
> On Wed, Jan 6, 2016 at 8:50 AM, Oleksandr Natalenko
> <oleksandr@natalenko.name> wrote:
>>
>> Unfortunately, the patch didn't help -- I've got the same stacktrace with slightly different offset (+3) within the function.
>>
>> Now trying to get full stacktrace via netconsole. Need more time.
>>
>> Meanwhile, any other ideas on what went wrong?
>
> That's odd b/c the patch already checks and avoids div0. Can post me
> the stacktrace and kernel warnings if any ...
>
> One possibility is that tcp_cwnd_reduction() may set a cwnd of 0,
> which then gets used to start another recovery phase. This may or may
> not be the culprit of this div0 issue because I wasn't able to
> reproduce exactly your issue on our servers. But I will post the fix
> today and CC you.
Could you turn off ecn (sysctl net.ipv4.tcp_ecn=0) to see if this still happen?

>
>>
>>
>> On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> wrote:
>> >On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
>> ><oleksandr@natalenko.name> wrote:
>> >> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
>> >mode by
>> >> default and SS mode conditionally) introduced changes to
>> >net/ipv4/tcp_input.c
>> >> tcp_cwnd_reduction() that, possibly, cause division by zero, and
>> >therefore,
>> >> kernel panic in interrupt handler [1].
>> >>
>> >> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
>> >issue.
>> >>
>> >> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
>> >> (occasionally).
>> >>
>> >> What could be done to help in debugging this issue?
>> >Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
>> >
>> >If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
>> >state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
>> >you try this debug / quick-fix patch and send me the error message if
>> >any?
>> >
>> >
>> >>
>> >> Regards,
>> >>   Oleksandr.
>> >>
>> >> [1] http://i.piccy.info/
>> >>
>> >i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg
>>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1305330

Here is stacktrace got via netconsole with ECN enabled and first patch 
applied:

https://gist.github.com/57fd5e2795b86c40eb80

Now will recompile kernel with patch you've sent to upstream and test it with 
ECN enabled again.

On середа, 6 січня 2016 р. 10:43:45 EET Yuchung Cheng wrote:
> On Wed, Jan 6, 2016 at 10:19 AM, Yuchung Cheng <ycheng@google.com> wrote:
> > On Wed, Jan 6, 2016 at 8:50 AM, Oleksandr Natalenko
> > 
> > <oleksandr@natalenko.name> wrote:
> >> Unfortunately, the patch didn't help -- I've got the same stacktrace with
> >> slightly different offset (+3) within the function.
> >> 
> >> Now trying to get full stacktrace via netconsole. Need more time.
> >> 
> >> Meanwhile, any other ideas on what went wrong?
> > 
> > That's odd b/c the patch already checks and avoids div0. Can post me
> > the stacktrace and kernel warnings if any ...
> > 
> > One possibility is that tcp_cwnd_reduction() may set a cwnd of 0,
> > which then gets used to start another recovery phase. This may or may
> > not be the culprit of this div0 issue because I wasn't able to
> > reproduce exactly your issue on our servers. But I will post the fix
> > today and CC you.
> 
> Could you turn off ecn (sysctl net.ipv4.tcp_ecn=0) to see if this still
> happen?
> >> On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> 
wrote:
> >> >On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
> >> >
> >> ><oleksandr@natalenko.name> wrote:
> >> >> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
> >> >
> >> >mode by
> >> >
> >> >> default and SS mode conditionally) introduced changes to
> >> >
> >> >net/ipv4/tcp_input.c
> >> >
> >> >> tcp_cwnd_reduction() that, possibly, cause division by zero, and
> >> >
> >> >therefore,
> >> >
> >> >> kernel panic in interrupt handler [1].
> >> >> 
> >> >> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
> >> >
> >> >issue.
> >> >
> >> >> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
> >> >> (occasionally).
> >> >> 
> >> >> What could be done to help in debugging this issue?
> >> >
> >> >Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
> >> >
> >> >If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
> >> >state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
> >> >you try this debug / quick-fix patch and send me the error message if
> >> >any?
> >> >
> >> >> Regards,
> >> >> 
> >> >>   Oleksandr.
> >> >> 
> >> >> [1] http://i.piccy.info/
> >> >
> >> >i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg
> >> 
> >> --
> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1305530

With the patch queued for upstream and ECN enabled I get WARN_ON_ONCE() 
triggered. Here is the stacktrace:

https://gist.github.com/89203e77bfcb051f269a

It seems that tp->prior_cwnd is zero.

Ideas?

On середа, 6 січня 2016 р. 10:43:45 EET Yuchung Cheng wrote:
> On Wed, Jan 6, 2016 at 10:19 AM, Yuchung Cheng <ycheng@google.com> wrote:
> > On Wed, Jan 6, 2016 at 8:50 AM, Oleksandr Natalenko
> > 
> > <oleksandr@natalenko.name> wrote:
> >> Unfortunately, the patch didn't help -- I've got the same stacktrace with
> >> slightly different offset (+3) within the function.
> >> 
> >> Now trying to get full stacktrace via netconsole. Need more time.
> >> 
> >> Meanwhile, any other ideas on what went wrong?
> > 
> > That's odd b/c the patch already checks and avoids div0. Can post me
> > the stacktrace and kernel warnings if any ...
> > 
> > One possibility is that tcp_cwnd_reduction() may set a cwnd of 0,
> > which then gets used to start another recovery phase. This may or may
> > not be the culprit of this div0 issue because I wasn't able to
> > reproduce exactly your issue on our servers. But I will post the fix
> > today and CC you.
> 
> Could you turn off ecn (sysctl net.ipv4.tcp_ecn=0) to see if this still
> happen?
> >> On December 22, 2015 4:10:32 AM EET, Yuchung Cheng <ycheng@google.com> 
wrote:
> >> >On Mon, Dec 21, 2015 at 12:25 PM, Oleksandr Natalenko
> >> >
> >> ><oleksandr@natalenko.name> wrote:
> >> >> Commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 (tcp: PRR uses CRB
> >> >
> >> >mode by
> >> >
> >> >> default and SS mode conditionally) introduced changes to
> >> >
> >> >net/ipv4/tcp_input.c
> >> >
> >> >> tcp_cwnd_reduction() that, possibly, cause division by zero, and
> >> >
> >> >therefore,
> >> >
> >> >> kernel panic in interrupt handler [1].
> >> >> 
> >> >> Reverting 3759824da87b30ce7a35b4873b62b0ba38905ef5 seems to fix the
> >> >
> >> >issue.
> >> >
> >> >> I'm able to reproduce the issue on 4.3.0–4.3.3 once per several day
> >> >> (occasionally).
> >> >> 
> >> >> What could be done to help in debugging this issue?
> >> >
> >> >Do you have ECN enabled (i.e. sysctl net.ipv4.tcp_ecn > 0)?
> >> >
> >> >If so I suspect an ACK carrying ECE during CA_Loss causes entering CWR
> >> >state w/o calling tcp_init_cwnd_reduct() to set tp->prior_cwnd. Can
> >> >you try this debug / quick-fix patch and send me the error message if
> >> >any?
> >> >
> >> >> Regards,
> >> >> 
> >> >>   Oleksandr.
> >> >> 
> >> >> [1] http://i.piccy.info/
> >> >
> >> >i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg
> >> 
> >> --
> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1305608 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Sun, Jan 10, 2016 at 5:23 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> With the patch queued for upstream and ECN enabled I get WARN_ON_ONCE()
> triggered. Here is the stacktrace:
>
> https://gist.github.com/89203e77bfcb051f269a
>
> It seems that tp->prior_cwnd is zero.

Hmm. Interesting. Can you please confirm that you are using the cubic
congestion control module?

thanks,
neal

[toc] | [prev] | [next] | [standalone]

#1305610 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Sun, Jan 10, 2016 at 9:48 AM, Neal Cardwell <ncardwell@google.com> wrote:
> On Sun, Jan 10, 2016 at 5:23 AM, Oleksandr Natalenko
> <oleksandr@natalenko.name> wrote:
>> With the patch queued for upstream and ECN enabled I get WARN_ON_ONCE()
>> triggered. Here is the stacktrace:
>>
>> https://gist.github.com/89203e77bfcb051f269a
>>
>> It seems that tp->prior_cwnd is zero.
>
> Hmm. Interesting. Can you please confirm that you are using the cubic
> congestion control module?

Would you be able to do an experiment where you reboot, and then
disable ECN upon startup, and then see if the warning still shows up
without ECN?

thanks,
neal

[toc] | [prev] | [next] | [standalone]

#1305612

Yes, this is my next step. Then I'll try to move from YeAH.

On неділя, 10 січня 2016 р. 09:54:16 EET Neal Cardwell wrote:
> On Sun, Jan 10, 2016 at 9:48 AM, Neal Cardwell <ncardwell@google.com> wrote:
> > On Sun, Jan 10, 2016 at 5:23 AM, Oleksandr Natalenko
> > 
> > <oleksandr@natalenko.name> wrote:
> >> With the patch queued for upstream and ECN enabled I get WARN_ON_ONCE()
> >> triggered. Here is the stacktrace:
> >> 
> >> https://gist.github.com/89203e77bfcb051f269a
> >> 
> >> It seems that tp->prior_cwnd is zero.
> > 
> > Hmm. Interesting. Can you please confirm that you are using the cubic
> > congestion control module?
> 
> Would you be able to do an experiment where you reboot, and then
> disable ECN upon startup, and then see if the warning still shows up
> without ECN?
> 
> thanks,
> neal

[toc] | [prev] | [next] | [standalone]

#1305611

I use YeAH. But YeAH code wasn't touched between 4.2 and 4.3.

On неділя, 10 січня 2016 р. 09:48:20 EET Neal Cardwell wrote:
> On Sun, Jan 10, 2016 at 5:23 AM, Oleksandr Natalenko
> 
> <oleksandr@natalenko.name> wrote:
> > With the patch queued for upstream and ECN enabled I get WARN_ON_ONCE()
> > triggered. Here is the stacktrace:
> > 
> > https://gist.github.com/89203e77bfcb051f269a
> > 
> > It seems that tp->prior_cwnd is zero.
> 
> Hmm. Interesting. Can you please confirm that you are using the cubic
> congestion control module?
> 
> thanks,
> neal

[toc] | [prev] | [next] | [standalone]

#1305640 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Sun, Jan 10, 2016 at 9:57 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> I use YeAH. But YeAH code wasn't touched between 4.2 and 4.3.

Oh, interesting. Looks like tcp_yeah_ssthresh() has a bug where its
intended reduction can be bigger than tp->snd_cwnd, leading to it
return a zero ssthresh (or even an ssthresh that underflows to ~4
billion). If tcp_yeah_ssthresh() returns an ssthresh of 0 then PRR
will try to pull the cwnd down to 0.

Can you please leave ECN and Yeah enabled and run something like the
following patch, to verify this conjecture? If the conjecture is
right, then the tcp_yeah warning should fire but not the new
tcp_cwnd_reduction() warning:

-----------
diff --git a/net/ipv4/tcp_yeah.c b/net/ipv4/tcp_yeah.c
index 17d3566..ef60cba 100644
--- a/net/ipv4/tcp_yeah.c
+++ b/net/ipv4/tcp_yeah.c
@@ -206,6 +206,7 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
        const struct tcp_sock *tp = tcp_sk(sk);
        struct yeah *yeah = inet_csk_ca(sk);
        u32 reduction;
+       s32 ssthresh;

        if (yeah->doing_reno_now < TCP_YEAH_RHO) {
                reduction = yeah->lastQ;
@@ -219,7 +220,9 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
        yeah->fast_count = 0;
        yeah->reno_count = max(yeah->reno_count>>1, 2U);

-       return tp->snd_cwnd - reduction;
+       ssthresh = tp->snd_cwnd - reduction;
+       if (WARN_ON_ONCE(ssthresh <= 0))
+               ssthresh = 1;
 }

 static struct tcp_congestion_ops tcp_yeah __read_mostly = {
-----------

If that works, then we may just want a version of this patch without
the warning.

Thanks!
neal

[toc] | [prev] | [next] | [standalone]

#1305649

Haven't you missed "return ssthresh;" statement?

On неділя, 10 січня 2016 р. 12:29:17 EET Neal Cardwell wrote:
> On Sun, Jan 10, 2016 at 9:57 AM, Oleksandr Natalenko
> 
> <oleksandr@natalenko.name> wrote:
> > I use YeAH. But YeAH code wasn't touched between 4.2 and 4.3.
> 
> Oh, interesting. Looks like tcp_yeah_ssthresh() has a bug where its
> intended reduction can be bigger than tp->snd_cwnd, leading to it
> return a zero ssthresh (or even an ssthresh that underflows to ~4
> billion). If tcp_yeah_ssthresh() returns an ssthresh of 0 then PRR
> will try to pull the cwnd down to 0.
> 
> Can you please leave ECN and Yeah enabled and run something like the
> following patch, to verify this conjecture? If the conjecture is
> right, then the tcp_yeah warning should fire but not the new
> tcp_cwnd_reduction() warning:
> 
> -----------
> diff --git a/net/ipv4/tcp_yeah.c b/net/ipv4/tcp_yeah.c
> index 17d3566..ef60cba 100644
> --- a/net/ipv4/tcp_yeah.c
> +++ b/net/ipv4/tcp_yeah.c
> @@ -206,6 +206,7 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
>         const struct tcp_sock *tp = tcp_sk(sk);
>         struct yeah *yeah = inet_csk_ca(sk);
>         u32 reduction;
> +       s32 ssthresh;
> 
>         if (yeah->doing_reno_now < TCP_YEAH_RHO) {
>                 reduction = yeah->lastQ;
> @@ -219,7 +220,9 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
>         yeah->fast_count = 0;
>         yeah->reno_count = max(yeah->reno_count>>1, 2U);
> 
> -       return tp->snd_cwnd - reduction;
> +       ssthresh = tp->snd_cwnd - reduction;
> +       if (WARN_ON_ONCE(ssthresh <= 0))
> +               ssthresh = 1;
>  }
> 
>  static struct tcp_congestion_ops tcp_yeah __read_mostly = {
> -----------
> 
> If that works, then we may just want a version of this patch without
> the warning.
> 
> Thanks!
> neal

[toc] | [prev] | [next] | [standalone]

#1305650 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Sun, Jan 10, 2016 at 12:50 PM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> Haven't you missed "return ssthresh;" statement?

Yes, sorry. You'd clearly need to return ssthresh as well. :-)

neal

[toc] | [prev] | [next] | [standalone]

#1305709

OK, it seems the assumption about YeAH is correct. Here is stacktrace fired by 
WARN_ON_ONCE():

https://gist.github.com/851cedcfca60d6120035

Is there sufficient info for you to prepare upstream patch?

On неділя, 10 січня 2016 р. 12:29:17 EET Neal Cardwell wrote:
> On Sun, Jan 10, 2016 at 9:57 AM, Oleksandr Natalenko
> 
> <oleksandr@natalenko.name> wrote:
> > I use YeAH. But YeAH code wasn't touched between 4.2 and 4.3.
> 
> Oh, interesting. Looks like tcp_yeah_ssthresh() has a bug where its
> intended reduction can be bigger than tp->snd_cwnd, leading to it
> return a zero ssthresh (or even an ssthresh that underflows to ~4
> billion). If tcp_yeah_ssthresh() returns an ssthresh of 0 then PRR
> will try to pull the cwnd down to 0.
> 
> Can you please leave ECN and Yeah enabled and run something like the
> following patch, to verify this conjecture? If the conjecture is
> right, then the tcp_yeah warning should fire but not the new
> tcp_cwnd_reduction() warning:
> 
> -----------
> diff --git a/net/ipv4/tcp_yeah.c b/net/ipv4/tcp_yeah.c
> index 17d3566..ef60cba 100644
> --- a/net/ipv4/tcp_yeah.c
> +++ b/net/ipv4/tcp_yeah.c
> @@ -206,6 +206,7 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
>         const struct tcp_sock *tp = tcp_sk(sk);
>         struct yeah *yeah = inet_csk_ca(sk);
>         u32 reduction;
> +       s32 ssthresh;
> 
>         if (yeah->doing_reno_now < TCP_YEAH_RHO) {
>                 reduction = yeah->lastQ;
> @@ -219,7 +220,9 @@ static u32 tcp_yeah_ssthresh(struct sock *sk)
>         yeah->fast_count = 0;
>         yeah->reno_count = max(yeah->reno_count>>1, 2U);
> 
> -       return tp->snd_cwnd - reduction;
> +       ssthresh = tp->snd_cwnd - reduction;
> +       if (WARN_ON_ONCE(ssthresh <= 0))
> +               ssthresh = 1;
>  }
> 
>  static struct tcp_congestion_ops tcp_yeah __read_mostly = {
> -----------
> 
> If that works, then we may just want a version of this patch without
> the warning.
> 
> Thanks!
> neal

[toc] | [prev] | [next] | [standalone]

#1306598 — Re: [REGRESSION] tcp/ipv4: kernel panic because of (possible) division by zero

On Sun, Jan 10, 2016 at 4:56 PM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> OK, it seems the assumption about YeAH is correct. Here is stacktrace fired by
> WARN_ON_ONCE():
>
> https://gist.github.com/851cedcfca60d6120035
>
> Is there sufficient info for you to prepare upstream patch?

Great. Thanks for running that test! Yes, I think that's enough info.
We sent a proposed upstream patch here:

  http://patchwork.ozlabs.org/patch/566072/
  tcp_yeah: don't set ssthresh below 2

Feel free to add your "Tested-By" to the thread if you have a chance
to test that patch.

Thanks,
neal

[toc] | [prev] | [next] | [standalone]

#1306814

Compiled and booted OK, no issues so far, but I'd prefer to give it a several 
days test.

Thanks!

On понеділок, 11 січня 2016 р. 13:47:08 EET Neal Cardwell wrote:
> Great. Thanks for running that test! Yes, I think that's enough info.
> We sent a proposed upstream patch here:
> 
>   http://patchwork.ozlabs.org/patch/566072/
>   tcp_yeah: don't set ssthresh below 2
> 
> Feel free to add your "Tested-By" to the thread if you have a chance
> to test that patch.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web