netdev - Re: [PATCH net v2 1/1] net/udp_gso: Allow TX timestamp with UDP GSO

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: <CAF=yD-KN_yxKUkuGe55_OR0ywyxsHNq4Bg0koe6B+Kq5QQLgzQ@mail.gmail.com>
Date:   Thu, 13 Jun 2019 17:50:38 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Fred Klassen <fklassen@...neta.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 1/1] net/udp_gso: Allow TX timestamp with UDP GSO

On Thu, Jun 13, 2019 at 4:58 PM Fred Klassen <fklassen@...neta.com> wrote:
>
> >> It also appears that other TX CMSG types cause similar issues, for
> >> example trying to set SOL_IP/IP_TOS.
> >
> > If correct we need to find the root cause. Without that, this is not
> > very informative. And likely the fix is not related, as that does not
> > involve tx_flags or tskey.
> >
> > What exact behavior do you observe: tx timestamp notifications go
> > missing when enabling IP_TOS?
>
> The IP_TOS observation was a false positive, so removing this comment for v3.
>
> >>
> >> This patch preserves tx_flags for the first UDP GSO segment. This
> >> mirrors the stack's behaviour for IPv4 fragments.
> >
> > But deviates from the established behavior of other transport protocol TCP.
>
> Ack. Removing comment for v3. The noted IPv4 fragment behavior is from at patch
> that is not been accepted yet.
>
> > I think it's a bit premature to resubmit as is while still discussing
> > the original patch.
> >
>
> Understood. Will wait for acceptance before submitting v3.
>
> >> --- a/net/ipv4/udp_offload.c
> >> +++ b/net/ipv4/udp_offload.c
> >> @@ -228,6 +228,10 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> >>        seg = segs;
> >>        uh = udp_hdr(seg);
> >>
> >> +       /* preserve TX timestamp and zero-copy info for first segment */
> >
> > As pointed out in v1, zerocopy flags are handled by protocol
> > independent skb_segment. It calls skb_zerocopy_clone.
> >
>
> Correcting in v3 to read …
> "preserve TX timestamp flags and TS key for first segment”.

Sounds good.

When resubmitting otherwise the same patch, please comment clearly why
this is the preferred solution (because in some ways it is not):

- Timestamping both first and last segmented is not feasible. Hardware
can only have one outstanding TS request at a time.

- Timestamping last segment may under report network latency of the
previous segments. Even though the doorbell is suppressed, the ring
producer counter has been incremented.

I have probably missed a few.

Timestamping the first segment has the downside that it may
underreport tx host network latency. It appears that we have to pick
one or the other. And possibly follow-up with a config flag to choose
behavior..