On Mon, May 6, 2019 at 2:58 PM Yifeng Sun <[email protected]> wrote:
>
> From: Florian Westphal <[email protected]>
>
> Upstream Commit:
> commit 93e66024b0249cec81e91328c55a754efd3192e0
> Author: Florian Westphal <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Pablo Neira Ayuso <[email protected]>
>
> This patch backports the above upstream patch to OVS and fixes compiling
> errors on RHEL kernels.
>
> Cc: Florian Westphal <[email protected]>
> Signed-off-by: Yifeng Sun <[email protected]>
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
