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
| ||
|
Message-ID: <CADVnQynXC=sEiYOcbSJBv2SML8gzooK_xitXt5uOqybTxj-VtQ@mail.gmail.com> Date: Wed, 20 Jul 2022 14:49:35 -0400 From: Neal Cardwell <ncardwell@...gle.com> To: LemmyHuang <hlm3280@....com> Cc: edumazet@...gle.com, davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Wei Wang <weiwan@...gle.com>, Yuchung Cheng <ycheng@...gle.com>, Soheil Hassas Yeganeh <soheil@...gle.com> Subject: Re: [PATCH net-next v2] tcp: fix condition for increasing pingpong count On Wed, Jul 20, 2022 at 3:25 AM LemmyHuang <hlm3280@....com> wrote: > > When CONFIG_HZ defaults to 1000Hz and the network transmission time is > less than 1ms, lsndtime and lrcvtime are likely to be equal, which will > lead to hundreds of interactions before entering pingpong mode. > > Fixes: 4a41f453bedf ("tcp: change pingpong threshold to 3") > Suggested-by: Jakub Kicinski <kuba@...nel.org> > Signed-off-by: LemmyHuang <hlm3280@....com> > --- > v2: > * Use !after() wrapping the values. (Jakub Kicinski) > > v1: https://lore.kernel.org/netdev/20220719130136.11907-1-hlm3280@163.com/ > --- > net/ipv4/tcp_output.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 858a15cc2..c1c95dc40 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -172,7 +172,7 @@ static void tcp_event_data_sent(struct tcp_sock *tp, > * and it is a reply for ato after last received packet, > * increase pingpong count. > */ > - if (before(tp->lsndtime, icsk->icsk_ack.lrcvtime) && > + if (!after(tp->lsndtime, icsk->icsk_ack.lrcvtime) && > (u32)(now - icsk->icsk_ack.lrcvtime) < icsk->icsk_ack.ato) > inet_csk_inc_pingpong_cnt(sk); > > -- Thanks for pointing out this problem! AFAICT this patch would result in incorrect behavior. With this patch, we could have cases where tp->lsndtime == icsk->icsk_ack.lrcvtime and (u32)(now - icsk->icsk_ack.lrcvtime) < icsk->icsk_ack.ato and yet we do not really have a ping-pong exchange. For example, with this patch we could have: T1: jiffies=J1; host B receives RPC request from host A T2: jiffies=J1; host B sends first RPC response data packet to host A; -> calls inet_csk_inc_pingpong_cnt() T3: jiffies=J1; host B sends second RPC response data packet to host A; -> calls inet_csk_inc_pingpong_cnt() In this scenario there is only one ping-pong exchange but the code calls inet_csk_inc_pingpong_cnt() twice. So I'm hoping we can come up with a better fix. A simpler approach might be to simplify the model and go back to having a single ping-pong interaction cause delayed ACKs to be enabled on a connection endpoint. Our team has been seeing good results for a while with the simpler approach. What do folks think? neal
Powered by blists - more mailing lists