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

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

[Vlad Buslov]

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(-)

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.
-- 
2.39.2

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

[Simon Horman]

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>

...

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

[Vlad Buslov]

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>
>
> ...

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

[Simon Horman]

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>

...

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html