On Tue, Feb 22, 2022 at 04:44:30PM +0100, Eelco Chaudron wrote: > > > On 22 Feb 2022, at 12:36, Marcelo Leitner wrote: > > > +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. > > Maybe it was too late last night, but why also untracked ones?
Nah, it was too early today here. :D > It should only match untracked ones with the est flag set, or do I miss > something? > Untracked ones can never have the est flag set, right? My bad. Please scratch that description. Right.. it can't match untracked ones because it's checking that est is set, and untracked ones can never have it. Yet, the point is, the mask, per say, is not wrong. All conntrack entries that have +est will also have +trk, okay, but a user filtering only for +est is not wrong and tc shouldn't reject it. I couldn't find a similar verification in ovs kernel. Maybe I missed it. But if vswitchd would need the tweak in here, seems ovs kernel doesn't need it, and then the two would potentially have different behaviours. > > > 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> > >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
