diff mbox series

[net] net/sched: act_ct: Always fill offloading tuple iifidx

Message ID 20231103151410.764271-1-vladbu@nvidia.com (mailing list archive)
State Accepted
Commit 9bc64bd0cd765f696fcd40fc98909b1f7c73b2ba
Delegated to: Netdev Maintainers
Headers show
Series [net] net/sched: act_ct: Always fill offloading tuple iifidx | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1312 this patch: 1312
netdev/cc_maintainers fail 1 blamed authors not CCed: ozsh@nvidia.com; 8 maintainers not CCed: fw@strlen.de netfilter-devel@vger.kernel.org coreteam@netfilter.org kadlec@netfilter.org dev@openvswitch.org ozsh@nvidia.com edumazet@google.com pshelar@ovn.org
netdev/build_clang success Errors and warnings before: 1340 this patch: 1340
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1340 this patch: 1340
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vlad Buslov Nov. 3, 2023, 3:14 p.m. UTC 
Referenced commit doesn't always set iifidx when offloading the flow to
hardware. Fix the following cases:

- nf_conn_act_ct_ext_fill() is called before extension is created with
nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
unspecified iifidx when connection is offloaded after only single
original-direction packet has been processed by tc data path. Always fill
the new nf_conn_act_ct_ext instance after creating it in
nf_conn_act_ct_ext_add().

- Offloading of unidirectional UDP NEW connections is now supported, but ct
flow iifidx field is not updated when connection is promoted to
bidirectional which can result reply-direction iifidx to be zero when
refreshing the connection. Fill in the extension and update flow iifidx
before calling flow_offload_refresh().

Fixes: 9795ded7f924 ("net/sched: act_ct: Fill offloading tuple iifidx")
Reviewed-by: Paul Blakey <paulb@nvidia.com>
Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
---
 include/net/netfilter/nf_conntrack_act_ct.h | 34 ++++++++++++---------
 net/openvswitch/conntrack.c                 |  2 +-
 net/sched/act_ct.c                          | 15 ++++++++-
 3 files changed, 34 insertions(+), 17 deletions(-)

Comments

Simon Horman Nov. 7, 2023, 4:27 p.m. UTC | #1 
On Fri, Nov 03, 2023 at 04:14:10PM +0100, Vlad Buslov wrote:
> Referenced commit doesn't always set iifidx when offloading the flow to
> hardware. Fix the following cases:
> 
> - nf_conn_act_ct_ext_fill() is called before extension is created with
> nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
> unspecified iifidx when connection is offloaded after only single
> original-direction packet has been processed by tc data path. Always fill
> the new nf_conn_act_ct_ext instance after creating it in
> nf_conn_act_ct_ext_add().
> 
> - Offloading of unidirectional UDP NEW connections is now supported, but ct
> flow iifidx field is not updated when connection is promoted to
> bidirectional which can result reply-direction iifidx to be zero when
> refreshing the connection. Fill in the extension and update flow iifidx
> before calling flow_offload_refresh().

Hi Vlad,

these changes look good to me. However, I do wonder if the changes for each
of the two points above should be split into two patches, and
if the fixes tag for the second point should be.

Fixes: 6a9bad0069cf ("net/sched: act_ct: offload UDP NEW connections")

> Fixes: 9795ded7f924 ("net/sched: act_ct: Fill offloading tuple iifidx")
> Reviewed-by: Paul Blakey <paulb@nvidia.com>
> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>

...
Vlad Buslov Nov. 7, 2023, 4:30 p.m. UTC | #2 
On Tue 07 Nov 2023 at 11:27, Simon Horman <horms@kernel.org> wrote:
> On Fri, Nov 03, 2023 at 04:14:10PM +0100, Vlad Buslov wrote:
>> Referenced commit doesn't always set iifidx when offloading the flow to
>> hardware. Fix the following cases:
>> 
>> - nf_conn_act_ct_ext_fill() is called before extension is created with
>> nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
>> unspecified iifidx when connection is offloaded after only single
>> original-direction packet has been processed by tc data path. Always fill
>> the new nf_conn_act_ct_ext instance after creating it in
>> nf_conn_act_ct_ext_add().
>> 
>> - Offloading of unidirectional UDP NEW connections is now supported, but ct
>> flow iifidx field is not updated when connection is promoted to
>> bidirectional which can result reply-direction iifidx to be zero when
>> refreshing the connection. Fill in the extension and update flow iifidx
>> before calling flow_offload_refresh().
>
> Hi Vlad,
>
> these changes look good to me. However, I do wonder if the changes for each
> of the two points above should be split into two patches, and
> if the fixes tag for the second point should be.
>
> Fixes: 6a9bad0069cf ("net/sched: act_ct: offload UDP NEW connections")

