[PATCH v2 net-next ] net/sched: cls_flower add CT_FLAGS_INVALID flag support

From: wenxu <redacted>

This patch add the TCA_FLOWER_KEY_CT_FLAGS_INVALID flag to
match the ct_state with invalid for conntrack.

Signed-off-by: wenxu <redacted>
---
v2:  initialize post_ct right on the declaration

 include/linux/skbuff.h       |  4 ++--
 include/net/sch_generic.h    |  1 +
 include/uapi/linux/pkt_cls.h |  1 +
 net/core/dev.c               |  2 ++
 net/core/flow_dissector.c    | 13 +++++++++----
 net/sched/act_ct.c           |  1 +
 net/sched/cls_flower.c       |  4 +++-
 7 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c9568cf..e22ccf0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -1353,8 +1353,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 skb_flow_dissect_ct(const struct sk_buff *skb,
 		    struct flow_dissector *flow_dissector,
 		    void *target_container,
-		    u16 *ctinfo_map,
-		    size_t mapsize);
+		    u16 *ctinfo_map, size_t mapsize,
+		    bool post_ct);
 void
 skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 			     struct flow_dissector *flow_dissector,

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 639e465..e7bee99 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h

@@ -388,6 +388,7 @@ struct qdisc_skb_cb {
 #define QDISC_CB_PRIV_LEN 20
 	unsigned char		data[QDISC_CB_PRIV_LEN];
 	u16			mru;
+	bool			post_ct;
 };
 
 typedef void tcf_chain_head_change_t(struct tcf_proto *tp_head, void *priv);

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index ee95f42..709668e 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -591,6 +591,7 @@ enum {
 	TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED = 1 << 1, /* Part of an existing connection. */
 	TCA_FLOWER_KEY_CT_FLAGS_RELATED = 1 << 2, /* Related to an established connection. */
 	TCA_FLOWER_KEY_CT_FLAGS_TRACKED = 1 << 3, /* Conntrack has occurred. */
+	TCA_FLOWER_KEY_CT_FLAGS_INVALID = 1 << 4, /* Conntrack is invalid. */
 };
 
 enum {

diff --git a/net/core/dev.c b/net/core/dev.c
index bae35c1..9dce3f7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3878,6 +3878,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
 	qdisc_skb_cb(skb)->mru = 0;
+	qdisc_skb_cb(skb)->post_ct = false;
 	mini_qdisc_bstats_cpu_update(miniq, skb);
 
 	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {

@@ -4960,6 +4961,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h)
 
 	qdisc_skb_cb(skb)->pkt_len = skb->len;
 	qdisc_skb_cb(skb)->mru = 0;
+	qdisc_skb_cb(skb)->post_ct = false;
 	skb->tc_at_ingress = 1;
 	mini_qdisc_bstats_cpu_update(miniq, skb);
 

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2d70ded..c565c7a 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c

@@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 void
 skb_flow_dissect_ct(const struct sk_buff *skb,
 		    struct flow_dissector *flow_dissector,
-		    void *target_container,
-		    u16 *ctinfo_map,
-		    size_t mapsize)
+		    void *target_container, u16 *ctinfo_map,
+		    size_t mapsize, bool post_ct)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 	struct flow_dissector_key_ct *key;

@@ -251,13 +250,19 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 		return;
 
 	ct = nf_ct_get(skb, &ctinfo);
-	if (!ct)
+	if (!ct && !post_ct)
 		return;
 
 	key = skb_flow_dissector_target(flow_dissector,
 					FLOW_DISSECTOR_KEY_CT,
 					target_container);
 
+	if (!ct) {
+		key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
+				TCA_FLOWER_KEY_CT_FLAGS_INVALID;
+		return;
+	}
+
 	if (ctinfo < mapsize)
 		key->ct_state = ctinfo_map[ctinfo];
 #if IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 83a5c67..b344207 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c

@@ -1030,6 +1030,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 
 out:
 	tcf_action_update_bstats(&c->common, skb);
+	qdisc_skb_cb(skb)->post_ct = true;
 	if (defrag)
 		qdisc_skb_cb(skb)->pkt_len = skb->len;
 	return retval;

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 1319986..0dcb5a0 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c

@@ -302,6 +302,7 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 		       struct tcf_result *res)
 {
 	struct cls_fl_head *head = rcu_dereference_bh(tp->root);
+	bool post_ct = qdisc_skb_cb(skb)->post_ct;
 	struct fl_flow_key skb_key;
 	struct fl_flow_mask *mask;
 	struct cls_fl_filter *f;

@@ -318,7 +319,8 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 		skb_flow_dissect_tunnel_info(skb, &mask->dissector, &skb_key);
 		skb_flow_dissect_ct(skb, &mask->dissector, &skb_key,
 				    fl_ct_info_to_flower_map,
-				    ARRAY_SIZE(fl_ct_info_to_flower_map));
+				    ARRAY_SIZE(fl_ct_info_to_flower_map),
+				    post_ct);
 		skb_flow_dissect_hash(skb, &mask->dissector, &skb_key);
 		skb_flow_dissect(skb, &mask->dissector, &skb_key, 0);
 

