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

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

[Marcelo Leitner]

+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>
>

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

[Eelco Chaudron]

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?
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?

> 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>
>>

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

[Marcelo Leitner]

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>
> >>
>

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

[Eelco Chaudron]

On 22 Feb 2022, at 17:02, Marcelo Leitner wrote:

> 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.

I do not know the exact details on the OVS Kernel conntrack part, but assuming it’s based on the same Netfilter infrastructure, any bits will be set only if the connection is tracked.  So the behavior from an OVS perspective will be the same for both.

>>
>>> 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>
>>>>
>>