netdev - Re: [PATCH net-next v2 1/8] udp: fixup csum for GSO receive slow path

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTScQW-jYCHksXk=85Ssa=HWWce7103A=Y69uduNzpfd6cA@mail.gmail.com>
Date:   Mon, 29 Mar 2021 11:24:30 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Alexander Lobakin <alobakin@...me>
Subject: Re: [PATCH net-next v2 1/8] udp: fixup csum for GSO receive slow path

On Mon, Mar 29, 2021 at 11:01 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Mon, 2021-03-29 at 09:52 -0400, Willem de Bruijn wrote:
> > > > That breaks the checksum-and-copy optimization when delivering to
> > > > local sockets. I wonder if that is a regression.
> > >
> > > The conversion to CHECKSUM_UNNECESSARY happens since
> > > commit 573e8fca255a27e3573b51f9b183d62641c47a3d.
> > >
> > > Even the conversion to CHECKSUM_PARTIAL happens independently from this
> > > series, since commit 6f1c0ea133a6e4a193a7b285efe209664caeea43.
> > >
> > > I don't see a regression here ?!?
> >
> > I mean that UDP packets with local destination socket and no tunnels
> > that arrive with CHECKSUM_NONE normally benefit from the
> > checksum-and-copy optimization in recvmsg() when copying to user.
> >
> > If those packets are now checksummed during GRO, that voids that
> > optimization, and the packet payload is now touched twice.
>
> The 'now' part confuses me. Nothing in this patch or this series
> changes the processing of CHECKSUM_NONE UDP packets with no tunnel.

Agreed. I did not mean to imply that this patch changes that. I was
responding to

> > > +       if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
> > > +               skb->csum_valid = 1;
> >
> > Not entirely obvious is that UDP packets arriving on a device with rx
> > checksum offload off, i.e., with CHECKSUM_NONE, are not matched by
> > this test.
> >
> > I assume that such packets are not coalesced by the GRO layer in the
> > first place. But I can't immediately spot the reason for it..

As you point out, such packets will already have had their checksum
verified at this point, so this branch only matches tunneled packets.
That point is just not immediately obvious from the code.

> I do see checksum validation in the GRO engine for CHECKSUM_NONE UDP
> packet prior to this series.
>
> I *think* the checksum-and-copy optimization is lost
> since 573e8fca255a27e3573b51f9b183d62641c47a3d.

Wouldn't this have been introduced with UDP_GRO?