Hi Simon,

I considered this but decided to send as single patch because
connections 'refresh' mechanism has already existed before the UDP NEW
offload and it didn't update the iifidx. While yes, it wasn't
technically necessary because only established connections were
considered for offloading I'm still leaning more towards considering it
a flow in original implementation since UDP NEW support wasn't the first
change modifying the offload behavior (43332cf97425 ("net/sched: act_ct:
Offload only ASSURED connections") was before that), so further changes
should have been anticipated. Hope this clarifies my motivation.

Note that I don't have strong opinion about it and willing to split the
patch, if necessary but to me it appears as just more trouble for
maintainers without any benefits...

>
>> Fixes: 9795ded7f924 ("net/sched: act_ct: Fill offloading tuple iifidx")
>> Reviewed-by: Paul Blakey <paulb@nvidia.com>
>> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
>
> ...
Simon Horman Nov. 8, 2023, 3:20 p.m. UTC | #3 
On Tue, Nov 07, 2023 at 06:30:24PM +0200, Vlad Buslov wrote:
> 
> On Tue 07 Nov 2023 at 11:27, Simon Horman <horms@kernel.org> wrote:
> > On Fri, Nov 03, 2023 at 04:14:10PM +0100, Vlad Buslov wrote:
> >> Referenced commit doesn't always set iifidx when offloading the flow to
> >> hardware. Fix the following cases:
> >> 
> >> - nf_conn_act_ct_ext_fill() is called before extension is created with
> >> nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
> >> unspecified iifidx when connection is offloaded after only single
> >> original-direction packet has been processed by tc data path. Always fill
> >> the new nf_conn_act_ct_ext instance after creating it in
> >> nf_conn_act_ct_ext_add().
> >> 
> >> - Offloading of unidirectional UDP NEW connections is now supported, but ct
> >> flow iifidx field is not updated when connection is promoted to
> >> bidirectional which can result reply-direction iifidx to be zero when
> >> refreshing the connection. Fill in the extension and update flow iifidx
> >> before calling flow_offload_refresh().
> >
> > Hi Vlad,
> >
> > these changes look good to me. However, I do wonder if the changes for each
> > of the two points above should be split into two patches, and
> > if the fixes tag for the second point should be.
> >
> > Fixes: 6a9bad0069cf ("net/sched: act_ct: offload UDP NEW connections")
> 
> Hi Simon,
> 
> I considered this but decided to send as single patch because
> connections 'refresh' mechanism has already existed before the UDP NEW
> offload and it didn't update the iifidx. While yes, it wasn't
> technically necessary because only established connections were
> considered for offloading I'm still leaning more towards considering it
> a flow in original implementation since UDP NEW support wasn't the first
> change modifying the offload behavior (43332cf97425 ("net/sched: act_ct:
> Offload only ASSURED connections") was before that), so further changes
> should have been anticipated. Hope this clarifies my motivation.
> 
> Note that I don't have strong opinion about it and willing to split the
> patch, if necessary but to me it appears as just more trouble for
> maintainers without any benefits...

Hi Vlad,

thanks for clarifying, I appreciate it.  I also don't have a strong feeling
on this, and with your clarification above I am now happy with the patch
arranged as-is.

Reviewed-by: Simon Horman <horms@kernel.org>

...
patchwork-bot+netdevbpf@kernel.org Nov. 9, 2023, 2 a.m. UTC | #4 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 3 Nov 2023 16:14:10 +0100 you wrote:
> Referenced commit doesn't always set iifidx when offloading the flow to
> hardware. Fix the following cases:
> 
> - nf_conn_act_ct_ext_fill() is called before extension is created with
> nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
> unspecified iifidx when connection is offloaded after only single
> original-direction packet has been processed by tc data path. Always fill
> the new nf_conn_act_ct_ext instance after creating it in
> nf_conn_act_ct_ext_add().
> 
> [...]

Here is the summary with links:
  - [net] net/sched: act_ct: Always fill offloading tuple iifidx
    https://git.kernel.org/netdev/net/c/9bc64bd0cd76

You are awesome, thank you!
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_conntrack_act_ct.h b/include/net/netfilter/nf_conntrack_act_ct.h
index 078d3c52c03f..e5f2f0b73a9a 100644
--- a/include/net/netfilter/nf_conntrack_act_ct.h
+++ b/include/net/netfilter/nf_conntrack_act_ct.h
@@ -20,21 +20,6 @@  static inline struct nf_conn_act_ct_ext *nf_conn_act_ct_ext_find(const struct nf
 #endif
 }
 
-static inline struct nf_conn_act_ct_ext *nf_conn_act_ct_ext_add(struct nf_conn *ct)
-{
-#if IS_ENABLED(CONFIG_NET_ACT_CT)
-	struct nf_conn_act_ct_ext *act_ct = nf_ct_ext_find(ct, NF_CT_EXT_ACT_CT);
-
-	if (act_ct)
-		return act_ct;
-
-	act_ct = nf_ct_ext_add(ct, NF_CT_EXT_ACT_CT, GFP_ATOMIC);
-	return act_ct;
-#else
-	return NULL;
-#endif
-}
-
 static inline void nf_conn_act_ct_ext_fill(struct sk_buff *skb, struct nf_conn *ct,
 					   enum ip_conntrack_info ctinfo)
 {
@@ -47,4 +32,23 @@  static inline void nf_conn_act_ct_ext_fill(struct sk_buff *skb, struct nf_conn *
 #endif
 }
 
+static inline struct
+nf_conn_act_ct_ext *nf_conn_act_ct_ext_add(struct sk_buff *skb,
+					   struct nf_conn *ct,
+					   enum ip_conntrack_info ctinfo)
+{
+#if IS_ENABLED(CONFIG_NET_ACT_CT)
+	struct nf_conn_act_ct_ext *act_ct = nf_ct_ext_find(ct, NF_CT_EXT_ACT_CT);
+
+	if (act_ct)
+		return act_ct;
+
+	act_ct = nf_ct_ext_add(ct, NF_CT_EXT_ACT_CT, GFP_ATOMIC);
+	nf_conn_act_ct_ext_fill(skb, ct, ctinfo);
+	return act_ct;
+#else
+	return NULL;
+#endif
+}
+
 #endif /* _NF_CONNTRACK_ACT_CT_H */
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 0b9a785dea45..3019a4406ca4 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -985,7 +985,7 @@  static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
 		if (err)
 			return err;
 
-		nf_conn_act_ct_ext_add(ct);
+		nf_conn_act_ct_ext_add(skb, ct, ctinfo);
 	} else if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
 		   labels_nonzero(&info->labels.mask)) {
 		err = ovs_ct_set_labels(ct, key, &info->labels.value,
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 9583645e86c2..0db0ecf1d110 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -376,6 +376,17 @@  static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry,
 	entry->tuplehash[dir].tuple.tc.iifidx = act_ct_ext->ifindex[dir];
 }
 
+static void tcf_ct_flow_ct_ext_ifidx_update(struct flow_offload *entry)
+{
+	struct nf_conn_act_ct_ext *act_ct_ext;
+
+	act_ct_ext = nf_conn_act_ct_ext_find(entry->ct);
+	if (act_ct_ext) {
+		tcf_ct_flow_tc_ifidx(entry, act_ct_ext, FLOW_OFFLOAD_DIR_ORIGINAL);
+		tcf_ct_flow_tc_ifidx(entry, act_ct_ext, FLOW_OFFLOAD_DIR_REPLY);
+	}
+}
+
 static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
 				  struct nf_conn *ct,
 				  bool tcp, bool bidirectional)
@@ -671,6 +682,8 @@  static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
 	else
 		ctinfo = IP_CT_ESTABLISHED_REPLY;
 
+	nf_conn_act_ct_ext_fill(skb, ct, ctinfo);
+	tcf_ct_flow_ct_ext_ifidx_update(flow);
 	flow_offload_refresh(nf_ft, flow, force_refresh);
 	if (!test_bit(IPS_ASSURED_BIT, &ct->status)) {
 		/* Process this flow in SW to allow promoting to ASSURED */
@@ -1034,7 +1047,7 @@  TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
 
 		if (!nf_ct_is_confirmed(ct))
-			nf_conn_act_ct_ext_add(ct);
+			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
 
 		/* This will take care of sending queued events
 		 * even if the connection is already confirmed.