On Thu, 4 Feb 2021, Ilya Maximets wrote:
> On 2/4/21 3:42 PM, Marcelo Ricardo Leitner wrote:
> > On Thu, Feb 04, 2021 at 03:34:41PM +0100, Ilya Maximets wrote:
> >> On 2/4/21 3:50 AM, [email protected] wrote:
> >>> From: wenxu <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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 <[email protected]>
>
> Thanks.
> Applied to master and backported down to 2.13.
Sorry for late reply, Looks good (although you already merged :))
>
> >
> > ...
> >>> @@ -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.
> >
>
> IIUC, this part of the code is from validate_ct_state() function, but
> it only used during ofproto/trace. But I agree that we should not
> silently clear one of the flags. Some part of the code should fail
> instead.
>
> >>> -
> >>> - mask->ct_state = 0;
> >>> }
> >>>
> >>> if (mask->ct_zone) {
> >>>
> >>
> >
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
