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

[Paul Blakey]

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
> 
> 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
>