From: Arnd Bergmann <arnd@arndb.de>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Pravin Shelar <pshelar@nicira.com>,
        "David S. Miller" <davem@davemloft.net>, Thomas Graf <tgraf@suug.ch>,
        Joe Stringer <joestringer@nicira.com>, Paolo Abeni <pabeni@redhat.com>,
        Jarno Rajahalme <jarno@ovn.org>, netdev@vger.kernel.org,
        dev@openvswitch.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] openvswitch: call only into reachable nf-nat code
Date: Fri, 18 Mar 2016 13:57:46 +0100
Message-ID: <33274216.3RTMPYvo6W@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <1458132481-318209-1-git-send-email-arnd@arndb.de>
References: <1458132481-318209-1-git-send-email-arnd@arndb.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2784
Lines: 68

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