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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 14 Apr 2022 12:11:36 -0400 From: Neal Cardwell <ncardwell@...gle.com> To: Pengcheng Yang <yangpc@...gsu.com> Cc: Eric Dumazet <edumazet@...gle.com>, Yuchung Cheng <ycheng@...gle.com>, netdev@...r.kernel.org Subject: Re: [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample On Wed, Apr 13, 2022 at 6:54 AM Pengcheng Yang <yangpc@...gsu.com> wrote: > > If an ACK (s)acks multiple skbs, we favor the information > from the most recently sent skb by choosing the skb with > the highest prior_delivered count. > But prior_delivered may be equal, because tp->delivered only > changes when receiving, which requires further comparison of > skb timestamp. > > Signed-off-by: Pengcheng Yang <yangpc@...gsu.com> > --- > net/ipv4/tcp_rate.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_rate.c b/net/ipv4/tcp_rate.c > index 617b818..ad893ad 100644 > --- a/net/ipv4/tcp_rate.c > +++ b/net/ipv4/tcp_rate.c > @@ -86,7 +86,9 @@ void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb, > return; > > if (!rs->prior_delivered || > - after(scb->tx.delivered, rs->prior_delivered)) { > + after(scb->tx.delivered, rs->prior_delivered) || > + (scb->tx.delivered == rs->prior_delivered && > + tcp_skb_timestamp_us(skb) > tp->first_tx_mstamp)) { > rs->prior_delivered_ce = scb->tx.delivered_ce; > rs->prior_delivered = scb->tx.delivered; > rs->prior_mstamp = scb->tx.delivered_mstamp; > -- Thank you for posting this patch! I agree there is a bug there, and your patch is an improvement. However, I think this patch is not a complete solution, since it does not handle the case where there are multiple skbs with the tcp_skb_timestamp_us() (which can happen if a outgoing buffered TSO/GSO skb is later split into multiple skbs with the same timestamp). RACK has to deal with the same question "which skb was sent first?", and already has a solution in tcp_rack_sent_after(). I suggest we share code between RACK and tcp_rate_skb_delivered() to make this check. This might involve making a copy of tcp_rack_sent_after() in include/net/tcp.h, naming the .h copy to tcp_skb_sent_after(), and reworking this logic to save and use the sequence number and timestamp so that it can use the new tcp_skb_sent_after() helper. After this fix propagates to net-next we could later then change RACK to use the new tcp_skb_sent_after() function, so we have a single helper used in two places. If you want to post a version of this patch that uses some approach like that, IMHO that would be welcome. If you do not have cycles, I am happy to post one when I get a moment. thanks, neal
Powered by blists - more mailing lists