netdev - Re: [PATCH net-next 2/2] bnx2x: add RSS capability for GRE traffic

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, 19 Aug 2013 07:57:15 -0700
From:	Jerry Chu <hkchu@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Dmitry Kravkov <dkravkov@...il.com>,
	Dmitry Kravkov <dmitry@...adcom.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Eilon Greenstein <eilong@...adcom.com>,
	Tom Herbert <therbert@...gle.com>,
	Maciej Żenczykowski <maze@...gle.com>,
	Uday Naik <uday@...gle.com>,
	Michael Dalton <mwdalton@...gle.com>
Subject: Re: [PATCH net-next 2/2] bnx2x: add RSS capability for GRE traffic

On Sun, Aug 18, 2013 at 4:44 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Sun, 2013-08-18 at 14:11 -0700, Jerry Chu wrote:
>
>> It can if the csum validation is deferred. But this requires the stack code to
>> aggregate csum from individual pkt header, which can get ugly as one needs
>> to remove the header part from the individual csum first.
>
> You can _not_ aggregate packets _before_ validating their checksum, or
> else you could end up with a correct final checksum while two or more
> segments had buggy checksums. Checksum are already quite weak, please
> do not make them even weaker ;)

Yes I know how weak the 16bit 1's complement inet csum (see my previous email).
I've had the pleasure to debug a number of bugs in the past where 16bit words
were flipped by the buggy h/w hence went undetected by the weak TCP csum.

But I also think it's a tradeoff call. For WAN it's probably a bad
idea but for data
center traffic, pkt corruption is usually rare. If the performance
gain is large enough,
perhaps it justifies this hack (?)

>
> GRO should not throw out any segments, it is not its role.

I did not suggest GRO to throw away any pkt. GRO would just fix the csum
field in the header of the aggregated pkt and passes it on.

>
> If a router receives a bunch of packets, it should forward them
> regardless of the (TCP) checksum being good or not.

It won't affect the forwarding path (i think).

>
> The final receiver has full responsibility for ultimately validate the
> checksums and update various SNMP counters.

When the GRO pkt with the csum field fixed (aggregated) get to
the receiving TCP endpoint, it will be csum validated by the TCP stack
because skb->ip_summed == CHECKSUM_NONE, and the pkt will be
tossed if the csum check fails like non-GRO pkts.

To be clear, this hack is not something I'd be proud of and I doubt the
performance gain will be enough to justify it. This idea grew more out of
desperation for not having csum offload for encap'ed pkts, which I think
is a P0 bug!

Jerry

>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html