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

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

[Xin Long]

lya 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 and 2nd patches move some common code into nf_conntrack_helper from
openvswitch so that they could be used by net/sched in the 4th patch (Note
there are still some other common code used in both OVS and TC, and I will
extract it in other patches). The 3rd patch extracts another function in
net/sched to make the 4th patch easier to write. The 4th patch adds this
feature in net/sched.

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"

v1->v2:
  - go to drop instead of return -EINVAL when fails to add SEQADJ ext in
    tcf_ct_act() as Paolo noticed.
  - add the 2nd patch to extract nf_ct_add_helper from openvswitch for
    tc act_ct use.
  - add ct exts only when the ct is not confirmed as Pablo noticed.

v2->v3:
  - fix a warning of unused variable 'err' when CONFIG_NF_NAT is disabled.

Xin Long (4):
  net: move the ct helper function to nf_conntrack_helper for ovs and tc
  net: move add ct 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 |   4 +
 include/net/tc_act/tc_ct.h                  |   1 +
 include/uapi/linux/tc_act/tc_ct.h           |   3 +
 net/netfilter/nf_conntrack_helper.c         | 102 +++++++++++++++++
 net/openvswitch/conntrack.c                 | 105 +----------------
 net/sched/act_ct.c                          | 118 ++++++++++++++++----
 6 files changed, 212 insertions(+), 121 deletions(-)

-- 
2.31.1

[PATCHv3 net-next 1/4] net: move the ct 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 c7b10234cf7c..19b5c54615c8 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.
  */
@@ -1038,7 +979,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

Re: [PATCHv3 net-next 1/4] net: move the ct helper function to nf_conntrack_helper for ovs and tc

[Aaron Conole]

Xin Long <lucien.xin@gmail.com> writes:

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

Hi Xin,

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

AFAICT, in all the places we call this we will have the nf_conn and
ip_conntrack_info already.  Maybe it makes sense to pass them here
rather than looking up again?  Originally, that information wasn't
available in the calling function - over time this has been added so we
might as well reduce the amount of "extra lookups" performed.

> +{
> +	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 c7b10234cf7c..19b5c54615c8 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.
>   */
> @@ -1038,7 +979,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;
>  		}

Re: [PATCHv3 net-next 1/4] net: move the ct helper function to nf_conntrack_helper for ovs and tc

[Xin Long]

On Wed, Nov 2, 2022 at 3:21 PM Aaron Conole <aconole@redhat.com> wrote:
>
> Xin Long <lucien.xin@gmail.com> writes:
>
> > 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>
> > ---
>
> Hi Xin,
>
> >  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)
>
> AFAICT, in all the places we call this we will have the nf_conn and
> ip_conntrack_info already.  Maybe it makes sense to pass them here
> rather than looking up again?  Originally, that information wasn't
> available in the calling function - over time this has been added so we
> might as well reduce the amount of "extra lookups" performed.
I just tried to keep nf_ct_helper() as similar as possible
to the original ovs_ct_helper().

But sure, we can also pass ct and ctinfo as the arguments,
like some other functions where it passes all of skb, ct and ctinfo.

Thanks.

>
> > +{
> > +     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 c7b10234cf7c..19b5c54615c8 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.
> >   */
> > @@ -1038,7 +979,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;
> >               }
>

[PATCHv3 net-next 2/4] net: move add ct helper function to nf_conntrack_helper for ovs and tc

[Xin Long]

Move ovs_ct_add_helper from openvswitch to nf_conntrack_helper and
rename as nf_ct_add_helper, so that it can be used in TC act_ct in
the next patch.

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

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 6c32e59fc16f..ad1adbfbeee2 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -116,6 +116,8 @@ 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);
+int nf_ct_add_helper(struct nf_conn *ct, const char *name, u8 family,
+		     u8 proto, bool nat, struct nf_conntrack_helper **hp);
 
 void nf_ct_helper_destroy(struct nf_conn *ct);
 
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 83615e479f87..1a2ab77d1bd7 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -311,6 +311,37 @@ int nf_ct_helper(struct sk_buff *skb, u16 proto)
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper);
 