-- 
1.8.3.1

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

The patch looks good to me, just some side comments below.

On Tue, Jan 19, 2021 at 04:31:50PM +0800, wenxu@ucloud.cn wrote:

I don't know why, but your patches often have function names here that
are not accurate. 

Here as well.

Here, I would expect to see a label/function name just before the
skb_flow_dissect_ct definition. But that's
skb_flow_dissect_set_enc_addr_type. skb_flow_dissect_meta is still one
other function up.

 void
 skb_flow_dissect_ct(const struct sk_buff *skb,
 		    struct flow_dissector *flow_dissector,
-		    void *target_container,
-		    u16 *ctinfo_map,
-		    size_t mapsize)
+		    void *target_container, u16 *ctinfo_map,
+		    size_t mapsize, bool post_ct)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 	struct flow_dissector_key_ct *key;

On Wed, Jan 20, 2021 at 3:40 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote:

quoted

On Tue, Jan 19, 2021 at 12:33 AM [off-list ref] wrote:

quoted

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2d70ded..c565c7a 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c

@@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 void
 skb_flow_dissect_ct(const struct sk_buff *skb,
                    struct flow_dissector *flow_dissector,
-                   void *target_container,
-                   u16 *ctinfo_map,
-                   size_t mapsize)
+                   void *target_container, u16 *ctinfo_map,
+                   size_t mapsize, bool post_ct)

Why do you pass this boolean as a parameter when you
can just read it from qdisc_skb_cb(skb)?

In this case, yes, but this way skb_flow_dissect_ct() can/is able to
not care about what the ->cb actually is. It could be called from
somewhere else too.

This sounds reasonable, it is in net/core/ directory anyway,
so should be independent of tc even though cls_flower is its
only caller.

Thanks.

On Tue, Jan 19, 2021 at 12:33 AM [off-list ref] wrote:

Why do you pass this boolean as a parameter when you
can just read it from qdisc_skb_cb(skb)?

Thanks.

On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote:

On Tue, Jan 19, 2021 at 12:33 AM [off-list ref] wrote:

quoted

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2d70ded..c565c7a 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c

@@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 void
 skb_flow_dissect_ct(const struct sk_buff *skb,
                    struct flow_dissector *flow_dissector,
-                   void *target_container,
-                   u16 *ctinfo_map,
-                   size_t mapsize)
+                   void *target_container, u16 *ctinfo_map,
+                   size_t mapsize, bool post_ct)

Why do you pass this boolean as a parameter when you
can just read it from qdisc_skb_cb(skb)?

In this case, yes, but this way skb_flow_dissect_ct() can/is able to
not care about what the ->cb actually is. It could be called from
somewhere else too.
That's my rationale on it, not sure if wenxu thought the same.

Thanks,
Marcelo

On 1/21/2021 9:09 AM, Cong Wang wrote:

On Wed, Jan 20, 2021 at 3:40 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote:

quoted

On Tue, Jan 19, 2021 at 12:33 AM [off-list ref] wrote:

quoted

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2d70ded..c565c7a 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c

@@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 void
 skb_flow_dissect_ct(const struct sk_buff *skb,
                    struct flow_dissector *flow_dissector,
-                   void *target_container,
-                   u16 *ctinfo_map,
-                   size_t mapsize)
+                   void *target_container, u16 *ctinfo_map,
+                   size_t mapsize, bool post_ct)

Why do you pass this boolean as a parameter when you
can just read it from qdisc_skb_cb(skb)?

In this case, yes, but this way skb_flow_dissect_ct() can/is able to
not care about what the ->cb actually is. It could be called from
somewhere else too.

This sounds reasonable, it is in net/core/ directory anyway,
so should be independent of tc even though cls_flower is its
only caller.

yes. This is the same what I think.

Thanks.

Hello:

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

On Tue, 19 Jan 2021 16:31:50 +0800 you wrote:

From: wenxu <redacted>

This patch add the TCA_FLOWER_KEY_CT_FLAGS_INVALID flag to
match the ct_state with invalid for conntrack.

Signed-off-by: wenxu <redacted>

[...]

Here is the summary with links:
  - [v2,net-next] net/sched: cls_flower add CT_FLAGS_INVALID flag support
    https://git.kernel.org/netdev/net-next/c/7baf2429a1a9

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