netdev - Re: [ovs-dev] [PATCH v2 08/10] netdev-offload-tc: Check for none offloadable ct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Feb 2022 03:36:05 -0800
From:   Marcelo Leitner <mleitner@...hat.com>
To:     Eelco Chaudron <echaudro@...hat.com>
Cc:     Ilya Maximets <i.maximets@....org>, dev@...nvswitch.org,
        Roi Dayan <roid@...dia.com>, Paul Blakey <paulb@...dia.com>,
        wenxu <wenxu@...oud.cn>, netdev@...r.kernel.org
Subject: Re: [ovs-dev] [PATCH v2 08/10] netdev-offload-tc: Check for none
 offloadable ct_state flag combination

+Cc Wenxu, Paul and netdev

On Tue, Feb 22, 2022 at 10:33:44AM +0100, Eelco Chaudron wrote:
>
>
> On 21 Feb 2022, at 15:53, Eelco Chaudron wrote:
>
> > On 21 Feb 2022, at 14:33, Marcelo Leitner wrote:
>
> <SNIP>
>
> >>>> Don’t think this is true, it will only print if +trk and any other flags are set.
> >>>> Guess this is where the miscommunication is.>
> >>>>> The message also seems to be a bit aggressive, especially since it will
> >>>>> almost always be printed.
> >>>
> >>> Yeah.  I missed the fact that you're checking for zero and flower->key.ct_state
> >>> will actually mark existence of other flags.  So, that is fine.
> >>>
> >>> However, I'm still not sure that the condition is fully correct.
> >>>
> >>> If we'll take a match on '+est' with all other flags wildcarded, that will
> >>> trigger the condition, because 'flower->key.ct_state' will contain the 'est' bit,
> >>> but 'trk' bit will not be set.  The point is that even though -trk+est is not
> >>
> >> Oh ow. tc flower will reject this combination today, btw. I don't know
> >> about hw implications for changing that by now.
> >>
> >> https://elixir.bootlin.com/linux/latest/C/ident/fl_validate_ct_state
> >> 'state' parameter in there is the value masked already.
> >>
> >> We directly mapped openflow restrictions to the datapath.
> >>
> >>> a valid combination and +trk+est is, OVS may in theory produce the match with
> >>> 'est' bit set and 'trk' bit wildcarded.  And that can be a correct configuration.
> >>
> >> I guess that means that the only possible parameter validation on
> >> ct_state at tc level is about its length. Thoughts?
> >>
> >
> > Guess I get it now also :) I was missing the wildcard bit that OVS implies when not specifying any :)
> >
> > I think I can fix this by just adding +trk on the TC side when we get the OVS wildcard for +trk. Guess this holds true as for TC there is no -trk +flags.
> >
> > I’m trying to replicate patch 9 all afternoon, and due to the fact I did not write down which test was causing the problem, and it taking 20-30 runs, it has not happened yet :( But will do it later tomorrow, see if it works in all cases ;)
> >
>
> So I’ve been doing some experiments (and running all system-traffic tests), and I think the following fix will solve the problem by just making sure the +trk flag is set in this case on the TC side.
> This will not change the behavior compared to the kernel.
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 0105d883f..3d2c1d844 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1541,6 +1541,12 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match)
>              flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>              flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>          }
> +
> +        if (flower->key.ct_state &&
> +            !(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) {
> +            flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> +            flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
> +        }

I had meant to update the kernel instead. As Ilya was saying, as this
is dealing with masks, the validation that tc is doing is not right. I
mean, for a connection to be in +est, it needs +trk, right, but for
matching, one could have the following value/mask:
 value=est
 mask=est
which means: match connections in Established AND also untracked ones.

Apparently this is what the test is triggering here, and the patch
above could lead to not match the 2nd part of the AND above.

When fixing the parameter validation in flower, we went too far.

  Marcelo

>      }
>
> I will send out a v3 of this set soon with this change included.
>
> //Eelco
>
> <SNIP>
>