Reviewed-by: wenxu <we...@ucloud.cn>
BR
wenxu
On 3/23/2021 2:13 AM, Marcelo Ricardo Leitner wrote:
> From: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
>
> Invalid detection works with two distinct moments: act_ct tries to find
> a conntrack entry and set post_ct true, indicating that that was
> attempted. Then, when flow dissector tries to dissect CT info and no
> entry is there, it knows that it was tried and no entry was found, and
> synthesizes/sets
> key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
> TCA_FLOWER_KEY_CT_FLAGS_INVALID;
> mimicing what OVS does.
>
> OVS has this a bit more streamlined, as it recomputes the key after
> trying to find a conntrack entry for it.
>
> Issue here is, when we have 'tc action ct clear', it didn't clear
> post_ct, causing a subsequent match on 'ct_state -trk' to fail, due to
> the above. The fix, thus, is to clear it.
>
> Reproducer rules:
> tc filter add dev enp130s0f0np0_0 ingress prio 1 chain 0 \
> protocol ip flower ip_proto tcp ct_state -trk \
> action ct zone 1 pipe \
> action goto chain 2
> tc filter add dev enp130s0f0np0_0 ingress prio 1 chain 2 \
> protocol ip flower \
> action ct clear pipe \
> action goto chain 4
> tc filter add dev enp130s0f0np0_0 ingress prio 1 chain 4 \
> protocol ip flower ct_state -trk \
> action mirred egress redirect dev enp130s0f1np1_0
>
> With the fix, the 3rd rule matches, like it does with OVS kernel
> datapath.
>
> Fixes: 7baf2429a1a9 ("net/sched: cls_flower add CT_FLAGS_INVALID flag
> support")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> ---
> net/sched/act_ct.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index
> f0a0aa125b00ad9e34725daf0ce4457d2d2ec32c..16e888a9601dd18c7b38c6ae72494d1aa975a37e
> 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -945,13 +945,14 @@ static int tcf_ct_act(struct sk_buff *skb, const struct
> tc_action *a,
> tcf_lastuse_update(&c->tcf_tm);
>
> if (clear) {
> + qdisc_skb_cb(skb)->post_ct = false;
> ct = nf_ct_get(skb, &ctinfo);
> if (ct) {
> nf_conntrack_put(&ct->ct_general);
> nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
> }
>
> - goto out;
> + goto out_clear;
> }
>
> family = tcf_ct_skb_nf_family(skb);
> @@ -1030,8 +1031,9 @@ static int tcf_ct_act(struct sk_buff *skb, const struct
> tc_action *a,
> skb_push_rcsum(skb, nh_ofs);
>
> out:
> - tcf_action_update_bstats(&c->common, skb);
> qdisc_skb_cb(skb)->post_ct = true;
> +out_clear:
> + tcf_action_update_bstats(&c->common, skb);
> if (defrag)
> qdisc_skb_cb(skb)->pkt_len = skb->len;
> return retval;
