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. > >> >> As for missing other flags, I suggest to add a check that all flags were >> parsed, the same as we do with >> match's mask, set translated flags mask to 0, and then check if we >> missed some new flag by checking mask the ct_state mask was zeroed. >> >> Like this: >> >> ---- >> >From 48a4cb1b537d3837df8c40c9c265ae95e9729255 Mon Sep 17 00:00:00 2001 >> From: Paul Blakey <[email protected]> >> Date: Wed, 3 Feb 2021 10:27:21 +0200 >> Subject: [PATCH] netdev-offload-tc: Check for unparsed ct_state flags >> >> Don't offload flows where we didn't fully parse ct_state flags. >> >> Signed-off-by: Paul Blakey <[email protected]> >> --- >> lib/netdev-offload-tc.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 08a4735..4362c6e 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -1705,7 +1705,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct >> match *match, >> flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_INVALID; >> } >> >> - mask->ct_state = 0; >> + mask->ct_state &= ~(OVS_CS_F_INVALID | >> + OVS_CS_F_REPLY_DIR | >> + OVS_CS_F_TRACKED | >> + OVS_CS_F_ESTABLISHED | >> + OVS_CS_F_NEW); >> } >> >> if (mask->ct_zone) { >> -- >> >> Does that sound good to you? > > Yes. > >> If so, as standalone patch, or in this patchset (will rebase the above as >> first patch in this series)? > > To ease the maintainers work, the optimal combination is a standalone > patch, to be applied before this patchset (without rpl and inv), is > welcomed (as it can be easily backported to stable branches). And then > a v2 here to extend the validation. We definitely need the fix first and new features after that. But I'm not sure if we can accept this feature right now if TC ignores unknown flags and we have no reliable way to tell if it actually supports them or not. Taking into account what I said above and also my reply to wenxu on v2 of "unsupport" patch (both you patches needs changes), it'll be better if wenxu (original reporter of the issue) will proceed with the "unsupport" patch with in-place disabling of consumed flags. And we can focus on TC in-kernel part and a way to probe for feature support. wenxu, do you want to proceed with your patch and send v3? Best regards, Ilya Maximets. > > Thanks, > Marcelo > >> >> Thanks, >> Paul. >> >>> >>>> >>>> Best regards, Ilya Maximets. >>>> >>>>> >>>>> BR >>>>> wenxu >>>>> >>>>> >>>>> From: Ilya Maximets <[email protected]> >>>>> Date: 2021-02-02 23:33:41 >>>>> To: Paul Blakey <[email protected]>,[email protected] >>>>> Cc: Oz Shlomo <[email protected]>,[email protected],Marcelo Leitner >>>>> <[email protected]>,wenxu <[email protected]> >>>>> Subject: Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl >>>>> and inv flags>On 2/2/21 1:47 PM, Paul Blakey wrote: >>>>>>> Add offload support for ct_state rpl and inv flags. >>>>>> >>>>>> Hi. >>>>>> >>>>>> Since you're working on this, could you, please, review the following >>>>>> patch: >>>>>> >>>>>> http://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ >>>>>> >>>>>> It seems like current OVS just ignores all the flags that can not >>>>>> be passed to TC and this doesn't look right. >>>>>> >>>>>>> >>>>>>> For example: >>>>>>> ovs-ofctl del-flows br-ovs >>>>>>> ovs-ofctl add-flow br-ovs arp,actions=normal >>>>>>> ovs-ofctl add-flow br-ovs "table=0, ip,ct_state=-trk >>>>>>> actions=ct(table=1,zone=5)" >>>>>>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_state=+trk+new >>>>>>> actions=ct(zone=5, commit),normal" >>>>>>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est+rpl >>>>>>> actions=normal" >>>>>>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est-rpl >>>>>>> actions=normal" >>>>>>> >>>>>>> Paul Blakey (2): >>>>>>> compat: Add TCA_FLOWER_KEY_CT_FLAGS_REPLY/INVALID definitions >>>>>>> netdev-offload-tc: Add support for ct_state rpl and inv flags >>>>>>> >>>>>>> acinclude.m4 | 6 +++--- >>>>>>> include/linux/pkt_cls.h | 4 +++- >>>>>>> lib/netdev-offload-tc.c | 28 ++++++++++++++++++++++++++++ >>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-) >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> _______________________________________________ >>> 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
