netdev - Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

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

Date:	Mon, 11 Jul 2016 15:21:40 +0200
From:	Paolo Abeni <pabeni@...hat.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jesse Gross <jesse@...nel.org>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum
 packets

On Fri, 2016-07-08 at 16:03 -0500, Tom Herbert wrote:
> On Thu, Jul 7, 2016 at 10:58 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> > currently, UDP packets with zero checksum are not allowed to
> > use udp offload's GRO. This patch admits such packets to
> > GRO, if the related socket settings allow it: ipv6 packets
> > are not admitted if the sockets don't have the no_check6_rx
> > flag set.
> >
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> >  net/ipv4/udp_offload.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 9c37338..ac783f4 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
> >         struct sock *sk;
> >
> >         if (NAPI_GRO_CB(skb)->encap_mark ||
> > -           (skb->ip_summed != CHECKSUM_PARTIAL &&
> > +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
> >              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> >              !NAPI_GRO_CB(skb)->csum_valid))
> 
> Paolo,
> 
> I think you might be misunderstanding the intent of this conditional.
> It is trying to deduce that that the inner TCP checksum has likely
> been validated or can be validated without doing  packet checksum
> calculation. This is trying to avoid doing host side checksum
> calculation in the GRO path and really has little to do with rather
> uh->check is zero or not. The assumption was that we shouldn't compute
> whole packet checksums in the GRO path because of performance. If this
> assumption is no longer valid (i.e. there's good data saying doing
> checksums in GRO path is a benefit) then all the checksum parts of
> this conditional should be removed.

Oh, my bad! I was hit by an ixgbe errata (82599 sometimes marks zero
checksum udp packets with CHECKSUM_NONE), so in my tests the above
condition was matched by 0 csum UDP packets. Than I misread csum_cnt
documentation, assuming it was not incremented for zero checksum UDP
packets: I thought that the matches I saw were due to !uh->check
instead of missing offload.

Thank you for the clarification,

Paolo