[ovs-dev] [PATCH 2/7] datapath: Pass nf_hook_state to nf_conntrack_in

Yi-Hung Wei yihung.wei at gmail.com
Wed May 8 20:53:19 UTC 2019


On Mon, May 6, 2019 at 2:58 PM Yifeng Sun <pkusunyifeng at gmail.com> wrote:
>
> From: Florian Westphal <fw at strlen.de>
>
> Upstream Commit:
>     commit 93e66024b0249cec81e91328c55a754efd3192e0
>     Author: Florian Westphal <fw at strlen.de>
>     Date:   Wed Sep 12 15:19:07 2018 +0200
>
>     netfilter: conntrack: pass nf_hook_state to packet and error handlers
>
>     nf_hook_state contains all the hook meta-information: netns, protocol family,
>     hook location, and so on.
>
>     Instead of only passing selected information, pass a pointer to entire
>     structure.
>
>     This will allow to merge the error and the packet handlers and remove
>     the ->new() function in followup patches.
>
>     Signed-off-by: Florian Westphal <fw at strlen.de>
>     Signed-off-by: Pablo Neira Ayuso <pablo at netfilter.org>
>
> This patch backports the above upstream patch to OVS and fixes compiling
> errors on RHEL kernels.
>
> Cc: Florian Westphal <fw at strlen.de>
> Signed-off-by: Yifeng Sun <pkusunyifeng at gmail.com>

Thanks for the patch. It looks good in general. I have some minor
comments below.

I think it might be a little bit clear to have nf_conntrack_in() than
nf_conntack_in in the title?

> ---
>  acinclude.m4                                          |  5 +++++
>  datapath/conntrack.c                                  |  8 ++++++--
>  datapath/linux/Modules.mk                             |  4 +++-
>  datapath/linux/compat/include/linux/netfilter.h       | 19 +++++++++++++++++++
>  .../compat/include/net/netfilter/nf_conntrack_core.h  | 11 +++++++++++
>  5 files changed, 44 insertions(+), 3 deletions(-)
>  create mode 100644 datapath/linux/compat/include/linux/netfilter.h
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index c9b744db0b94..372be5f4dccd 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -603,6 +603,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>                          [ndo_change_mtu], [OVS_DEFINE([HAVE_RHEL7_MAX_MTU])])
>
>    OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [nf_hook_state])
> +  OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/netfilter.h], [nf_hook_state],
> +                        [struct net ], [OVS_DEFINE([HAVE_NF_HOOK_STATE_NET])])
Looks like there's an extra space after struct net?


>    OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [nf_register_net_hook])
>    OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [nf_hookfn.*nf_hook_ops],
>                    [OVS_DEFINE([HAVE_NF_HOOKFN_ARG_OPS])])
> @@ -929,6 +931,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>    OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_l3proto.h],
>                    [nf_conntrack_l3proto],
>                    [OVS_DEFINE([HAVE_NF_CONNTRACK_L3PROATO_H])])
> +  OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_core.h],
> +                        [nf_conntrack_in], [nf_hook_state],
> +                        [OVS_DEFINE([HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE])])
>
>    if cmp -s datapath/linux/kcompat.h.new \
>              datapath/linux/kcompat.h >/dev/null 2>&1; then
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 52208bad3029..8c1a80308d6a 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -987,6 +987,11 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>         struct nf_conn *ct;
>
>         if (!cached) {
> +               struct nf_hook_state state = {
> +                       .hook = NF_INET_PRE_ROUTING,
> +                       .pf = info->family,
> +                       .net = net,
> +               };
>                 struct nf_conn *tmpl = info->ct;
>                 int err;
>
> @@ -998,8 +1003,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>                         nf_ct_set(skb, tmpl, IP_CT_NEW);
>                 }
>
> -               err = nf_conntrack_in(net, info->family,
> -                                     NF_INET_PRE_ROUTING, skb);
> +               err = nf_conntrack_in(skb, &state);
>                 if (err != NF_ACCEPT)
>                         return -ENOENT;
>
> diff --git a/datapath/linux/Modules.mk b/datapath/linux/Modules.mk
> index caa2525ff0ab..e8483385dcbb 100644
> --- a/datapath/linux/Modules.mk
> +++ b/datapath/linux/Modules.mk
> @@ -114,5 +114,7 @@ openvswitch_headers += \
>         linux/compat/include/net/erspan.h \
>         linux/compat/include/uapi/linux/netfilter.h \
>         linux/compat/include/linux/mm.h \
> -       linux/compat/include/linux/overflow.h
> +       linux/compat/include/linux/overflow.h \
> +       linux/compat/include/net/ipv6_frag.h \
> +       linux/compat/include/linux/netfilter.h
Should  linux/compat/include/net/ipv6_frag.h be added when ipv6_frag.h
backport is introduced? We can either move this line to the next patch
or squash the next patch into this one.

Also, it would be better to list the header files in alphabetical
order.  Looks like mm.h and overflow.h is out of order tho.


> diff --git a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> index 7834c8c25f79..d0e9aadcba76 100644
> --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> @@ -104,4 +104,15 @@ static inline bool rpl_nf_ct_delete(struct nf_conn *ct, u32 portid, int report)
>  #define nf_ct_delete rpl_nf_ct_delete
>  #endif /* HAVE_NF_CONN_TIMER */
>
> +static inline unsigned int
> +rpl_nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
> +{
> +#ifdef HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE
> +       return nf_conntrack_in(skb, state);
> +#else
> +       return nf_conntrack_in(state->net, state->pf, state->hook, skb);
> +#endif /* HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE */
> +}
> +#define nf_conntrack_in rpl_nf_conntrack_in
> +
>  #endif /* _NF_CONNTRACK_CORE_WRAPPER_H */
> --
Should we replace nf_conntrack_in() when
HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE is not defined?

Thanks,

-Yi-Hung


More information about the dev mailing list