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

[PATCH net-next 0/2] net: sched: add helper support in 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) was installed in the OVS kernel module, which was
caused by that TC didn't support the ftp helper offloaded from OVS.

This patchset is to add the helper support for ovs offloading. Note that
the 1st patch is fixing a memleak issue, and also making the 2nd patch
easier to write for the feature support.

Xin Long (2):
  net: sched: fix the err path of tcf_ct_init in act_ct
  net: sched: add helper support in act_ct

 include/net/tc_act/tc_ct.h        |   1 +
 include/uapi/linux/tc_act/tc_ct.h |   3 +
 net/sched/act_ct.c                | 193 +++++++++++++++++++++++++++---
 3 files changed, 182 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH net-next 1/2] net: sched: fix the err path of tcf_ct_init in act_ct

[Xin Long]

When it returns err from tcf_ct_flow_table_get(), the param tmpl should
have been freed in the cleanup. Otherwise a memory leak will occur.

While fixing this problem, this patch also makes 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.

Fixes: c34b961a2492 ("net/sched: act_ct: Create nf flow table per zone")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_ct.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 9d19710835b0..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.
@@ -1401,14 +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:
 	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;
 }
@@ -1420,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 2/2] 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.

Note when calling helper->help() in tcf_ct_act(), the packet will be
dropped if skb's family and proto do not match the helper's.

Reported-by: Ilya Maximets <i.maximets@ovn.org>
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                | 163 +++++++++++++++++++++++++++++-
 3 files changed, 165 insertions(+), 2 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..771cf72ee9e1 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;
@@ -832,6 +833,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)
@@ -1022,6 +1030,69 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
 #endif
 }
 
+static int tcf_ct_helper(struct sk_buff *skb, u8 family)
+{
+	const struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	enum ip_conntrack_info ctinfo;
+	unsigned int protoff;
+	struct nf_conn *ct;
+	u8 proto;
+	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 != family)
+		return NF_DROP;
+
+	switch (family) {
+	case NFPROTO_IPV4:
+		protoff = ip_hdrlen(skb);
+		proto = ip_hdr(skb)->protocol;
+		break;
+	case NFPROTO_IPV6: {
+		__be16 frag_off;
+		int ofs;
+
+		proto = ipv6_hdr(skb)->nexthdr;
+		ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &proto, &frag_off);
+		if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
+			pr_debug("proto header not found\n");
+			return NF_DROP;
+		}
+		protoff = ofs;
+		break;
+	}
+	default:
+		WARN_ONCE(1, "helper invoked on non-IP family!");
+		return NF_DROP;
+	}
+
+	if (helper->tuple.dst.protonum != proto)
+		return NF_DROP;
+
+	err = helper->help(skb, protoff, ct, ctinfo);
+	if (err != NF_ACCEPT)
+		return NF_DROP;
+
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
+		return NF_DROP;
+
+	return NF_ACCEPT;
+}
+
 static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		      struct tcf_result *res)
 {
@@ -1033,6 +1104,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;
@@ -1119,6 +1191,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 || add_helper) : commit) {
+		if (tcf_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 +1255,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 +1339,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 +1380,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 +1443,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 +1633,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 +1698,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 2/2] net: sched: add helper support in act_ct

[Paolo Abeni]

On Fri, 2022-09-23 at 11:28 -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.
> 
> Note when calling helper->help() in tcf_ct_act(), the packet will be
> dropped if skb's family and proto do not match the helper's.
> 
> Reported-by: Ilya Maximets <i.maximets@ovn.org>

This tag is a bit out of place here, as it should belong to fixes. Do
you mean 'Suggested-by' ?

> 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                | 163 +++++++++++++++++++++++++++++-
>  3 files changed, 165 insertions(+), 2 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..771cf72ee9e1 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;
> @@ -832,6 +833,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)
> @@ -1022,6 +1030,69 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>  #endif
>  }
>  
> +static int tcf_ct_helper(struct sk_buff *skb, u8 family)
> +{

This is very similar to ovs_ct_helper(), I'm wondering if a common
helper could be factored out?

> +	const struct nf_conntrack_helper *helper;
> +	const struct nf_conn_help *help;
> +	enum ip_conntrack_info ctinfo;
> +	unsigned int protoff;
> +	struct nf_conn *ct;
> +	u8 proto;
> +	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 != family)
> +		return NF_DROP;
> +
> +	switch (family) {
> +	case NFPROTO_IPV4:
> +		protoff = ip_hdrlen(skb);
> +		proto = ip_hdr(skb)->protocol;
> +		break;
> +	case NFPROTO_IPV6: {
> +		__be16 frag_off;
> +		int ofs;
> +
> +		proto = ipv6_hdr(skb)->nexthdr;
> +		ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &proto, &frag_off);
> +		if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
> +			pr_debug("proto header not found\n");
> +			return NF_DROP;

Why this is returning NF_DROP while ovs_ct_helper() returns NF_ACCEPT
here?

> +		}
> +		protoff = ofs;
> +		break;
> +	}
> +	default:
> +		WARN_ONCE(1, "helper invoked on non-IP family!");
> +		return NF_DROP;
> +	}
> +
> +	if (helper->tuple.dst.protonum != proto)
> +		return NF_DROP;

