[PATCH net-next 0/3] net: add helper support in tc act_ct for ovs offloading

[PATCH net-next 0/3] net: add helper support in tc act_ct for ovs offloading

[Xin Long]

Ilya reported an issue that FTP traffic would be broken when the OVS flow
with ct(commit,alg=ftp) installed in the OVS kernel module, and it was
caused by that TC didn't support the ftp helper offloaded from OVS.

This patchset is to add the helper support in act_ct for ovs offloading
in kernel net/sched.

The 1st patch moves some code from openvswitch into nf_conntrack_helper
so that it can be used by net/sched in the 3rd patch (Note there are
still some other common code used in both OVS and TC and I will extract
it in other patches). The 2nd patch extracts another function in net/
sched to make the 3rd patch easier to write.

The user space part will be added in another patch, and with it these OVS
flows (FTP over SNAT) can be used to test this feature:

  table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk \
    actions=ct(table=1, nat), normal
  table=0, in_port=veth2,tcp,ct_state=-trk actions=ct(table=1, nat)
  table=0, in_port=veth1,tcp,ct_state=-trk actions=ct(table=0, nat)
  table=0, in_port=veth1,tcp,ct_state=+trk+rel actions=ct(commit, nat),normal
  table=0, in_port=veth1,tcp,ct_state=+trk+est actions=veth2"

  table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new \
    actions=ct(commit, nat(src=7.7.16.1), alg=ftp),normal"
  table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+est actions=veth2"
  table=1, in_port=veth2,tcp,ct_state=+trk+est actions=veth1"

Xin Long (3):
  net: move the helper function to nf_conntrack_helper for ovs and tc
  net: sched: call tcf_ct_params_free to free params in tcf_ct_init
  net: sched: add helper support in act_ct

 include/net/netfilter/nf_conntrack_helper.h |   2 +
 include/net/tc_act/tc_ct.h                  |   1 +
 include/uapi/linux/tc_act/tc_ct.h           |   3 +
 net/netfilter/nf_conntrack_helper.c         |  71 ++++++++++
 net/openvswitch/conntrack.c                 |  61 +-------
 net/sched/act_ct.c                          | 148 +++++++++++++++++---
 6 files changed, 204 insertions(+), 82 deletions(-)

-- 
2.31.1

[PATCH net-next 1/3] net: move the helper function to nf_conntrack_helper for ovs and tc

[Xin Long]

Move ovs_ct_helper from openvswitch to nf_conntrack_helper and rename
as nf_ct_helper so that it can be used in TC act_ct in the next patch.
Note that it also adds the checks for the family and proto, as in TC
act_ct, the packets with correct family and proto are not guaranteed.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/netfilter/nf_conntrack_helper.h |  2 +
 net/netfilter/nf_conntrack_helper.c         | 71 +++++++++++++++++++++
 net/openvswitch/conntrack.c                 | 61 +-----------------
 3 files changed, 74 insertions(+), 60 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 9939c366f720..6c32e59fc16f 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -115,6 +115,8 @@ struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 			      gfp_t flags);
 
+int nf_ct_helper(struct sk_buff *skb, u16 proto);
+
 void nf_ct_helper_destroy(struct nf_conn *ct);
 
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index ff737a76052e..83615e479f87 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -26,7 +26,9 @@
 #include <net/netfilter/nf_conntrack_extend.h>
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 #include <net/netfilter/nf_log.h>
+#include <net/ip.h>
 
 static DEFINE_MUTEX(nf_ct_helper_mutex);
 struct hlist_head *nf_ct_helper_hash __read_mostly;
@@ -240,6 +242,75 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 }
 EXPORT_SYMBOL_GPL(__nf_ct_try_assign_helper);
 
