[ovs-dev] [PATCH v3] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags

[Marcelo Ricardo Leitner]

On Thu, Feb 04, 2021 at 03:34:41PM +0100, Ilya Maximets wrote:
> On 2/4/21 3:50 AM, wenxu at ucloud.cn wrote:
> > From: wenxu <wenxu at ucloud.cn>
> > 
> > TC flower doesn't support some ct state flags such as
> > INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
> > 
> 
> Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")
> 
> > Signed-off-by: wenxu <wenxu at ucloud.cn>
> > ---
> >  lib/netdev-offload-tc.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> This version loogs good to me.
> Marcelo, Paul, what do you think?

+1
Reviewed-by: Marcelo Ricardo Leitner <mleitner at redhat.com>

...
> > @@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >                  flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> >              }
> >              flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> > +            mask->ct_state &= ~OVS_CS_F_TRACKED;
> >          }
> >  
> >          if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
> >              flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >              flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >          }

Btw, this check is probably useless as validate_ct_state() already does:

      if (state & CS_NEW && state & CS_ESTABLISHED) {
          ds_put_format(ds, "%s: invalid connection state: "
                        "\"new\" and \"est\" are mutually exclusive\n",

And it would be wrong if it was effective. The translation shouldn't
be saying which flag has preference.

> > -
> > -        mask->ct_state = 0;
> >      }
> >  
> >      if (mask->ct_zone) {
> > 
>