I'm wondering if NF_DROP is appropriate here. This should be a 
situation similar to the above one: the current packet does not match
the helper.

Thanks!

Paolo

Re: [PATCH net-next 1/2] net: sched: fix the err path of tcf_ct_init in act_ct

[Paolo Abeni]

On Fri, 2022-09-23 at 11:28 -0400, Xin Long wrote:
> When it returns err from tcf_ct_flow_table_get(), the param tmpl should
> have been freed in the cleanup. Otherwise a memory leak will occur.
> 
> While fixing this problem, this patch also makes 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.
> 
> Fixes: c34b961a2492 ("net/sched: act_ct: Create nf flow table per zone")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

I think it's better if you re-submit this patch for -net explicitly, as
it LGTM and makes sense to let it reach the affected kernel soon.

Thanks!

Paolo

Re: [PATCH net-next 1/2] net: sched: fix the err path of tcf_ct_init in act_ct

[Xin Long]

On Tue, Sep 27, 2022 at 8:43 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2022-09-23 at 11:28 -0400, Xin Long wrote:
> > When it returns err from tcf_ct_flow_table_get(), the param tmpl should
> > have been freed in the cleanup. Otherwise a memory leak will occur.
> >
> > While fixing this problem, this patch also makes 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.
> >
> > Fixes: c34b961a2492 ("net/sched: act_ct: Create nf flow table per zone")
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> I think it's better if you re-submit this patch for -net explicitly, as
> it LGTM and makes sense to let it reach the affected kernel soon.
If so, I will have to wait until this patch is merged on net-next,
then post the other one for net-next, right?

>
> Thanks!
>
> Paolo
>
>

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

[Xin Long]

On Tue, Sep 27, 2022 at 6:29 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2022-09-23 at 11:28 -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.
> >
> > Note when calling helper->help() in tcf_ct_act(), the packet will be
> > dropped if skb's family and proto do not match the helper's.
> >
> > Reported-by: Ilya Maximets <i.maximets@ovn.org>
>
> This tag is a bit out of place here, as it should belong to fixes. Do
> you mean 'Suggested-by' ?
This one was reported as an OVS bug, but from TC side, it's a feature.
I will remove it.

