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