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

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

[Oleksandr Natalenko]

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?

Regards,
  Oleksandr.

[1] http://i.piccy.info/
i9/6f5cb187c4ff282d189f78c63f95af43/1450729403/283985/951663/panic.jpg

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

[Yuchung Cheng]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

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

[-- Attachment #2: 0001-tcp-debug-tcp_cwnd_reduction-div0.patch --]
[-- Type: application/octet-stream, Size: 1350 bytes --]

From 00c80bddae93d712d1e51e021266c0067d2e130e Mon Sep 17 00:00:00 2001
From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 21 Dec 2015 17:37:51 -0800
Subject: [PATCH] tcp: debug tcp_cwnd_reduction div0

It's been reported tcp_cwnd_reduction() may have div0 bug b/c
tp->prior_cwnd == 0. To avoid this, the best option is to simply
use packet conservation to send packets in cwnd reduction states.
Also add some pr_err to debug this.

Signed-off-by: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_input.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7b1fddc..c27fa84 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2478,8 +2478,12 @@ static void tcp_cwnd_reduction(struct sock *sk, const int prior_unsacked,
 	int newly_acked_sacked = prior_unsacked -
 				 (tp->packets_out - tp->sacked_out);
 
+	if (WARN_ON(!tp->prior_cwnd))
+		pr_err(" cwr prior_cwnd0 %d cwnd %u %d %d 0x%x\n",
+			inet_csk(sk)->icsk_ca_state, tp->snd_cwnd,
+			tp->prr_out, tp->prr_delivered, flag);
 	tp->prr_delivered += newly_acked_sacked;
-	if (delta < 0) {
+	if (delta < 0 && tp->prior_cwnd) {
 		u64 dividend = (u64)tp->snd_ssthresh * tp->prr_delivered +
 			       tp->prior_cwnd - 1;
 		sndcnt = div_u64(dividend, tp->prior_cwnd) - tp->prr_out;
-- 
2.6.0.rc2.230.g3dd15c0

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

[Oleksandr Natalenko]

That is correct, I have net.ipv4.tcp_ecn set to 1.

I've recompiled the kernel with proposed patch, now still waiting for issue to 
be triggered.

Could I manually simulate the erroneous TCP ECN behavior to speed up the 
debugging?

On понеділок, 21 грудня 2015 р. 18:10:32 EET Yuchung Cheng 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

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

[Oleksandr Natalenko]

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.

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

[Yuchung Cheng]

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.

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

[Yuchung Cheng]

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.

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

[Oleksandr Natalenko]

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.

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

[Oleksandr Natalenko]

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.

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

[Oleksandr Natalenko]

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.

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

[Neal Cardwell]

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

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

[Neal Cardwell]

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

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

[Oleksandr Natalenko]

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

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

[Oleksandr Natalenko]

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

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

[Neal Cardwell]

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

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

[Oleksandr Natalenko]

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

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

[Neal Cardwell]

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

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

[Oleksandr Natalenko]

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

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

[Neal Cardwell]

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

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

[Oleksandr Natalenko]

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.