Re: [PATCH 3.2 085/115] veth: donât modify ip_summed; doing so treats packets with bad checksums as good.

From: Ben Greear
Date: Thu Apr 28 2016 - 09:45:57 EST




On 04/28/2016 03:29 AM, Sabrina Dubroca wrote:
Hello,

2016-04-27, 17:14:44 -0700, Ben Greear wrote:
On 04/27/2016 05:00 PM, Hannes Frederic Sowa wrote:
Hi Ben,

On Wed, Apr 27, 2016, at 20:07, Ben Hutchings wrote:
On Wed, 2016-04-27 at 08:59 -0700, Ben Greear wrote:
On 04/26/2016 04:02 PM, Ben Hutchings wrote:

3.2.80-rc1 review patch. If anyone has any objections, please let me know.
I would be careful about this. It causes regressions when sending
PACKET_SOCKET buffers from user-space to veth devices.

There was a proposed upstream fix for the regression, but it has not gone
into the tree as far as I know.

http://www.spinics.net/lists/netdev/msg370436.html
[...]

OK, I'll drop this for now.

The fall out from not having this patch is in my opinion a bigger
fallout than not having this patch. This patch fixes silent data
corruption vs. the problem Ben Greear is talking about, which might not
be that a common usage.

What do others think?

Bye,
Hannes


This patch from Cong Wang seems to fix the regression for me, I think it should be added and
tested in the main tree, and then apply them to stable as a pair.

http://dmz2.candelatech.com/?p=linux-4.4.dev.y/.git;a=commitdiff;h=8153e983c0e5eba1aafe1fc296248ed2a553f1ac;hp=454b07405d694dad52e7f41af5816eed0190da8a

Actually, no, this is not really a regression.

If you capture packets on a device with checksum offloading enabled,
the TCP/UDP checksum isn't filled. veth also behaves that way. What
the "veth: don't modify ip_summed" patch does is enable proper
checksum validation on veth. This really was a bug in veth.

Cong's patch would also break cases where we choose to inject packets
with invalid checksums, and they would now be accepted as correct.

Your use case is invalid, it just happened to work because of a
bug. If you want the stack to fill checksums so that you want capture
and reinject packets, you have to disable checksum offloading (or
compute the checksum yourself in userspace).

Disabling checksum offloading or computing in user-space (and then
recomputing in veth to verify the checksum?) is a huge performance loss.

Maybe we could add a socket option to enable Cong's patch on a per-socket
basis? That way my use-case can still work and you can have this new
behaviour by default?

Thanks,
Ben


--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com