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