netdev - Re: [PATCH] tcp: fix TCP socks unreleased in BBR mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoCnsEi8KahgbhrVDawdhsjnAS4X8je0oCE-KZoCyf1Gcg@mail.gmail.com>
Date:   Wed, 3 Jun 2020 21:51:09 +0800
From:   Jason Xing <kerneljasonxing@...il.com>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        liweishi <liweishi@...ishou.com>,
        Shujin Li <lishujin@...ishou.com>
Subject: Re: [PATCH] tcp: fix TCP socks unreleased in BBR mode

On Wed, Jun 3, 2020 at 8:02 PM Neal Cardwell <ncardwell@...gle.com> wrote:
>
> On Wed, Jun 3, 2020 at 1:44 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Tue, Jun 2, 2020 at 10:05 PM Jason Xing <kerneljasonxing@...il.com> wrote:
> > >
> > > Hi Eric,
> > >
> > > I'm still trying to understand what you're saying before. Would this
> > > be better as following:
> > > 1) discard the tcp_internal_pacing() function.
> > > 2) remove where the tcp_internal_pacing() is called in the
> > > __tcp_transmit_skb() function.
> > >
> > > If we do so, we could avoid 'too late to give up pacing'. Meanwhile,
> > > should we introduce the tcp_wstamp_ns socket field as commit
> > > (864e5c090749) does?
> > >
> >
> > Please do not top-post on netdev mailing list.
> >
> >
> > I basically suggested double-checking which point in TCP could end up
> > calling tcp_internal_pacing()
> > while the timer was already armed.
> >
> > I guess this is mtu probing.

I tested the patch Eric suggested and the system display the stack
trace which means there's one more exception we have to take into
consideration. The call trace is listed as following:
 Call Trace:
  <IRQ>
  __tcp_retransmit_skb+0x188/0x7f0
  ? bbr_set_state+0x7f/0x90 [tcp_bbr]
  tcp_retransmit_skb+0x14/0xc0
  tcp_retransmit_timer+0x313/0xa10
  ? native_sched_clock+0x37/0x90
  ? tcp_write_timer_handler+0x210/0x210
  tcp_write_timer_handler+0xb1/0x210
  tcp_write_timer+0x6d/0x80
  call_timer_fn+0x29/0x110
  run_timer_softirq+0x3cb/0x400
  ? native_sched_clock+0x37/0x90
  __do_softirq+0xdf/0x2ed
  irq_exit+0xf7/0x100
  smp_apic_timer_interrupt+0x68/0x120
  apic_timer_interrupt+0xf/0x20
  </IRQ>

I admitted that this case is not that easily triggered, but it is the
one that avoids the check during tcp_mtu_probe() period. The first skb
is sent out without being checked by tcp_pacing_check  when RTO comes.

>
> Perhaps this could also happen from some of the retransmission code
> paths that don't use tcp_xmit_retransmit_queue()? Perhaps
> tcp_retransmit_timer() (RTO) and  tcp_send_loss_probe() TLP? It seems
> they could indirectly cause a call to __tcp_transmit_skb() and thus
> tcp_internal_pacing() without first checking if the pacing timer was
> already armed?
>

Point taken. There are indeed several places using __tcp_transmit_skb
where could cause such an issue, that is to say, slab increasing. All
these particular cases, I think, should all be taken into account.

Thanks,
Jason

> neal