+/* 'skb' should already be pulled to nh_ofs. */
+int nf_ct_helper(struct sk_buff *skb, u16 proto)
+{
+	const struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	enum ip_conntrack_info ctinfo;
+	unsigned int protoff;
+	struct nf_conn *ct;
+	int err;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
+		return NF_ACCEPT;
+
+	help = nfct_help(ct);
+	if (!help)
+		return NF_ACCEPT;
+
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		return NF_ACCEPT;
+
+	if (helper->tuple.src.l3num != NFPROTO_UNSPEC &&
+	    helper->tuple.src.l3num != proto)
+		return NF_ACCEPT;
+
+	switch (proto) {
+	case NFPROTO_IPV4:
+		protoff = ip_hdrlen(skb);
+		proto = ip_hdr(skb)->protocol;
+		break;
+	case NFPROTO_IPV6: {
+		u8 nexthdr = ipv6_hdr(skb)->nexthdr;
+		__be16 frag_off;
+		int ofs;
+
+		ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
+				       &frag_off);
+		if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
+			pr_debug("proto header not found\n");
+			return NF_ACCEPT;
+		}
+		protoff = ofs;
+		proto = nexthdr;
+		break;
+	}
+	default:
+		WARN_ONCE(1, "helper invoked on non-IP family!");
+		return NF_DROP;
+	}
+
+	if (helper->tuple.dst.protonum != proto)
+		return NF_ACCEPT;
+
+	err = helper->help(skb, protoff, ct, ctinfo);
+	if (err != NF_ACCEPT)
+		return err;
+
+	/* Adjust seqs after helper.  This is needed due to some helpers (e.g.,
+	 * FTP with NAT) adusting the TCP payload size when mangling IP
+	 * addresses and/or port numbers in the text-based control connection.
+	 */
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
+		return NF_DROP;
+	return NF_ACCEPT;
+}
+EXPORT_SYMBOL_GPL(nf_ct_helper);
+
 /* appropriate ct lock protecting must be taken by caller */
 static int unhelp(struct nf_conn *ct, void *me)
 {
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index cb255d8ed99a..75e709303815 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -434,65 +434,6 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key,
 	return 0;
 }
 
-/* 'skb' should already be pulled to nh_ofs. */
-static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
-{
-	const struct nf_conntrack_helper *helper;
-	const struct nf_conn_help *help;
-	enum ip_conntrack_info ctinfo;
-	unsigned int protoff;
-	struct nf_conn *ct;
-	int err;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
-		return NF_ACCEPT;
-
-	help = nfct_help(ct);
-	if (!help)
-		return NF_ACCEPT;
-
-	helper = rcu_dereference(help->helper);
-	if (!helper)
-		return NF_ACCEPT;
-
-	switch (proto) {
-	case NFPROTO_IPV4:
-		protoff = ip_hdrlen(skb);
-		break;
-	case NFPROTO_IPV6: {
-		u8 nexthdr = ipv6_hdr(skb)->nexthdr;
-		__be16 frag_off;
-		int ofs;
-
-		ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
-				       &frag_off);
-		if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
-			pr_debug("proto header not found\n");
-			return NF_ACCEPT;
-		}
-		protoff = ofs;
-		break;
-	}
-	default:
-		WARN_ONCE(1, "helper invoked on non-IP family!");
-		return NF_DROP;
-	}
-
-	err = helper->help(skb, protoff, ct, ctinfo);
-	if (err != NF_ACCEPT)
-		return err;
-
-	/* Adjust seqs after helper.  This is needed due to some helpers (e.g.,
-	 * FTP with NAT) adusting the TCP payload size when mangling IP
-	 * addresses and/or port numbers in the text-based control connection.
-	 */
-	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
-	    !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
-		return NF_DROP;
-	return NF_ACCEPT;
-}
-
 /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
  * value if 'skb' is freed.
  */
@@ -1037,7 +978,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		 */
 		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
 					      info->commit) &&
-		    ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
+		    nf_ct_helper(skb, info->family) != NF_ACCEPT) {
 			return -EINVAL;
 		}
 
-- 
2.31.1

[PATCH net-next 2/3] net: sched: call tcf_ct_params_free to free params in tcf_ct_init

[Xin Long]

This patch is to make the err path simple by calling tcf_ct_params_free(),
so that it won't cause problems when more members are added into param and
need freeing on the err path.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_ct.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index b38d91d6b249..193a460a9d7f 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -345,11 +345,9 @@ static void tcf_ct_flow_table_cleanup_work(struct work_struct *work)
 	module_put(THIS_MODULE);
 }
 
