[ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl and inv flags

[Ilya Maximets]

On 2/4/21 9:31 AM, Paul Blakey wrote:
> 
> 
> On Wed, 3 Feb 2021, Ilya Maximets wrote:
> 
>> On 2/3/21 1:39 PM, Ilya Maximets wrote:
>>> On 2/3/21 12:58 PM, Marcelo Leitner wrote:
>>>> On Wed, Feb 03, 2021 at 10:54:19AM +0200, Paul Blakey wrote:
>>>>>
>>>>>
>>>>> On Tue, 2 Feb 2021, Marcelo Leitner wrote:
>>>>>
>>>>>> On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote:
>>>>>>> On 2/2/21 4:52 PM, wenxu wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> just ingore my patch. Now kernel can support match invalid
>>>>>>>> ct_state in th tc flower.
>>>>>>>
>>>>>>> I don't think that we can ignore it.  At least we need this
>>>>>>> fix on stable branches.  And also there are other conntrack
>>>>>>> flags that are simply ignored, not only inv and rpl.  These
>>>>>>> are snat, dnat, rel, probably some other.  So, we need this
>>>>>>> change in some form anyway.
>>>>>>
>>>>>> That would be great, indeed.
>>>>>>
>>>>>> The issue is also present in tc layer, btw, but maybe Paul has his on
>>>>>> the charts already. The effect here could be that if you use a newer
>>>>>> ovs that supports inv ct_state, for example, up to wenxu's patch the
>>>>>> kernel will simply ignore it.
>>>>>>
>>>>>> Best regards,
>>>>>> Marcelo
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Regarding the inv flag, yes if you offload a +inv rule, and not have
>>>>> an updated kernel then tc rule won't match it and it will fall back to 
>>>>> software. Drivers should see this flag and reject offload, same with rpl.
>>>>
>>>> To be more precise, it will fallback to vswitchd upcall, because the
>>>> tc rule will be accepted and a ovs kernel can't be added then.
>>>>
>>>> flower is using a u16 without a validation of known bits. But if
>>>> unknown bits are used, flow dissector won't populate them, and such
>>>> packet will "slip through" (it should have matched, but not).
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L689
>>>> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L1398
>>>>
>>>>>
>>>>> We can probe for support by adding a ct_state rule with +inv,+rpl and see
>>>>> the echo back. Add that for V2?
>>>>
>>>> With the above, I don't think that this (or probing in general in this
>>>> case) is feasible. My understanding is that flower will just echo back
>>>> the unknown bits, due to the above.
>>>
>>> IIUC, TC doesn't understand some flags and never reports any errors to
>>> the userspace about that.  In this case we should not try to offload
>>> these flags to TC or we will have broken logic in datapath flows.
>>>
>>> I think, TC must return EINVAL or EOPNOTSUPP or something else in case
>>> of unknown flags, so OVS will be able to put the correct flow to
>>> openvswitch kernel module instead and avoid incorrect packet matches.
>>>
>>> Since current TC code doesn't do that, this needs to be implemented in
>>> TC first.  After that we can implement probing on the userspace side,
>>> e.g. try to put certainly non-existent flags and check if TC supports
>>> checking for invalid flags.  If it supports that, we can after that check
>>> for all real flags that might be not supported by current kernel.
>>>
>>> Until then, I think, we can not reliably support inv and rpl flags on
>>> the OVS side.
>>
>> After a small discussion with Marcelo, summarizing the steps that are
>> needed in order to support offloading of inv/rpl/etc. in OVS:
>>
>> 1. Finish work on "unsupport" patch (wenxu).  Apply it and backport
>>    to all stable branches of OVS.
> 
> Here you mean wenxu doing the single reject of +inv, or rejecting if any
> of current or future ct_state flags didn't translate, as we do with
> the flow's mask (set each parsed attr/flag to 0, and then check mask for
> 0).
> B

Yep, rejecting of all current or future ct_state flags that are not
explicitly consumed by netdev-offload-tc.  As in v3 of wenxu's patch:
  http://patchwork.ozlabs.org/project/openvswitch/patch/1612407014-25237-1-git-send-email-wenxu@ucloud.cn/

>>
>> 2. Fix TC in kernel, so it will reject unknown bits in ct_state,
>>    e.g. by returning EINVAL.  This will allow OVS to fall back to
>>    install flow into the kernel module instead of TC.
>>
> 
> I'll send this upstream.
> 
>> 3. Add probing mechanism to detect if kernel fixed or buggy.
>>    e.g. try to send UINT16_MAX as ct_state and check that kernel
>>    returns EINVAL.
> 
> Good idea, will do.
> 
>>
>> 4. Implement inv/rpl/etc. support for kernels that passes probing.
>>    OVS will not support these flags on buggy kernels even if
>>    underlying kernel supports them, because there is no way to tell
>>    if it actually supports them or just ignores.
>>
>> That is how I see that.  Suggestions on how to do that differently
>> are welcome.
> 
> Sounds good.
> 
>>
>> Best regards, Ilya Maximets.
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>