On 6/2/20 12:13 PM, Lorenz Bauer wrote:
On Mon, 1 Jun 2020 at 22:25, Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
On Mon, 1 Jun 2020, Daniel Borkmann wrote:
On 6/1/20 7:48 PM, Alan Maguire wrote:
On Wed, 13 May 2020, Lorenz Bauer wrote:
Option 1: always downgrade UNNECESSARY to NONE
- Easiest to back port
- The helper is safe by default
- Performance impact unclear
- No escape hatch for Cilium
Option 2: add a flag to force CHECKSUM_NONE
- New UAPI, can this be backported?
- The helper isn't safe by default, needs documentation
- Escape hatch for Cilium
Option 3: downgrade to CHECKSUM_NONE, add flag to skip this
- New UAPI, can this be backported?
- The helper is safe by default
- Escape hatch for Cilium (though you'd need to detect availability of
the
flag somehow)
This seems most reasonable to me; I can try and cook a proposal for
tomorrow as
potential fix. Even if we add a flag, this is still backportable to stable
(as
long as the overall patch doesn't get too complex and the backport itself
stays
compatible uapi-wise to latest kernels. We've done that before.). I happen
to
have two ixgbe NICs on some of my test machines which seem to be setting
the
CHECKSUM_UNNECESSARY, so I'll run some experiments from over here as well.
Great! I'm happy to test, of course.
I had a go at implementing option 3 as a few colleagues ran into this
problem. They confirmed the fix below resolved the issue. Daniel is
this roughly what you had in mind? I can submit a patch for the bpf
tree if that's acceptable with the new flag. Do we need a few
tests though?
Coded this [0] up last week which Lorenz gave a spin as well. Originally
wanted to
get it out Friday night, but due to internal release stuff it got too late Fri
night
and didn't want to rush it at 3am anymore, so the series as fixes is going out
tomorrow
morning [today was public holiday in CH over here].
Looks great! Although I've only seen this issue arise
for cases where csum_level == 0, should we also
add "skb->csum_level = 0;" when we reset the
ip_summed value?
FWIW I had the same reaction. Maybe it's worth adding after all, Daniel?
Although not needed, but yeah, fair enough. I've added a small skb helper for it.
Series is out here now, ptal [0].
Thanks,
Daniel
[0] https://lore.kernel.org/bpf/cover.1591108731.git.daniel@xxxxxxxxxxxxx/