+int nf_ct_add_helper(struct nf_conn *ct, const char *name, u8 family,
+		     u8 proto, bool nat, struct nf_conntrack_helper **hp)
+{
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help;
+	int ret = 0;
+
+	helper = nf_conntrack_helper_try_module_get(name, family, proto);
+	if (!helper)
+		return -EINVAL;
+
+	help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+	if (!help) {
+		nf_conntrack_helper_put(helper);
+		return -ENOMEM;
+	}
+#if IS_ENABLED(CONFIG_NF_NAT)
+	if (nat) {
+		ret = nf_nat_helper_try_module_get(name, family, proto);
+		if (ret) {
+			nf_conntrack_helper_put(helper);
+			return ret;
+		}
+	}
+#endif
+	rcu_assign_pointer(help->helper, helper);
+	*hp = helper;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nf_ct_add_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 19b5c54615c8..d37011e678c2 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1291,43 +1291,6 @@ int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
 	return 0;
 }
 
-static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
-			     const struct sw_flow_key *key, bool log)
-{
-	struct nf_conntrack_helper *helper;
-	struct nf_conn_help *help;
-	int ret = 0;
-
-	helper = nf_conntrack_helper_try_module_get(name, info->family,
-						    key->ip.proto);
-	if (!helper) {
-		OVS_NLERR(log, "Unknown helper \"%s\"", name);
-		return -EINVAL;
-	}
-
-	help = nf_ct_helper_ext_add(info->ct, GFP_KERNEL);
-	if (!help) {
-		nf_conntrack_helper_put(helper);
-		return -ENOMEM;
-	}
-
-#if IS_ENABLED(CONFIG_NF_NAT)
-	if (info->nat) {
-		ret = nf_nat_helper_try_module_get(name, info->family,
-						   key->ip.proto);
-		if (ret) {
-			nf_conntrack_helper_put(helper);
-			OVS_NLERR(log, "Failed to load \"%s\" NAT helper, error: %d",
-				  name, ret);
-			return ret;
-		}
-	}
-#endif
-	rcu_assign_pointer(help->helper, helper);
-	info->helper = helper;
-	return ret;
-}
-
 #if IS_ENABLED(CONFIG_NF_NAT)
 static int parse_nat(const struct nlattr *attr,
 		     struct ovs_conntrack_info *info, bool log)
@@ -1661,9 +1624,12 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 	}
 
 	if (helper) {
-		err = ovs_ct_add_helper(&ct_info, helper, key, log);
-		if (err)
+		err = nf_ct_add_helper(ct_info.ct, helper, ct_info.family,
+				       key->ip.proto, ct_info.nat, &ct_info.helper);
+		if (err) {
+			OVS_NLERR(log, "Failed to add %s helper %d", helper, err);
 			goto err_free_ct;
+		}
 	}
 
 	err = ovs_nla_add_action(sfa, OVS_ACTION_ATTR_CT, &ct_info,
-- 
2.31.1

[PATCHv3 net-next 3/4] 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

[PATCHv3 net-next 4/4] 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                | 83 +++++++++++++++++++++++++++++--
 3 files changed, 82 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..85f4dc5650da 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 (!nf_ct_is_confirmed(ct) && 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))
+				goto drop;
+		}
+	}
+
+	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,
@@ -1256,8 +1291,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 +1354,31 @@ 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 = nf_ct_add_helper(tmpl, name, family, proto,
+				       p->ct_action & TCA_CT_ACT_NAT, &p->helper);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to add helper");
+			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 +1547,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 +1612,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: [ovs-dev] [PATCHv3 net-next 4/4] net: sched: add helper support in act_ct

[Marcelo Ricardo Leitner]

On Mon, Oct 31, 2022 at 11:36:10AM -0400, Xin Long wrote:
...
> --- 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)

Nit, it could have fetched 'force' from p->ct_action too then, as it
is only used in this function.

There's a typo in Ilya's name in the cover letter.

Other than this, LGTM.
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [ovs-dev] [PATCHv3 net-next 4/4] net: sched: add helper support in act_ct

[Xin Long]

On Tue, Nov 1, 2022 at 11:00 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 11:36:10AM -0400, Xin Long wrote:
> ...
> > --- 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)
>
> Nit, it could have fetched 'force' from p->ct_action too then, as it
> is only used in this function.
right, we can save one variable here.

>
> There's a typo in Ilya's name in the cover letter.
Ah sorry, it was a letter missed when copying from the last cover.

>
> Other than this, LGTM.
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks!