| Message ID | 1458132481-318209-1-git-send-email-arnd@arndb.de |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Mar 16, 2016 at 01:47:13PM +0100, Arnd Bergmann wrote: > The openvswitch code has gained support for calling into the > nf-nat-ipv4/ipv6 modules, however those can be loadable modules > in a configuration in which openvswitch is built-in, leading > to link errors: > > net/built-in.o: In function `__ovs_ct_lookup': > :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation' > :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation' > > The dependency on (!NF_NAT || NF_NAT) was meant to prevent > this, but NF_NAT is set to 'y' if any of the symbols selecting > it are built-in, but the link error happens when any of them > are modular. > > A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in, > CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely > to be useful in practice, but the driver currently only handles > IPv6 being optional. > > This patch improves the Kconfig dependency so that openvswitch > cannot be built-in if either of the two other symbols are set > to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute() > with two "if (IS_ENABLED())" checks that should catch all corner > cases also make the code more readable. > > The same #ifdef exists ovs_ct_nat_to_attr(), where it does not > cause a link error, but for consistency I'm changing it the same > way. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 05752523e565 ("openvswitch: Interface with NAT.") > --- > net/openvswitch/Kconfig | 3 ++- > net/openvswitch/conntrack.c | 16 ++++++++-------- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig > index 234a73344c6e..961fb60115df 100644 > --- a/net/openvswitch/Kconfig > +++ b/net/openvswitch/Kconfig > @@ -7,7 +7,8 @@ config OPENVSWITCH > depends on INET > depends on !NF_CONNTRACK || \ > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > - (!NF_NAT || NF_NAT))) > + (!NF_NAT_IPV4 || NF_NAT_IPV4) && \ > + (!NF_NAT_IPV6 || NF_NAT_IPV6))) Not related with this patch, just a side note/recommendation. I understand this code just got into tree, and that this needs a bit work/iterations but this thing above is ugly, I wonder if there is a better way to avoid this. Probably with some modularization of the openvswitch code this will look better, I mean: 1) adding Kconfig switches to enable conntrack and NAT support to net/openvswitch/Kconfig. 2) Move the NAT code to the corresponding openvswitch/nat.c file. Just my two cents.
On Wednesday 16 March 2016 14:25:36 Pablo Neira Ayuso wrote: > Not related with this patch, just a side note/recommendation. > > I understand this code just got into tree, and that this needs a bit > work/iterations but this thing above is ugly, I wonder if there is a > better way to avoid this. > > Probably with some modularization of the openvswitch code this will > look better, I mean: > > 1) adding Kconfig switches to enable conntrack and NAT support to > net/openvswitch/Kconfig. > > 2) Move the NAT code to the corresponding openvswitch/nat.c file. > > Just my two cents. Yes, I think that would be good too. I also found that the driver used to look like that but it was changed as part of f88f69dd17f1 ("openvswitch: Remove conntrack Kconfig option."). Arnd
On 17 March 2016 at 02:56, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 16 March 2016 14:25:36 Pablo Neira Ayuso wrote: >> Not related with this patch, just a side note/recommendation. >> >> I understand this code just got into tree, and that this needs a bit >> work/iterations but this thing above is ugly, I wonder if there is a >> better way to avoid this. >> >> Probably with some modularization of the openvswitch code this will >> look better, I mean: >> >> 1) adding Kconfig switches to enable conntrack and NAT support to >> net/openvswitch/Kconfig. >> >> 2) Move the NAT code to the corresponding openvswitch/nat.c file. >> >> Just my two cents. > > Yes, I think that would be good too. I also found that the driver > used to look like that but it was changed as part of f88f69dd17f1 > ("openvswitch: Remove conntrack Kconfig option."). I don't see how it helps to separate the conntrack, nat pieces into different Kconfig switches unless you're splitting their code completely out of the openvswitch module. Prior to f88f69dd17f1, OPENVSWITCH Kconfig option had no dependency on NF_CONNTRACK. OPENVSWITCH_CONNTRACK was a switch to build those pieces into the OPENVSWITCH module (ie, "openvswitch-$(CONFIG_OPENVSWITCH_CONNTRACK) += conntrack.o"). If you configured NF_CONNTRACK=m, OPENVSWITCH=y, OPENVSWITCH_CONNTRACK=y then it would break in the same kind of way as the bug that Arnd is reporting. So, that patch was introduced to shift the dependency up to the OPENVSWITCH kconfig option. At that point, there was no strong benefit to keeping the conntrack separately configurable, so that was removed. Further splitting the code out in some way (eg new modules) seemed way overkill to tidy up some ugly-looking dependency logic. Maybe someone with better Kconfig-fu than me can point out a better way.
On 17 March 2016 at 01:47, Arnd Bergmann <arnd@arndb.de> wrote: > The openvswitch code has gained support for calling into the > nf-nat-ipv4/ipv6 modules, however those can be loadable modules > in a configuration in which openvswitch is built-in, leading > to link errors: > > net/built-in.o: In function `__ovs_ct_lookup': > :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation' > :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation' > > The dependency on (!NF_NAT || NF_NAT) was meant to prevent > this, but NF_NAT is set to 'y' if any of the symbols selecting > it are built-in, but the link error happens when any of them > are modular. > > A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in, > CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely > to be useful in practice, but the driver currently only handles > IPv6 being optional. > > This patch improves the Kconfig dependency so that openvswitch > cannot be built-in if either of the two other symbols are set > to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute() > with two "if (IS_ENABLED())" checks that should catch all corner > cases also make the code more readable. > > The same #ifdef exists ovs_ct_nat_to_attr(), where it does not > cause a link error, but for consistency I'm changing it the same > way. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 05752523e565 ("openvswitch: Interface with NAT.") Thanks for the fix, LGTM. Acked-by: Joe Stringer <joe@ovn.org>
On Wednesday 16 March 2016 13:47:13 Arnd Bergmann wrote: > The openvswitch code has gained support for calling into the > nf-nat-ipv4/ipv6 modules, however those can be loadable modules > in a configuration in which openvswitch is built-in, leading > to link errors: > > net/built-in.o: In function `__ovs_ct_lookup': > :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation' > :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation' > > The dependency on (!NF_NAT || NF_NAT) was meant to prevent > this, but NF_NAT is set to 'y' if any of the symbols selecting > it are built-in, but the link error happens when any of them > are modular. > > A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in, > CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely > to be useful in practice, but the driver currently only handles > IPv6 being optional. > > This patch improves the Kconfig dependency so that openvswitch > cannot be built-in if either of the two other symbols are set > to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute() > with two "if (IS_ENABLED())" checks that should catch all corner > cases also make the code more readable. > > The same #ifdef exists ovs_ct_nat_to_attr(), where it does not > cause a link error, but for consistency I'm changing it the same > way. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: 05752523e565 ("openvswitch: Interface with NAT.") > --- > net/openvswitch/Kconfig | 3 ++- > net/openvswitch/conntrack.c | 16 ++++++++-------- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig > index 234a73344c6e..961fb60115df 100644 > --- a/net/openvswitch/Kconfig > +++ b/net/openvswitch/Kconfig > @@ -7,7 +7,8 @@ config OPENVSWITCH > depends on INET > depends on !NF_CONNTRACK || \ > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > - (!NF_NAT || NF_NAT))) > + (!NF_NAT_IPV4 || NF_NAT_IPV4) && \ > + (!NF_NAT_IPV6 || NF_NAT_IPV6))) > select LIBCRC32C > select MPLS > select NET_MPLS_GSO I've now seen a new regression: net/built-in.o: In function `__ovs_ct_lookup': switchdev.c:(.text+0x136e8c): undefined reference to `nf_ct_nat_ext_add' switchdev.c:(.text+0x136f38): undefined reference to `nf_nat_packet' switchdev.c:(.text+0x136f80): undefined reference to `nf_nat_setup_info' switchdev.c:(.text+0x136f98): undefined reference to `nf_nat_alloc_null_binding' Apparently, the (!NF_NAT || NF_NAT) statement is also needed in addition to the other two. I'm resending the fixed patch, as it doesn't seem to have been merged yet. If it's in some other tree already and you'd rather have a patch on top, I can send that too. Arnd
diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig index 234a73344c6e..961fb60115df 100644 --- a/net/openvswitch/Kconfig +++ b/net/openvswitch/Kconfig @@ -7,7 +7,8 @@ config OPENVSWITCH depends on INET depends on !NF_CONNTRACK || \ (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ - (!NF_NAT || NF_NAT))) + (!NF_NAT_IPV4 || NF_NAT_IPV4) && \ + (!NF_NAT_IPV6 || NF_NAT_IPV6))) select LIBCRC32C select MPLS select NET_MPLS_GSO diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index dc5eb29fe7d6..ff04b5db04b3 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -535,14 +535,15 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct, switch (ctinfo) { case IP_CT_RELATED: case IP_CT_RELATED_REPLY: - if (skb->protocol == htons(ETH_P_IP) && + if (IS_ENABLED(CONFIG_NF_NAT_IPV4) && + skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->protocol == IPPROTO_ICMP) { if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo, hooknum)) err = NF_DROP; goto push; -#if IS_ENABLED(CONFIG_NF_NAT_IPV6) - } else if (skb->protocol == htons(ETH_P_IPV6)) { + } else if (IS_ENABLED(CONFIG_NF_NAT_IPV6) && + skb->protocol == htons(ETH_P_IPV6)) { __be16 frag_off; u8 nexthdr = ipv6_hdr(skb)->nexthdr; int hdrlen = ipv6_skip_exthdr(skb, @@ -557,7 +558,6 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct, err = NF_DROP; goto push; } -#endif } /* Non-ICMP, fall thru to initialize if needed. */ case IP_CT_NEW: @@ -1238,7 +1238,8 @@ static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info, } if (info->range.flags & NF_NAT_RANGE_MAP_IPS) { - if (info->family == NFPROTO_IPV4) { + if (IS_ENABLED(CONFIG_NF_NAT_IPV4) && + info->family == NFPROTO_IPV4) { if (nla_put_in_addr(skb, OVS_NAT_ATTR_IP_MIN, info->range.min_addr.ip) || (info->range.max_addr.ip @@ -1246,8 +1247,8 @@ static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info, (nla_put_in_addr(skb, OVS_NAT_ATTR_IP_MAX, info->range.max_addr.ip)))) return false; -#if IS_ENABLED(CONFIG_NF_NAT_IPV6) - } else if (info->family == NFPROTO_IPV6) { + } else if (IS_ENABLED(CONFIG_NF_NAT_IPV6) && + info->family == NFPROTO_IPV6) { if (nla_put_in6_addr(skb, OVS_NAT_ATTR_IP_MIN, &info->range.min_addr.in6) || (memcmp(&info->range.max_addr.in6, @@ -1256,7 +1257,6 @@ static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info, (nla_put_in6_addr(skb, OVS_NAT_ATTR_IP_MAX, &info->range.max_addr.in6)))) return false; -#endif } else { return false; }
The openvswitch code has gained support for calling into the nf-nat-ipv4/ipv6 modules, however those can be loadable modules in a configuration in which openvswitch is built-in, leading to link errors: net/built-in.o: In function `__ovs_ct_lookup': :(.text+0x2cc2c8): undefined reference to `nf_nat_icmp_reply_translation' :(.text+0x2cc66c): undefined reference to `nf_nat_icmpv6_reply_translation' The dependency on (!NF_NAT || NF_NAT) was meant to prevent this, but NF_NAT is set to 'y' if any of the symbols selecting it are built-in, but the link error happens when any of them are modular. A second issue is that even if CONFIG_NF_NAT_IPV6 is built-in, CONFIG_NF_NAT_IPV4 might be completely disabled. This is unlikely to be useful in practice, but the driver currently only handles IPv6 being optional. This patch improves the Kconfig dependency so that openvswitch cannot be built-in if either of the two other symbols are set to 'm', and it replaces the incorrect #ifdef in ovs_ct_nat_execute() with two "if (IS_ENABLED())" checks that should catch all corner cases also make the code more readable. The same #ifdef exists ovs_ct_nat_to_attr(), where it does not cause a link error, but for consistency I'm changing it the same way. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 05752523e565 ("openvswitch: Interface with NAT.") --- net/openvswitch/Kconfig | 3 ++- net/openvswitch/conntrack.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-)