netdev - Re: [net-next 01/12] net/mlx5e: Add UDP GSO support

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]

Date:   Mon, 2 Jul 2018 10:45:29 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     borisp@...lanox.com
Cc:     Alexander Duyck <alexander.duyck@...il.com>,
        David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>, ogerlitz@...lanox.com,
        yossiku@...lanox.com
Subject: Re: [net-next 01/12] net/mlx5e: Add UDP GSO support

On Mon, Jul 2, 2018 at 9:34 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Mon, Jul 2, 2018 at 1:30 AM Boris Pismenny <borisp@...lanox.com> wrote:
> >
> >
> >
> > On 7/2/2018 4:45 AM, Willem de Bruijn wrote:
> > >>> I've noticed that we could get cleaner code in our driver if we remove
> > >>> these two lines from net/ipv4/udp_offload.c:
> > >>> if (skb_is_gso(segs))
> > >>>                mss *= skb_shinfo(segs)->gso_segs;
> > >>>
> > >>> I think that this is correct in case of GSO_PARTIAL segmentation for the
> > >>> following reasons:
> > >>> 1. After this change the UDP payload field is consistent with the IP
> > >>> header payload length field. Currently, IPv4 length is 1500 and UDP
> > >>> total length is the full unsegmented length.
> > >
> > > How does this simplify the driver? Does it currently have to
> > > change the udph->length field to the mss on the wire, because the
> > > device only splits + replicates the headers + computes the csum?
> >
> > Yes, this is the code I have at the moment.
> >
> > The device's limitation is more subtle than this. It could adjust the
> > length, but then the checksum would be wrong.
>
> I see. We do have to keep in mind other devices. Alexander's ixgbe
> RFC patch does not have this logic, so that device must update the
> field directly.
>
>   https://patchwork.ozlabs.org/patch/908396/

To be clear, I think it's fine to remove these two lines if it does not cause
problems for the ixgbe. It's quite possible that that device sets the udp
length field unconditionally, ignoring the previous value. In which case both
devices will work after this change without additional driver logic.