netdev - Re: [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample

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

Message-ID: <000001d850e8$0c5743f0$2505cbd0$@wangsu.com>
Date:   Sat, 16 Apr 2022 00:44:16 +0800
From:   "Pengcheng Yang" <yangpc@...gsu.com>
To:     "'Neal Cardwell'" <ncardwell@...gle.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 Fri, Apr 15, 2022 at 12:12 AM Neal Cardwell <ncardwell@...gle.com> wrote:
> 
> 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>
> 
> 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).
> 
My initial thought was that this case would not affect the correctness of rate_sample,
since the timestamp of these skbs are the same.
So I am confused whether we have to find the *real* most recently (with the highest seq) skb
(at the cost of a little extra cost)?

> 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.
> 

Ok. I will send the V2 later according to your suggestion. Thanks neal.

> 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