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. 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. 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. 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. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
