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

[Aaron Conole <aconole@redhat.com>]

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