-static void tcf_ct_flow_table_put(struct tcf_ct_params *params)
+static void tcf_ct_flow_table_put(struct tcf_ct_flow_table *ct_ft)
 {
-	struct tcf_ct_flow_table *ct_ft = params->ct_ft;
-
-	if (refcount_dec_and_test(&params->ct_ft->ref)) {
+	if (refcount_dec_and_test(&ct_ft->ref)) {
 		rhashtable_remove_fast(&zones_ht, &ct_ft->node, zones_params);
 		INIT_RCU_WORK(&ct_ft->rwork, tcf_ct_flow_table_cleanup_work);
 		queue_rcu_work(act_ct_wq, &ct_ft->rwork);
@@ -832,18 +830,23 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
 	return err;
 }
 
-static void tcf_ct_params_free(struct rcu_head *head)
+static void tcf_ct_params_free(struct tcf_ct_params *params)
 {
-	struct tcf_ct_params *params = container_of(head,
-						    struct tcf_ct_params, rcu);
-
-	tcf_ct_flow_table_put(params);
-
+	if (params->ct_ft)
+		tcf_ct_flow_table_put(params->ct_ft);
 	if (params->tmpl)
 		nf_ct_put(params->tmpl);
 	kfree(params);
 }
 
+static void tcf_ct_params_free_rcu(struct rcu_head *head)
+{
+	struct tcf_ct_params *params;
+
+	params = container_of(head, struct tcf_ct_params, rcu);
+	tcf_ct_params_free(params);
+}
+
 #if IS_ENABLED(CONFIG_NF_NAT)
 /* Modelled after nf_nat_ipv[46]_fn().
  * range is only used for new, uninitialized NAT state.
@@ -1390,7 +1393,7 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
 
 	err = tcf_ct_flow_table_get(net, params);
 	if (err)
-		goto cleanup_params;
+		goto cleanup;
 
 	spin_lock_bh(&c->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
@@ -1401,17 +1404,15 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 	if (params)
-		call_rcu(&params->rcu, tcf_ct_params_free);
+		call_rcu(&params->rcu, tcf_ct_params_free_rcu);
 
 	return res;
 
-cleanup_params:
-	if (params->tmpl)
-		nf_ct_put(params->tmpl);
 cleanup:
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
-	kfree(params);
+	if (params)
+		tcf_ct_params_free(params);
 	tcf_idr_release(*a, bind);
 	return err;
 }
@@ -1423,7 +1424,7 @@ static void tcf_ct_cleanup(struct tc_action *a)
 
 	params = rcu_dereference_protected(c->params, 1);
 	if (params)
-		call_rcu(&params->rcu, tcf_ct_params_free);
+		call_rcu(&params->rcu, tcf_ct_params_free_rcu);
 }
 
 static int tcf_ct_dump_key_val(struct sk_buff *skb,
-- 
2.31.1

[PATCH net-next 3/3] net: sched: add helper support in act_ct

[Xin Long]

This patch is to add helper support in act_ct for OVS actions=ct(alg=xxx)
offloading, which is corresponding to Commit cae3a2627520 ("openvswitch:
Allow attaching helpers to ct action") in OVS kernel part.

The difference is when adding TC actions family and proto cannot be got
from the filter/match, other than helper name in tb[TCA_CT_HELPER_NAME],
we also need to send the family in tb[TCA_CT_HELPER_FAMILY] and the
proto in tb[TCA_CT_HELPER_PROTO] to kernel.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/tc_act/tc_ct.h        |   1 +
 include/uapi/linux/tc_act/tc_ct.h |   3 +
 net/sched/act_ct.c                | 113 ++++++++++++++++++++++++++++--
 3 files changed, 112 insertions(+), 5 deletions(-)

diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
index 8250d6f0a462..b24ea2d9400b 100644
--- a/include/net/tc_act/tc_ct.h
+++ b/include/net/tc_act/tc_ct.h
@@ -10,6 +10,7 @@
 #include <net/netfilter/nf_conntrack_labels.h>
 
 struct tcf_ct_params {
+	struct nf_conntrack_helper *helper;
 	struct nf_conn *tmpl;
 	u16 zone;
 
diff --git a/include/uapi/linux/tc_act/tc_ct.h b/include/uapi/linux/tc_act/tc_ct.h
index 5fb1d7ac1027..6c5200f0ed38 100644
--- a/include/uapi/linux/tc_act/tc_ct.h
+++ b/include/uapi/linux/tc_act/tc_ct.h
@@ -22,6 +22,9 @@ enum {
 	TCA_CT_NAT_PORT_MIN,	/* be16 */
 	TCA_CT_NAT_PORT_MAX,	/* be16 */
 	TCA_CT_PAD,
+	TCA_CT_HELPER_NAME,	/* string */
+	TCA_CT_HELPER_FAMILY,	/* u8 */
+	TCA_CT_HELPER_PROTO,	/* u8 */
 	__TCA_CT_MAX
 };
 
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 193a460a9d7f..f237c27079db 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -33,6 +33,7 @@
 #include <net/netfilter/nf_conntrack_acct.h>
 #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
 #include <net/netfilter/nf_conntrack_act_ct.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 #include <uapi/linux/netfilter/nf_nat.h>
 
 static struct workqueue_struct *act_ct_wq;
@@ -655,7 +656,7 @@ struct tc_ct_action_net {
 
 /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
 static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
-				   u16 zone_id, bool force)
+				   struct tcf_ct_params *p, bool force)
 {
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct;
@@ -665,8 +666,15 @@ static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
 		return false;
 	if (!net_eq(net, read_pnet(&ct->ct_net)))
 		goto drop_ct;
-	if (nf_ct_zone(ct)->id != zone_id)
+	if (nf_ct_zone(ct)->id != p->zone)
 		goto drop_ct;
+	if (p->helper) {
+		struct nf_conn_help *help;
+
+		help = nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
+		if (help && rcu_access_pointer(help->helper) != p->helper)
+			goto drop_ct;
+	}
 
 	/* Force conntrack entry direction. */
 	if (force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
@@ -832,6 +840,13 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
 
 static void tcf_ct_params_free(struct tcf_ct_params *params)
 {
+	if (params->helper) {
+#if IS_ENABLED(CONFIG_NF_NAT)
+		if (params->ct_action & TCA_CT_ACT_NAT)
+			nf_nat_helper_put(params->helper);
+#endif
+		nf_conntrack_helper_put(params->helper);
+	}
 	if (params->ct_ft)
 		tcf_ct_flow_table_put(params->ct_ft);
 	if (params->tmpl)
@@ -1033,6 +1048,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	struct nf_hook_state state;
 	int nh_ofs, err, retval;
 	struct tcf_ct_params *p;
+	bool add_helper = false;
 	bool skip_add = false;
 	bool defrag = false;
 	struct nf_conn *ct;
@@ -1086,7 +1102,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	 * actually run the packet through conntrack twice unless it's for a
 	 * different zone.
 	 */
-	cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
+	cached = tcf_ct_skb_nfct_cached(net, skb, p, force);
 	if (!cached) {
 		if (tcf_ct_flow_table_lookup(p, skb, family)) {
 			skip_add = true;
@@ -1119,6 +1135,22 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	if (err != NF_ACCEPT)
 		goto drop;
 
+	if (commit && p->helper && !nfct_help(ct)) {
+		err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
+		if (err)
+			goto drop;
+		add_helper = true;
+		if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
+			if (!nfct_seqadj_ext_add(ct))
+				return -EINVAL;
+		}
+	}
+
+	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
+		if (nf_ct_helper(skb, family) != NF_ACCEPT)
+			goto drop;
+	}
+
 	if (commit) {
 		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
 		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
@@ -1167,6 +1199,9 @@ static const struct nla_policy ct_policy[TCA_CT_MAX + 1] = {
 	[TCA_CT_NAT_IPV6_MAX] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
 	[TCA_CT_NAT_PORT_MIN] = { .type = NLA_U16 },
 	[TCA_CT_NAT_PORT_MAX] = { .type = NLA_U16 },
+	[TCA_CT_HELPER_NAME] = { .type = NLA_STRING, .len = NF_CT_HELPER_NAME_LEN },
+	[TCA_CT_HELPER_FAMILY] = { .type = NLA_U8 },
+	[TCA_CT_HELPER_PROTO] = { .type = NLA_U8 },
 };
 
 static int tcf_ct_fill_params_nat(struct tcf_ct_params *p,
@@ -1248,6 +1283,39 @@ static void tcf_ct_set_key_val(struct nlattr **tb,
 		nla_memcpy(mask, tb[mask_type], len);
 }
 
+static int tcf_ct_add_helper(struct tcf_ct_params *p, const char *name, u8 family,
+			     u8 proto, struct netlink_ext_ack *extack)
+{
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help;
+	int err;
+
+	helper = nf_conntrack_helper_try_module_get(name, family, proto);
+	if (!helper) {
+		NL_SET_ERR_MSG_MOD(extack, "Unknown helper");
+		return -EINVAL;
+	}
+
+	help = nf_ct_helper_ext_add(p->tmpl, GFP_KERNEL);
+	if (!help) {
+		nf_conntrack_helper_put(helper);
+		return -ENOMEM;
+	}
+#if IS_ENABLED(CONFIG_NF_NAT)
+	if (p->ct_action & TCA_CT_ACT_NAT) {
+		err = nf_nat_helper_try_module_get(name, family, proto);
+		if (err) {
+			nf_conntrack_helper_put(helper);
+			NL_SET_ERR_MSG_MOD(extack, "Failed to load NAT helper");
+			return err;
+		}
+	}
+#endif
+	rcu_assign_pointer(help->helper, helper);
+	p->helper = helper;
+	return 0;
+}
+
 static int tcf_ct_fill_params(struct net *net,
 			      struct tcf_ct_params *p,
 			      struct tc_ct *parm,
@@ -1256,8 +1324,9 @@ static int tcf_ct_fill_params(struct net *net,
 {
 	struct tc_ct_action_net *tn = net_generic(net, act_ct_ops.net_id);
 	struct nf_conntrack_zone zone;
+	int err, family, proto, len;
 	struct nf_conn *tmpl;
-	int err;
+	char *name;
 
 	p->zone = NF_CT_DEFAULT_ZONE_ID;
 
@@ -1318,10 +1387,28 @@ static int tcf_ct_fill_params(struct net *net,
 		NL_SET_ERR_MSG_MOD(extack, "Failed to allocate conntrack template");
 		return -ENOMEM;
 	}
-	__set_bit(IPS_CONFIRMED_BIT, &tmpl->status);
 	p->tmpl = tmpl;
+	if (tb[TCA_CT_HELPER_NAME]) {
+		name = nla_data(tb[TCA_CT_HELPER_NAME]);
+		len = nla_len(tb[TCA_CT_HELPER_NAME]);
+		if (len > 16 || name[len - 1] != '\0') {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to parse helper name.");
+			err = -EINVAL;
+			goto err;
+		}
+		family = tb[TCA_CT_HELPER_FAMILY] ? nla_get_u8(tb[TCA_CT_HELPER_FAMILY]) : AF_INET;
+		proto = tb[TCA_CT_HELPER_PROTO] ? nla_get_u8(tb[TCA_CT_HELPER_PROTO]) : IPPROTO_TCP;
+		err = tcf_ct_add_helper(p, name, family, proto, extack);
+		if (err)
+			goto err;
+	}
 
+	__set_bit(IPS_CONFIRMED_BIT, &tmpl->status);
 	return 0;
+err:
+	nf_ct_put(p->tmpl);
+	p->tmpl = NULL;
+	return err;
 }
 
 static int tcf_ct_init(struct net *net, struct nlattr *nla,
@@ -1490,6 +1577,19 @@ static int tcf_ct_dump_nat(struct sk_buff *skb, struct tcf_ct_params *p)
 	return 0;
 }
 
+static int tcf_ct_dump_helper(struct sk_buff *skb, struct nf_conntrack_helper *helper)
+{
+	if (!helper)
+		return 0;
+
+	if (nla_put_string(skb, TCA_CT_HELPER_NAME, helper->name) ||
+	    nla_put_u8(skb, TCA_CT_HELPER_FAMILY, helper->tuple.src.l3num) ||
+	    nla_put_u8(skb, TCA_CT_HELPER_PROTO, helper->tuple.dst.protonum))
+		return -1;
+
+	return 0;
+}
+
 static inline int tcf_ct_dump(struct sk_buff *skb, struct tc_action *a,
 			      int bind, int ref)
 {
@@ -1542,6 +1642,9 @@ static inline int tcf_ct_dump(struct sk_buff *skb, struct tc_action *a,
 	if (tcf_ct_dump_nat(skb, p))
 		goto nla_put_failure;
 
+	if (tcf_ct_dump_helper(skb, p->helper))
+		goto nla_put_failure;
+
 skip_dump:
 	if (nla_put(skb, TCA_CT_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
-- 
2.31.1

Re: [PATCH net-next 3/3] net: sched: add helper support in act_ct

[Paolo Abeni]

On Tue, 2022-10-04 at 21:19 -0400, Xin Long wrote:
> This patch is to add helper support in act_ct for OVS actions=ct(alg=xxx)
> offloading, which is corresponding to Commit cae3a2627520 ("openvswitch:
> Allow attaching helpers to ct action") in OVS kernel part.
> 
> The difference is when adding TC actions family and proto cannot be got
> from the filter/match, other than helper name in tb[TCA_CT_HELPER_NAME],
> we also need to send the family in tb[TCA_CT_HELPER_FAMILY] and the
> proto in tb[TCA_CT_HELPER_PROTO] to kernel.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/tc_act/tc_ct.h        |   1 +
>  include/uapi/linux/tc_act/tc_ct.h |   3 +
>  net/sched/act_ct.c                | 113 ++++++++++++++++++++++++++++--
>  3 files changed, 112 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
> index 8250d6f0a462..b24ea2d9400b 100644
> --- a/include/net/tc_act/tc_ct.h
> +++ b/include/net/tc_act/tc_ct.h
> @@ -10,6 +10,7 @@
>  #include <net/netfilter/nf_conntrack_labels.h>
>  
>  struct tcf_ct_params {
> +	struct nf_conntrack_helper *helper;
>  	struct nf_conn *tmpl;
>  	u16 zone;
>  
> diff --git a/include/uapi/linux/tc_act/tc_ct.h b/include/uapi/linux/tc_act/tc_ct.h
> index 5fb1d7ac1027..6c5200f0ed38 100644
> --- a/include/uapi/linux/tc_act/tc_ct.h
> +++ b/include/uapi/linux/tc_act/tc_ct.h
> @@ -22,6 +22,9 @@ enum {
>  	TCA_CT_NAT_PORT_MIN,	/* be16 */
>  	TCA_CT_NAT_PORT_MAX,	/* be16 */
>  	TCA_CT_PAD,
> +	TCA_CT_HELPER_NAME,	/* string */
> +	TCA_CT_HELPER_FAMILY,	/* u8 */
> +	TCA_CT_HELPER_PROTO,	/* u8 */
>  	__TCA_CT_MAX
>  };
>  
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 193a460a9d7f..f237c27079db 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -33,6 +33,7 @@
>  #include <net/netfilter/nf_conntrack_acct.h>
>  #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
>  #include <net/netfilter/nf_conntrack_act_ct.h>
> +#include <net/netfilter/nf_conntrack_seqadj.h>
>  #include <uapi/linux/netfilter/nf_nat.h>
>  
>  static struct workqueue_struct *act_ct_wq;
> @@ -655,7 +656,7 @@ struct tc_ct_action_net {
>  
>  /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
>  static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
> -				   u16 zone_id, bool force)
> +				   struct tcf_ct_params *p, bool force)
>  {
>  	enum ip_conntrack_info ctinfo;
>  	struct nf_conn *ct;
> @@ -665,8 +666,15 @@ static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
>  		return false;
>  	if (!net_eq(net, read_pnet(&ct->ct_net)))
>  		goto drop_ct;
> -	if (nf_ct_zone(ct)->id != zone_id)
> +	if (nf_ct_zone(ct)->id != p->zone)
>  		goto drop_ct;
> +	if (p->helper) {
> +		struct nf_conn_help *help;
> +
> +		help = nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
> +		if (help && rcu_access_pointer(help->helper) != p->helper)
> +			goto drop_ct;
> +	}
>  
>  	/* Force conntrack entry direction. */
>  	if (force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
> @@ -832,6 +840,13 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
>  
>  static void tcf_ct_params_free(struct tcf_ct_params *params)
>  {
> +	if (params->helper) {
> +#if IS_ENABLED(CONFIG_NF_NAT)
> +		if (params->ct_action & TCA_CT_ACT_NAT)
> +			nf_nat_helper_put(params->helper);
> +#endif
> +		nf_conntrack_helper_put(params->helper);
> +	}

There is exactly the same code chunk in __ovs_ct_free_action(), I guess
you can extract a common helper here, too.

>  	if (params->ct_ft)
>  		tcf_ct_flow_table_put(params->ct_ft);
>  	if (params->tmpl)
> @@ -1033,6 +1048,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	struct nf_hook_state state;
>  	int nh_ofs, err, retval;
>  	struct tcf_ct_params *p;
> +	bool add_helper = false;
>  	bool skip_add = false;
>  	bool defrag = false;
>  	struct nf_conn *ct;
> @@ -1086,7 +1102,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	 * actually run the packet through conntrack twice unless it's for a
>  	 * different zone.
>  	 */
> -	cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
> +	cached = tcf_ct_skb_nfct_cached(net, skb, p, force);
>  	if (!cached) {
>  		if (tcf_ct_flow_table_lookup(p, skb, family)) {
>  			skip_add = true;
> @@ -1119,6 +1135,22 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	if (err != NF_ACCEPT)
>  		goto drop;
>  
> +	if (commit && p->helper && !nfct_help(ct)) {
> +		err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> +		if (err)
> +			goto drop;
> +		add_helper = true;
> +		if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> +			if (!nfct_seqadj_ext_add(ct))
> +				return -EINVAL;

This return looks suspect/wrong. It will confuse the tc action
mechanism. I guess you shold do
				goto drop;

even here. 

> +		}
> +	}
> +
> +	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> +		if (nf_ct_helper(skb, family) != NF_ACCEPT)
> +			goto drop;

With the above change, this chunk closely resamble

https://elixir.bootlin.com/linux/latest/source/net/openvswitch/conntrack.c#L1018
...
https://elixir.bootlin.com/linux/latest/source/net/openvswitch/conntrack.c#L1042

opening to an additional common helper;) Not strictly necessary, just
nice to have :)


Thanks!

Paolo

Re: [PATCH net-next 3/3] net: sched: add helper support in act_ct

[Xin Long]

On Thu, Oct 6, 2022 at 5:57 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2022-10-04 at 21:19 -0400, Xin Long wrote:
> > This patch is to add helper support in act_ct for OVS actions=ct(alg=xxx)
> > offloading, which is corresponding to Commit cae3a2627520 ("openvswitch:
> > Allow attaching helpers to ct action") in OVS kernel part.
> >
> > The difference is when adding TC actions family and proto cannot be got
> > from the filter/match, other than helper name in tb[TCA_CT_HELPER_NAME],
> > we also need to send the family in tb[TCA_CT_HELPER_FAMILY] and the
> > proto in tb[TCA_CT_HELPER_PROTO] to kernel.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/tc_act/tc_ct.h        |   1 +
> >  include/uapi/linux/tc_act/tc_ct.h |   3 +
> >  net/sched/act_ct.c                | 113 ++++++++++++++++++++++++++++--
> >  3 files changed, 112 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
> > index 8250d6f0a462..b24ea2d9400b 100644
> > --- a/include/net/tc_act/tc_ct.h
> > +++ b/include/net/tc_act/tc_ct.h
> > @@ -10,6 +10,7 @@
> >  #include <net/netfilter/nf_conntrack_labels.h>
> >
> >  struct tcf_ct_params {
> > +     struct nf_conntrack_helper *helper;
> >       struct nf_conn *tmpl;
> >       u16 zone;
> >
> > diff --git a/include/uapi/linux/tc_act/tc_ct.h b/include/uapi/linux/tc_act/tc_ct.h
> > index 5fb1d7ac1027..6c5200f0ed38 100644
> > --- a/include/uapi/linux/tc_act/tc_ct.h
> > +++ b/include/uapi/linux/tc_act/tc_ct.h
> > @@ -22,6 +22,9 @@ enum {
> >       TCA_CT_NAT_PORT_MIN,    /* be16 */
> >       TCA_CT_NAT_PORT_MAX,    /* be16 */
> >       TCA_CT_PAD,
> > +     TCA_CT_HELPER_NAME,     /* string */
> > +     TCA_CT_HELPER_FAMILY,   /* u8 */
> > +     TCA_CT_HELPER_PROTO,    /* u8 */
> >       __TCA_CT_MAX
> >  };
> >
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index 193a460a9d7f..f237c27079db 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -33,6 +33,7 @@
> >  #include <net/netfilter/nf_conntrack_acct.h>
> >  #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
> >  #include <net/netfilter/nf_conntrack_act_ct.h>
> > +#include <net/netfilter/nf_conntrack_seqadj.h>
> >  #include <uapi/linux/netfilter/nf_nat.h>
> >
> >  static struct workqueue_struct *act_ct_wq;
> > @@ -655,7 +656,7 @@ struct tc_ct_action_net {
> >
> >  /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
> >  static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
> > -                                u16 zone_id, bool force)
> > +                                struct tcf_ct_params *p, bool force)
> >  {
> >       enum ip_conntrack_info ctinfo;
> >       struct nf_conn *ct;
> > @@ -665,8 +666,15 @@ static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
> >               return false;
> >       if (!net_eq(net, read_pnet(&ct->ct_net)))
> >               goto drop_ct;
> > -     if (nf_ct_zone(ct)->id != zone_id)
> > +     if (nf_ct_zone(ct)->id != p->zone)
> >               goto drop_ct;
> > +     if (p->helper) {
> > +             struct nf_conn_help *help;
> > +
> > +             help = nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
> > +             if (help && rcu_access_pointer(help->helper) != p->helper)
> > +                     goto drop_ct;
> > +     }
> >
> >       /* Force conntrack entry direction. */
> >       if (force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
> > @@ -832,6 +840,13 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
> >
> >  static void tcf_ct_params_free(struct tcf_ct_params *params)
> >  {
> > +     if (params->helper) {
> > +#if IS_ENABLED(CONFIG_NF_NAT)
> > +             if (params->ct_action & TCA_CT_ACT_NAT)
> > +                     nf_nat_helper_put(params->helper);
> > +#endif
> > +             nf_conntrack_helper_put(params->helper);
> > +     }
>
> There is exactly the same code chunk in __ovs_ct_free_action(), I guess
> you can extract a common helper here, too.
>
> >       if (params->ct_ft)
> >               tcf_ct_flow_table_put(params->ct_ft);
> >       if (params->tmpl)
> > @@ -1033,6 +1048,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >       struct nf_hook_state state;
> >       int nh_ofs, err, retval;
> >       struct tcf_ct_params *p;
> > +     bool add_helper = false;
> >       bool skip_add = false;
> >       bool defrag = false;
> >       struct nf_conn *ct;
> > @@ -1086,7 +1102,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >        * actually run the packet through conntrack twice unless it's for a
> >        * different zone.
> >        */
> > -     cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
> > +     cached = tcf_ct_skb_nfct_cached(net, skb, p, force);
> >       if (!cached) {
> >               if (tcf_ct_flow_table_lookup(p, skb, family)) {
> >                       skip_add = true;
> > @@ -1119,6 +1135,22 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >       if (err != NF_ACCEPT)
> >               goto drop;
> >
> > +     if (commit && p->helper && !nfct_help(ct)) {
> > +             err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> > +             if (err)
> > +                     goto drop;
> > +             add_helper = true;
> > +             if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> > +                     if (!nfct_seqadj_ext_add(ct))
> > +                             return -EINVAL;
>
> This return looks suspect/wrong. It will confuse the tc action
> mechanism. I guess you shold do
>                                 goto drop;
>
> even here.
You're right, will fix it.

Thanks.
>
> > +             }
> > +     }
> > +
> > +     if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> > +             if (nf_ct_helper(skb, family) != NF_ACCEPT)
> > +                     goto drop;
>
> With the above change, this chunk closely resamble
>
> https://elixir.bootlin.com/linux/latest/source/net/openvswitch/conntrack.c#L1018
> ...
> https://elixir.bootlin.com/linux/latest/source/net/openvswitch/conntrack.c#L1042
>
> opening to an additional common helper;) Not strictly necessary, just
> nice to have :)
>
>
> Thanks!
>
> Paolo
>

Re: [PATCH net-next 3/3] net: sched: add helper support in act_ct

[Pablo Neira Ayuso]

On Tue, Oct 04, 2022 at 09:19:56PM -0400, Xin Long wrote:
[...]
> @@ -1119,6 +1135,22 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	if (err != NF_ACCEPT)
>  		goto drop;
>  
> +	if (commit && p->helper && !nfct_help(ct)) {
> +		err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> +		if (err)
> +			goto drop;
> +		add_helper = true;
> +		if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> +			if (!nfct_seqadj_ext_add(ct))

You can only add ct extensions if ct is !nf_ct_is_confirmed(ct)), is
this guaranteed in this codepath?

> +				return -EINVAL;
> +		}
> +	}
> +
> +	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> +		if (nf_ct_helper(skb, family) != NF_ACCEPT)
> +			goto drop;
> +	}
> +
>  	if (commit) {
>  		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
>  		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);

Re: [PATCH net-next 3/3] net: sched: add helper support in act_ct

[Xin Long]

On Thu, Oct 6, 2022 at 10:11 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Oct 04, 2022 at 09:19:56PM -0400, Xin Long wrote:
> [...]
> > @@ -1119,6 +1135,22 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >       if (err != NF_ACCEPT)
> >               goto drop;
> >
> > +     if (commit && p->helper && !nfct_help(ct)) {
> > +             err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> > +             if (err)
> > +                     goto drop;
> > +             add_helper = true;
> > +             if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> > +                     if (!nfct_seqadj_ext_add(ct))
>
> You can only add ct extensions if ct is !nf_ct_is_confirmed(ct)), is
> this guaranteed in this codepath?
This is a good catch, the same issue exists on __nf_ct_try_assign_helper(),
and also in __ovs_ct_lookup().

I could trigger the warning on OVS conntrack with the flow:

table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
actions=ct(commit, table=1)
table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
actions=ct(commit, alg=ftp),normal

I will prepare a fix for ovs conntrack first.

Thanks.

>
> > +                             return -EINVAL;
> > +             }
> > +     }
> > +
> > +     if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> > +             if (nf_ct_helper(skb, family) != NF_ACCEPT)
> > +                     goto drop;
> > +     }
> > +
> >       if (commit) {
> >               tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
> >               tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);