>
> > 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                | 163 +++++++++++++++++++++++++++++-
> >  3 files changed, 165 insertions(+), 2 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..771cf72ee9e1 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;
> > @@ -832,6 +833,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)
> > @@ -1022,6 +1030,69 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
> >  #endif
> >  }
> >
> > +static int tcf_ct_helper(struct sk_buff *skb, u8 family)
> > +{
>
> This is very similar to ovs_ct_helper(), I'm wondering if a common
> helper could be factored out?
I wanted to, but these are two modules, I don't expect one depends another.
Although this is for OVS offloading, but it can still be used independently.
maybe I should move this function to nf_conntrack_helper.c in netfilter?

>
> > +     const struct nf_conntrack_helper *helper;
> > +     const struct nf_conn_help *help;
> > +     enum ip_conntrack_info ctinfo;
> > +     unsigned int protoff;
> > +     struct nf_conn *ct;
> > +     u8 proto;
> > +     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 != family)
> > +             return NF_DROP;
> > +
> > +     switch (family) {
> > +     case NFPROTO_IPV4:
> > +             protoff = ip_hdrlen(skb);
> > +             proto = ip_hdr(skb)->protocol;
> > +             break;
> > +     case NFPROTO_IPV6: {
> > +             __be16 frag_off;
> > +             int ofs;
> > +
> > +             proto = ipv6_hdr(skb)->nexthdr;
> > +             ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &proto, &frag_off);
> > +             if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
> > +                     pr_debug("proto header not found\n");
> > +                     return NF_DROP;
>
> Why this is returning NF_DROP while ovs_ct_helper() returns NF_ACCEPT
> here?
>
> > +             }
> > +             protoff = ofs;
> > +             break;
> > +     }
> > +     default:
> > +             WARN_ONCE(1, "helper invoked on non-IP family!");
> > +             return NF_DROP;
> > +     }
> > +
> > +     if (helper->tuple.dst.protonum != proto)
> > +             return NF_DROP;
>
> I'm wondering if NF_DROP is appropriate here. This should be a
> situation similar to the above one: the current packet does not match
> the helper.
I was thinking the packets arriving here should be matched with the helper's,
this would force users do a right configuration on flower or other match. But
this might be too harsh and will change to NF_ACCEPT.

Thanks!

>
> Thanks!
>
> Paolo
>

Re: [PATCH net-next 1/2] net: sched: fix the err path of tcf_ct_init in act_ct

[Paolo Abeni]

On Tue, 2022-09-27 at 10:45 -0400, Xin Long wrote:
> On Tue, Sep 27, 2022 at 8:43 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Fri, 2022-09-23 at 11:28 -0400, Xin Long wrote:
> > > When it returns err from tcf_ct_flow_table_get(), the param tmpl should
> > > have been freed in the cleanup. Otherwise a memory leak will occur.
> > > 
> > > While fixing this problem, this patch also makes 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.
> > > 
> > > Fixes: c34b961a2492 ("net/sched: act_ct: Create nf flow table per zone")
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > 
> > I think it's better if you re-submit this patch for -net explicitly, as
> > it LGTM and makes sense to let it reach the affected kernel soon.
> If so, I will have to wait until this patch is merged on net-next,
> then post the other one for net-next, right?

Yes. Note that such merge happens once per week...

Cheers,

Paolo

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

[Ilya Maximets]

On 9/27/22 17:04, Xin Long wrote:
> On Tue, Sep 27, 2022 at 6:29 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> On Fri, 2022-09-23 at 11:28 -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.
>>>
>>> Note when calling helper->help() in tcf_ct_act(), the packet will be
>>> dropped if skb's family and proto do not match the helper's.
>>>
>>> Reported-by: Ilya Maximets <i.maximets@ovn.org>
>>
>> This tag is a bit out of place here, as it should belong to fixes. Do
>> you mean 'Suggested-by' ?
> This one was reported as an OVS bug, but from TC side, it's a feature.

My 2c:
- The fact that act_ct doesn't execute helpers attached to skb outside
  of TC (in OVS) can be considered as a bug.
- The ability to set helpers in act_ct itself is indeed a new feature.

Though it was decided to implement both things at the same time to
avoid confusion around what is supported and what is not supported,
especially since there will be no meaningful way to detect if the bug
actually fixed in the kernel or not.

CC: Eelco.

P.S. might also make sense to CC: ovs-dev on a next revision for visibility.

Best regards, Ilya Maximets.