| Message ID | 1557179808-27283-6-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | datapath: Support 4.19.x and 4.20.x kernel | expand |
On Mon, May 6, 2019 at 2:59 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > From: Florian Westphal <fw@strlen.de> > > Upstream commit: > commit 60e3be94e6a1c5162a0763c9aafb5190b2b1fdce > Author: Florian Westphal <fw@strlen.de> > Date: Mon Jun 25 17:55:32 2018 +0200 > > openvswitch: use nf_ct_get_tuplepr, invert_tuplepr > > These versions deal with the l3proto/l4proto details internally. > It removes only caller of nf_ct_get_tuple, so make it static. > > After this, l3proto->get_l4proto() can be removed in a followup patch. > > Signed-off-by: Florian Westphal <fw@strlen.de> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > This patch backports the above upstream kernel patch to OVS. > > Cc: Florian Westphal <fw@strlen.de> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > --- > acinclude.m4 | 2 ++ > datapath/conntrack.c | 17 +++-------------- > .../include/net/netfilter/nf_conntrack_core.h | 19 +++++++++++++++++++ > datapath/linux/compat/nf_conntrack_core.c | 21 +++++++++++++++++++++ > 4 files changed, 45 insertions(+), 14 deletions(-) > > diff --git a/acinclude.m4 b/acinclude.m4 > index 4f9aebc325ba..4c533bb98949 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -936,6 +936,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > [OVS_DEFINE([HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE])]) > OVS_GREP_IFELSE([$KSRC/include/net/ipv6_frag.h], [IP6_DEFRAG_CONNTRACK_IN], > [OVS_DEFINE([HAVE_IPV6_FRAG_H])]) > + OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h], > + [nf_ct_invert_tuplepr]) > > 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 52825a6b20fb..391755c25cb0 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -646,23 +646,12 @@ static struct nf_conn * > ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, > u8 l3num, struct sk_buff *skb, bool natted) > { > - const struct nf_conntrack_l3proto *l3proto; > - const struct nf_conntrack_l4proto *l4proto; > struct nf_conntrack_tuple tuple; > struct nf_conntrack_tuple_hash *h; > struct nf_conn *ct; > - unsigned int dataoff; > - u8 protonum; > > - l3proto = __nf_ct_l3proto_find(l3num); > - if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, > - &protonum) <= 0) { > - pr_debug("ovs_ct_find_existing: Can't get protonum\n"); > - return NULL; > - } > - l4proto = __nf_ct_l4proto_find(l3num, protonum); > - if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, l3num, > - protonum, net, &tuple, l3proto, l4proto)) { > + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), l3num, > + net, &tuple)) { > pr_debug("ovs_ct_find_existing: Can't get tuple\n"); > return NULL; > } > @@ -671,7 +660,7 @@ ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, > if (natted) { > struct nf_conntrack_tuple inverse; > > - if (!nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto)) { > + if (!nf_ct_invert_tuplepr(&inverse, &tuple, l3num, skb)) { The upstream patch 60e3be94e6a ("openvswitch: use nf_ct_get_tuplepr, invert_tuplepr") only gives two parameters for nf_ct_invert_tuplepr(). Any reason why we provide 4 parameters for it? @@ -632,7 +621,7 @@ ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, if (natted) { struct nf_conntrack_tuple inverse; - if (!nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto)) { + if (!nf_ct_invert_tuplepr(&inverse, &tuple)) { pr_debug("ovs_ct_find_existing: Inversion failed!\n"); return NULL; } > pr_debug("ovs_ct_find_existing: Inversion failed!\n"); > return NULL; > } > 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 d0e9aadcba76..8eba1242042b 100644 > --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h > +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h > @@ -3,6 +3,25 @@ > > #include_next <net/netfilter/nf_conntrack_core.h> > > +#ifdef HAVE_NF_CT_INVERT_TUPLEPR > + > +#include <net/netfilter/nf_conntrack.h> > + > +static inline bool > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > + const struct nf_conntrack_tuple *orig, > + u8 l3num, struct sk_buff *skb) > +{ > + return nf_ct_invert_tuplepr(inverse, orig); > +} > +#else > +bool > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > + const struct nf_conntrack_tuple *orig, > + u8 l3num, struct sk_buff *skb); > +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */ > +#define nf_ct_invert_tuplepr rpl_nf_ct_invert_tuplepr > + AFAIK, nf_ct_invert_tuplepr() was introduced in quite old kernel, and OVS kernel datapath only supports back to 3.10 kernel. I am not sure if these backports is required given that we provide the correct 2 parameters? Can you check on that? Thanks, -Yi-Hung > #ifndef HAVE_NF_CT_TMPL_ALLOC_TAKES_STRUCT_ZONE > > #include <net/netfilter/nf_conntrack_zones.h> > diff --git a/datapath/linux/compat/nf_conntrack_core.c b/datapath/linux/compat/nf_conntrack_core.c > index a7d3d4331e4a..397a5f69ffe9 100644 > --- a/datapath/linux/compat/nf_conntrack_core.c > +++ b/datapath/linux/compat/nf_conntrack_core.c > @@ -11,3 +11,24 @@ const struct nf_conntrack_zone nf_ct_zone_dflt = { > }; > > #endif /* HAVE_NF_CT_ZONE_INIT */ > + > +#ifndef HAVE_NF_CT_INVERT_TUPLEPR > +bool > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > + const struct nf_conntrack_tuple *orig, > + u8 l3num, struct sk_buff *skb) > +{ > + const struct nf_conntrack_l3proto *l3proto; > + const struct nf_conntrack_l4proto *l4proto; > + u8 protonum; > + > + l3proto = __nf_ct_l3proto_find(l3num); > + if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, > + &protonum) <= 0) { > + pr_debug("ovs_ct_find_existing: Can't get protonum\n"); > + return false; > + } > + l4proto = __nf_ct_l4proto_find(l3num, protonum); > + return nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto); > +} > +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */ > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks for reviewing, please see my acks in below. Yifeng On Wed, May 8, 2019 at 2:19 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > On Mon, May 6, 2019 at 2:59 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > > > From: Florian Westphal <fw@strlen.de> > > > > Upstream commit: > > commit 60e3be94e6a1c5162a0763c9aafb5190b2b1fdce > > Author: Florian Westphal <fw@strlen.de> > > Date: Mon Jun 25 17:55:32 2018 +0200 > > > > openvswitch: use nf_ct_get_tuplepr, invert_tuplepr > > > > These versions deal with the l3proto/l4proto details internally. > > It removes only caller of nf_ct_get_tuple, so make it static. > > > > After this, l3proto->get_l4proto() can be removed in a followup patch. > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > Acked-by: Pravin B Shelar <pshelar@ovn.org> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > This patch backports the above upstream kernel patch to OVS. > > > > Cc: Florian Westphal <fw@strlen.de> > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > --- > > acinclude.m4 | 2 ++ > > datapath/conntrack.c | 17 +++-------------- > > .../include/net/netfilter/nf_conntrack_core.h | 19 +++++++++++++++++++ > > datapath/linux/compat/nf_conntrack_core.c | 21 +++++++++++++++++++++ > > 4 files changed, 45 insertions(+), 14 deletions(-) > > > > diff --git a/acinclude.m4 b/acinclude.m4 > > index 4f9aebc325ba..4c533bb98949 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -936,6 +936,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > > [OVS_DEFINE([HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE])]) > > OVS_GREP_IFELSE([$KSRC/include/net/ipv6_frag.h], [IP6_DEFRAG_CONNTRACK_IN], > > [OVS_DEFINE([HAVE_IPV6_FRAG_H])]) > > + OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h], > > + [nf_ct_invert_tuplepr]) > > > > 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 52825a6b20fb..391755c25cb0 100644 > > --- a/datapath/conntrack.c > > +++ b/datapath/conntrack.c > > @@ -646,23 +646,12 @@ static struct nf_conn * > > ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, > > u8 l3num, struct sk_buff *skb, bool natted) > > { > > - const struct nf_conntrack_l3proto *l3proto; > > - const struct nf_conntrack_l4proto *l4proto; > > struct nf_conntrack_tuple tuple; > > struct nf_conntrack_tuple_hash *h; > > struct nf_conn *ct; > > - unsigned int dataoff; > > - u8 protonum; > > > > - l3proto = __nf_ct_l3proto_find(l3num); > > - if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, > > - &protonum) <= 0) { > > - pr_debug("ovs_ct_find_existing: Can't get protonum\n"); > > - return NULL; > > - } > > - l4proto = __nf_ct_l4proto_find(l3num, protonum); > > - if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, l3num, > > - protonum, net, &tuple, l3proto, l4proto)) { > > + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), l3num, > > + net, &tuple)) { > > pr_debug("ovs_ct_find_existing: Can't get tuple\n"); > > return NULL; > > } > > @@ -671,7 +660,7 @@ ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, > > if (natted) { > > struct nf_conntrack_tuple inverse; > > > > - if (!nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto)) { > > + if (!nf_ct_invert_tuplepr(&inverse, &tuple, l3num, skb)) { > The upstream patch 60e3be94e6a ("openvswitch: use nf_ct_get_tuplepr, > invert_tuplepr") only gives two parameters for > nf_ct_invert_tuplepr(). Any reason why we provide 4 parameters for it? The issue here is that for kernels without nf_ct_invert_tuplepr(), two parameters of nf_ct_invert_tuplepr() are not enough to implement this invert functionality, as see in rpl_nf_ct_invert_tuplepr() below. It is considered a compromise. Do you have any good idea? > > @@ -632,7 +621,7 @@ ovs_ct_find_existing(struct net *net, const struct > nf_conntrack_zone *zone, > if (natted) { > struct nf_conntrack_tuple inverse; > > - if (!nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto)) { > + if (!nf_ct_invert_tuplepr(&inverse, &tuple)) { > pr_debug("ovs_ct_find_existing: Inversion failed!\n"); > return NULL; > } > > > > > pr_debug("ovs_ct_find_existing: Inversion failed!\n"); > > return NULL; > > } > > 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 d0e9aadcba76..8eba1242042b 100644 > > --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h > > +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h > > @@ -3,6 +3,25 @@ > > > > #include_next <net/netfilter/nf_conntrack_core.h> > > > > +#ifdef HAVE_NF_CT_INVERT_TUPLEPR > > + > > +#include <net/netfilter/nf_conntrack.h> > > + > > +static inline bool > > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > > + const struct nf_conntrack_tuple *orig, > > + u8 l3num, struct sk_buff *skb) > > +{ > > + return nf_ct_invert_tuplepr(inverse, orig); > > +} > > +#else > > +bool > > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > > + const struct nf_conntrack_tuple *orig, > > + u8 l3num, struct sk_buff *skb); > > +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */ > > +#define nf_ct_invert_tuplepr rpl_nf_ct_invert_tuplepr > > + > AFAIK, nf_ct_invert_tuplepr() was introduced in quite old kernel, and > OVS kernel datapath only supports back to 3.10 kernel. I am not sure > if these backports is required given that we provide the correct 2 > parameters? Can you check on that? After checking, nf_ct_invert_tuplepr() is being used here by kernel 4.19, 4.20 and 5.0. > > Thanks, > > -Yi-Hung > > > > #ifndef HAVE_NF_CT_TMPL_ALLOC_TAKES_STRUCT_ZONE > > > > #include <net/netfilter/nf_conntrack_zones.h> > > diff --git a/datapath/linux/compat/nf_conntrack_core.c b/datapath/linux/compat/nf_conntrack_core.c > > index a7d3d4331e4a..397a5f69ffe9 100644 > > --- a/datapath/linux/compat/nf_conntrack_core.c > > +++ b/datapath/linux/compat/nf_conntrack_core.c > > @@ -11,3 +11,24 @@ const struct nf_conntrack_zone nf_ct_zone_dflt = { > > }; > > > > #endif /* HAVE_NF_CT_ZONE_INIT */ > > + > > +#ifndef HAVE_NF_CT_INVERT_TUPLEPR > > +bool > > +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, > > + const struct nf_conntrack_tuple *orig, > > + u8 l3num, struct sk_buff *skb) > > +{ > > + const struct nf_conntrack_l3proto *l3proto; > > + const struct nf_conntrack_l4proto *l4proto; > > + u8 protonum; > > + > > + l3proto = __nf_ct_l3proto_find(l3num); > > + if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, > > + &protonum) <= 0) { > > + pr_debug("ovs_ct_find_existing: Can't get protonum\n"); > > + return false; > > + } > > + l4proto = __nf_ct_l4proto_find(l3num, protonum); > > + return nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto); > > +} > > +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */ > > -- > > 2.7.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/acinclude.m4 b/acinclude.m4 index 4f9aebc325ba..4c533bb98949 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -936,6 +936,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ [OVS_DEFINE([HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE])]) OVS_GREP_IFELSE([$KSRC/include/net/ipv6_frag.h], [IP6_DEFRAG_CONNTRACK_IN], [OVS_DEFINE([HAVE_IPV6_FRAG_H])]) + OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h], + [nf_ct_invert_tuplepr]) 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 52825a6b20fb..391755c25cb0 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -646,23 +646,12 @@ static struct nf_conn * ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, u8 l3num, struct sk_buff *skb, bool natted) { - const struct nf_conntrack_l3proto *l3proto; - const struct nf_conntrack_l4proto *l4proto; struct nf_conntrack_tuple tuple; struct nf_conntrack_tuple_hash *h; struct nf_conn *ct; - unsigned int dataoff; - u8 protonum; - l3proto = __nf_ct_l3proto_find(l3num); - if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, - &protonum) <= 0) { - pr_debug("ovs_ct_find_existing: Can't get protonum\n"); - return NULL; - } - l4proto = __nf_ct_l4proto_find(l3num, protonum); - if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, l3num, - protonum, net, &tuple, l3proto, l4proto)) { + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), l3num, + net, &tuple)) { pr_debug("ovs_ct_find_existing: Can't get tuple\n"); return NULL; } @@ -671,7 +660,7 @@ ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, if (natted) { struct nf_conntrack_tuple inverse; - if (!nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto)) { + if (!nf_ct_invert_tuplepr(&inverse, &tuple, l3num, skb)) { pr_debug("ovs_ct_find_existing: Inversion failed!\n"); return NULL; } 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 d0e9aadcba76..8eba1242042b 100644 --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h @@ -3,6 +3,25 @@ #include_next <net/netfilter/nf_conntrack_core.h> +#ifdef HAVE_NF_CT_INVERT_TUPLEPR + +#include <net/netfilter/nf_conntrack.h> + +static inline bool +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, + const struct nf_conntrack_tuple *orig, + u8 l3num, struct sk_buff *skb) +{ + return nf_ct_invert_tuplepr(inverse, orig); +} +#else +bool +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, + const struct nf_conntrack_tuple *orig, + u8 l3num, struct sk_buff *skb); +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */ +#define nf_ct_invert_tuplepr rpl_nf_ct_invert_tuplepr + #ifndef HAVE_NF_CT_TMPL_ALLOC_TAKES_STRUCT_ZONE #include <net/netfilter/nf_conntrack_zones.h> diff --git a/datapath/linux/compat/nf_conntrack_core.c b/datapath/linux/compat/nf_conntrack_core.c index a7d3d4331e4a..397a5f69ffe9 100644 --- a/datapath/linux/compat/nf_conntrack_core.c +++ b/datapath/linux/compat/nf_conntrack_core.c @@ -11,3 +11,24 @@ const struct nf_conntrack_zone nf_ct_zone_dflt = { }; #endif /* HAVE_NF_CT_ZONE_INIT */ + +#ifndef HAVE_NF_CT_INVERT_TUPLEPR +bool +rpl_nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse, + const struct nf_conntrack_tuple *orig, + u8 l3num, struct sk_buff *skb) +{ + const struct nf_conntrack_l3proto *l3proto; + const struct nf_conntrack_l4proto *l4proto; + u8 protonum; + + l3proto = __nf_ct_l3proto_find(l3num); + if (l3proto->get_l4proto(skb, skb_network_offset(skb), &dataoff, + &protonum) <= 0) { + pr_debug("ovs_ct_find_existing: Can't get protonum\n"); + return false; + } + l4proto = __nf_ct_l4proto_find(l3num, protonum); + return nf_ct_invert_tuple(&inverse, &tuple, l3proto, l4proto); +} +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */