Re: [PATCH net] net/sched: act_ct: clear post_ct if doing ct_clear

[wenxu <wenxu@ucloud.cn>]

Reviewed-by: wenxu <wenxu@ucloud.cn>


BR

wenxu

On 3/23/2021 2:13 AM, Marcelo Ricardo Leitner wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@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.leitner@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;