netdev - Re: [PATCH net] tcp: inherit timestamp on mtu probe

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: <CA+FuTSfK=xSMJvVNJB7DKdqwG_FAi2gLjbCvkXVqF99n71rRdg@mail.gmail.com>
Date:   Tue, 27 Aug 2019 16:53:22 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net] tcp: inherit timestamp on mtu probe

On Tue, Aug 27, 2019 at 4:07 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, Aug 27, 2019 at 9:09 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > TCP associates tx timestamp requests with a byte in the bytestream.
> > If merging skbs in tcp_mtu_probe, migrate the tstamp request.
> >
> > Similar to MSG_EOR, do not allow moving a timestamp from any segment
> > in the probe but the last. This to avoid merging multiple timestamps.
> >
> > Tested with the packetdrill script at
> > https://github.com/wdebruij/packetdrill/commits/mtu_probe-1
> >
> > Link: http://patchwork.ozlabs.org/patch/1143278/#2232897
> > Fixes: 4ed2d765dfac ("net-timestamp: TCP timestamping")
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > ---
> >  net/ipv4/tcp_output.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 5c46bc4c7e8d..42abc9bd687a 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -2053,7 +2053,7 @@ static bool tcp_can_coalesce_send_queue_head(struct sock *sk, int len)
> >                 if (len <= skb->len)
> >                         break;
> >
> > -               if (unlikely(TCP_SKB_CB(skb)->eor))
> > +               if (unlikely(TCP_SKB_CB(skb)->eor) || tcp_has_tx_tstamp(skb))
> >                         return false;
> >
> >                 len -= skb->len;
> > @@ -2170,6 +2170,7 @@ static int tcp_mtu_probe(struct sock *sk)
> >                          * we need to propagate it to the new skb.
> >                          */
> >                         TCP_SKB_CB(nskb)->eor = TCP_SKB_CB(skb)->eor;
> > +                       tcp_skb_collapse_tstamp(nskb, skb);
>
> nit: maybe rename tcp_skb_collapse_tstamp() to tcp_skb_tstamp_copy()
> or something ?
>
> Its name came from the fact that it was only used from
> tcp_collapse_retrans(), but it will no
> longer be the case after your fix.

Sure, that's more descriptive.

One caveat, the function is exposed in a header, so it's a
bit more churn. If you don't mind that, I'll send the v2.