Re: Checksum behaviour of bpf_redirected packets

[Daniel Borkmann <daniel@iogearbox.net>]

On 6/2/20 12:13 PM, Lorenz Bauer wrote:
> On Mon, 1 Jun 2020 at 22:25, Alan Maguire <alan.maguire@oracle.com> 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@iogearbox.net/