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