Thanks for the review, please see my acks in below.
On Wed, May 8, 2019 at 1:53 PM Yi-Hung Wei <yihung....@gmail.com> wrote:
>
> On Mon, May 6, 2019 at 2:58 PM Yifeng Sun <pkusunyif...@gmail.com> wrote:
> >
> > From: Florian Westphal <f...@strlen.de>
> >
> > Upstream Commit:
> > commit 93e66024b0249cec81e91328c55a754efd3192e0
> > Author: Florian Westphal <f...@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 <f...@strlen.de>
> > Signed-off-by: Pablo Neira Ayuso <pa...@netfilter.org>
> >
> > This patch backports the above upstream patch to OVS and fixes compiling
> > errors on RHEL kernels.
> >
> > Cc: Florian Westphal <f...@strlen.de>
> > Signed-off-by: Yifeng Sun <pkusunyif...@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?
Will fix.
>
> > ---
> > 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?
The extra space is intentional because 'struct nf_hook_state' contains
other fields of
type 'struct net_device'.
>
>
> > 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.
Will fix.
>
> 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.
Will fix.
>
>
> > 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?
In this case, we still need to replace it because nf_conntrack_in()
used in conntrack.c
is completely different after backporting.
>
> Thanks,
>
> -Yi-Hung
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
