Re: [PATCH] tcp: check socket state before calling WARN_ON

[Eric Dumazet]

On Wed, Dec 4, 2024 at 4:22 AM Youngmin Nam <youngmin.nam@xxxxxxxxxxx> wrote:
>
> On Tue, Dec 03, 2024 at 10:34:46AM -0500, Neal Cardwell wrote:
> > On Tue, Dec 3, 2024 at 6:07 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Dec 3, 2024 at 9:10 AM Youngmin Nam <youngmin.nam@xxxxxxxxxxx> wrote:
> > > >
> > > > We encountered the following WARNINGs
> > > > in tcp_sacktag_write_queue()/tcp_fastretrans_alert()
> > > > which triggered a kernel panic due to panic_on_warn.
> > > >
> > > > case 1.
> > > > ------------[ cut here ]------------
> > > > WARNING: CPU: 4 PID: 453 at net/ipv4/tcp_input.c:2026
> > > > Call trace:
> > > >  tcp_sacktag_write_queue+0xae8/0xb60
> > > >  tcp_ack+0x4ec/0x12b8
> > > >  tcp_rcv_state_process+0x22c/0xd38
> > > >  tcp_v4_do_rcv+0x220/0x300
> > > >  tcp_v4_rcv+0xa5c/0xbb4
> > > >  ip_protocol_deliver_rcu+0x198/0x34c
> > > >  ip_local_deliver_finish+0x94/0xc4
> > > >  ip_local_deliver+0x74/0x10c
> > > >  ip_rcv+0xa0/0x13c
> > > > Kernel panic - not syncing: kernel: panic_on_warn set ...
> > > >
> > > > case 2.
> > > > ------------[ cut here ]------------
> > > > WARNING: CPU: 0 PID: 648 at net/ipv4/tcp_input.c:3004
> > > > Call trace:
> > > >  tcp_fastretrans_alert+0x8ac/0xa74
> > > >  tcp_ack+0x904/0x12b8
> > > >  tcp_rcv_state_process+0x22c/0xd38
> > > >  tcp_v4_do_rcv+0x220/0x300
> > > >  tcp_v4_rcv+0xa5c/0xbb4
> > > >  ip_protocol_deliver_rcu+0x198/0x34c
> > > >  ip_local_deliver_finish+0x94/0xc4
> > > >  ip_local_deliver+0x74/0x10c
> > > >  ip_rcv+0xa0/0x13c
> > > > Kernel panic - not syncing: kernel: panic_on_warn set ...
> > > >
> > >
> > > I have not seen these warnings firing. Neal, have you seen this in the past ?
> >
> > I can't recall seeing these warnings over the past 5 years or so, and
> > (from checking our monitoring) they don't seem to be firing in our
> > fleet recently.
> >
> > > In any case this test on sk_state is too specific.
> >
> > I agree with Eric. IMHO TCP_FIN_WAIT1 deserves all the same warnings
> > as ESTABLISHED, since in this state the connection may still have a
> > big queue of data it is trying to reliably send to the other side,
> > with full loss recovery and congestion control logic.
> Yes I agree with Eric as well.
>
> >
> > I would suggest that instead of running with panic_on_warn it would
> > make more sense to not panic on warning, and instead add more detail
> > to these warning messages in your kernels during your testing, to help
> > debug what is going wrong. I would suggest adding to the warning
> > message:
> >
> > tp->packets_out
> > tp->sacked_out
> > tp->lost_out
> > tp->retrans_out
> > tcp_is_sack(tp)
> > tp->mss_cache
> > inet_csk(sk)->icsk_ca_state
> > inet_csk(sk)->icsk_pmtu_cookie
>
> Hi Neal.
> Thanks for your opinion.
>
> By the way, we enable panic_on_warn by default for stability.
> As you know, panic_on_warn is not applied to a specific subsystem but to the entire kernel.
> We just want to avoid the kernel panic.
>
> So when I see below lwn article, I think we might use pr_warn() instaed of WARN_ON().
> https://lwn.net/Articles/969923/
>
> How do you think of it ?

You want to silence a WARN_ON() because you chose  to make all WARN_ON() fatal.

We want something to be able to fix real bugs, because we really care
of TCP being correct.

We have these discussions all the time.
https://lwn.net/Articles/969923/ is a good summary.

It makes sense for debug kernels (for instance used by syzkaller or
other fuzzers) to panic,
but for production this is a high risk, there is a reason
panic_on_warn is not set by default.

If we use a soft  print there like pr_warn(), no future bug will be caught.

What next : add a new sysctl to panic whenever a pr_warn() is hit by syzkaller ?

Then Android will set this sysctl "because of stability concerns"

> >
> > A hunch would be that this is either firing for (a) non-SACK
> > connections, or (b) after an MTU reduction.
> >
> > In particular, you might try `echo 0 >
> > /proc/sys/net/ipv4/tcp_mtu_probing` and see if that makes the warnings
> > go away.
> >
> > cheers,
> > neal
> >