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

Wed Feb 3 11:58:17 UTC 2021

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 <paulb at nvidia.com>
> 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 <paulb at nvidia.com>
> ---
>  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 <i.maximets at ovn.org>
> > > > Date: 2021-02-02 23:33:41
> > > > To:  Paul Blakey <paulb at nvidia.com>,dev at openvswitch.org
> > > > Cc:  Oz Shlomo <ozsh at nvidia.com>,i.maximets at ovn.org,Marcelo Leitner <mleitner at redhat.com>,wenxu <wenxu at ucloud.cn>
> > > > 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/1610344573-15780-1-git-send-email-wenxu@ucloud.cn/
> > > >>
> > > >>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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > 
> 