diff mbox series

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

Message ID dd268092346925b34d5963debfd6df4410545828.1616436250.git.marcelo.leitner@gmail.com (mailing list archive)
State Accepted
Commit 8ca1b090e5c9a71abeea1dda8757f4ec3811f06e
Delegated to: Netdev Maintainers
Headers show
Series [net] net/sched: act_ct: clear post_ct if doing ct_clear | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers fail 2 blamed authors not CCed: marcelo.leitner@gmail.com kuba@kernel.org; 6 maintainers not CCed: jiri@resnulli.us marcelo.leitner@gmail.com jhs@mojatatu.com xiyou.wangcong@gmail.com davem@davemloft.net kuba@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: Possible repeated word: 'that'
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Marcelo Ricardo Leitner March 22, 2021, 6:13 p.m. UTC 
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(-)

Comments

wenxu March 23, 2021, 7:15 a.m. UTC | #1 
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;
patchwork-bot+netdevbpf@kernel.org March 23, 2021, 9:40 p.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Mon, 22 Mar 2021 15:13:22 -0300 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net/sched: act_ct: clear post_ct if doing ct_clear
    https://git.kernel.org/netdev/net/c/8ca1b090e5c9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

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;