build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Jan Beulich]

With BRIDGE=y and IPV6=m commit
fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
link-local address for multicast listener queries") causes the build to
break.

Similary, even if both are =m, but ipv6.ko got blacklisted (as is
happening in various SuSE distros when disabling IPv6), there's
a runtime problem since bridge.ko then won't load anymore due
to the missing symbol.

I note that infiniband appears to have had a similar problem
(didn't verify whether they have other dependencies on ipv6.ko),
resolved by an odd looking dependency of INFINIBAND_ADDR_TRANS
on !(INFINIBAND = y && IPV6 = m).

IP_VS also seems to have a similar issue.

Resolving this the infiniband way seems rather undesirable to me.
Instead I would think that this and any similar dependency should
get resolved via placing a stub pointer in net/ipv6/addrconf_core.c
(for this particular case), and having ipv6.ko set the pointer to the
actual implementation.

Otherwise, namely in distro kernels, pure IPv4 environments
pointlessly load the (huge) ipv6.ko just to satisfy symbol
references that would never get called.

One unrelated other observation with this change of yours:
daddr is an input argument to ipv6_dev_get_saddr(), yet
it gets initialized only after the function was called. Is that
really correct?

Thanks, Jan

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Stephen Hemminger]

On Wed, 16 Mar 2011 12:34:19 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> With BRIDGE=y and IPV6=m commit
> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
> link-local address for multicast listener queries") causes the build to
> break.

Rather than continue with the config games, lets just make the necessary
ipv6 pieces accessible.

-- 

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Stephen Hemminger]

On Wed, 16 Mar 2011 12:34:19 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> With BRIDGE=y and IPV6=m commit
> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
> link-local address for multicast listener queries") causes the build to
> break.

I have no problem building with 2.6.38 and BRIDGE=y and IPV6=m.
Blacklisting ipv6 module is just self inflicted pain, don't do it.

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Brian Haley]

On 03/16/2011 08:34 AM, Jan Beulich wrote:
> With BRIDGE=y and IPV6=m commit
> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
> link-local address for multicast listener queries") causes the build to
> break.
> 
> Similary, even if both are =m, but ipv6.ko got blacklisted (as is
> happening in various SuSE distros when disabling IPv6), there's
> a runtime problem since bridge.ko then won't load anymore due
> to the missing symbol.

Load the ipv6 module with disable=1, which is why I added it :)

>From Documentation/networking/ipv6.txt:

disable

        Specifies whether to load the IPv6 module, but disable all
        its functionality.  This might be used when another module
        has a dependency on the IPv6 module being loaded, but no
        IPv6 addresses or operations are desired.

        The possible values and their effects are:

        0
                IPv6 is enabled.

                This is the default value.

        1
                IPv6 is disabled.

                No IPv6 addresses will be added to interfaces, and
                it will not be possible to open an IPv6 socket.

                A reboot is required to enable IPv6.

-Brian

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 16 Mar 2011 08:24:41 -0700

> On Wed, 16 Mar 2011 12:34:19 +0000
> "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> With BRIDGE=y and IPV6=m commit
>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>> link-local address for multicast listener queries") causes the build to
>> break.
> 
> Rather than continue with the config games, lets just make the necessary
> ipv6 pieces accessible.

You can't Stephen, ipv6_dev_get_saddr() requires access to the actual ipv6
device state, that means you have to pull in the entire ipv6 stack in because
there are dependencies all the way down into the routing code.

We added a Kconfig fix to cure this specific problem, which made it
into 2.6.38-final, so I don't understand why Jan is even seeing this,
it's supposed to force BRIDGE modular if IPV6 is modular:

commit dcbcdf22f500ac6e4ec06485341024739b9dc241
Author: Randy Dunlap <randy.dunlap@oracle.com>
Date:   Thu Mar 10 13:45:57 2011 -0800

    net: bridge builtin vs. ipv6 modular
    
    When configs BRIDGE=y and IPV6=m, this build error occurs:
    
    br_multicast.c:(.text+0xa3341): undefined reference to `ipv6_dev_get_saddr'
    
    BRIDGE_IGMP_SNOOPING is boolean; if it were tristate, then adding
    	depends on IPV6 || IPV6=n
    to BRIDGE_IGMP_SNOOPING would be a good fix.  As it is currently,
    making BRIDGE depend on the IPV6 config works.
    
    Reported-by: Patrick Schaaf <netdev@bof.de>
    Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
index 9190ae4..6dee7bf 100644
--- a/net/bridge/Kconfig
+++ b/net/bridge/Kconfig
@@ -6,6 +6,7 @@ config BRIDGE
 	tristate "802.1d Ethernet Bridging"
 	select LLC
 	select STP
+	depends on IPV6 || IPV6=n
 	---help---
 	  If you say Y here, then your Linux box will be able to act as an
 	  Ethernet bridge, which means that the different Ethernet segments it

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[David Miller]

From: "Jan Beulich" <JBeulich@novell.com>
Date: Wed, 16 Mar 2011 12:34:19 +0000

> With BRIDGE=y and IPV6=m commit
> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
> link-local address for multicast listener queries") causes the build to
> break.

Fixed by:

commit dcbcdf22f500ac6e4ec06485341024739b9dc241
Author: Randy Dunlap <randy.dunlap@oracle.com>
Date:   Thu Mar 10 13:45:57 2011 -0800

    net: bridge builtin vs. ipv6 modular
    
    When configs BRIDGE=y and IPV6=m, this build error occurs:
    
    br_multicast.c:(.text+0xa3341): undefined reference to `ipv6_dev_get_saddr'
    
    BRIDGE_IGMP_SNOOPING is boolean; if it were tristate, then adding
    	depends on IPV6 || IPV6=n
    to BRIDGE_IGMP_SNOOPING would be a good fix.  As it is currently,
    making BRIDGE depend on the IPV6 config works.
    
    Reported-by: Patrick Schaaf <netdev@bof.de>
    Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
index 9190ae4..6dee7bf 100644
--- a/net/bridge/Kconfig
+++ b/net/bridge/Kconfig
@@ -6,6 +6,7 @@ config BRIDGE
 	tristate "802.1d Ethernet Bridging"
 	select LLC
 	select STP
+	depends on IPV6 || IPV6=n
 	---help---
 	  If you say Y here, then your Linux box will be able to act as an
 	  Ethernet bridge, which means that the different Ethernet segments it

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Jan Beulich]

>>> On 16.03.11 at 18:49, David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Wed, 16 Mar 2011 08:24:41 -0700
> 
>> On Wed, 16 Mar 2011 12:34:19 +0000
>> "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>> With BRIDGE=y and IPV6=m commit
>>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>>> link-local address for multicast listener queries") causes the build to
>>> break.
>> 
>> Rather than continue with the config games, lets just make the necessary
>> ipv6 pieces accessible.
> 
> You can't Stephen, ipv6_dev_get_saddr() requires access to the actual ipv6
> device state, that means you have to pull in the entire ipv6 stack in 
> because
> there are dependencies all the way down into the routing code.
> 
> We added a Kconfig fix to cure this specific problem, which made it
> into 2.6.38-final, so I don't understand why Jan is even seeing this,
> it's supposed to force BRIDGE modular if IPV6 is modular:

Oh, sorry, I was still on -rc7.

Nevertheless, I don't think this is the right way to fix it (nor
in infiniband and possibly ip_vs as pointed out).

Jan

> commit dcbcdf22f500ac6e4ec06485341024739b9dc241
> Author: Randy Dunlap <randy.dunlap@oracle.com>
> Date:   Thu Mar 10 13:45:57 2011 -0800
> 
>     net: bridge builtin vs. ipv6 modular
>     
>     When configs BRIDGE=y and IPV6=m, this build error occurs:
>     
>     br_multicast.c:(.text+0xa3341): undefined reference to 
> `ipv6_dev_get_saddr'
>     
>     BRIDGE_IGMP_SNOOPING is boolean; if it were tristate, then adding
>     	depends on IPV6 || IPV6=n
>     to BRIDGE_IGMP_SNOOPING would be a good fix.  As it is currently,
>     making BRIDGE depend on the IPV6 config works.
>     
>     Reported-by: Patrick Schaaf <netdev@bof.de>
>     Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
> index 9190ae4..6dee7bf 100644
> --- a/net/bridge/Kconfig
> +++ b/net/bridge/Kconfig
> @@ -6,6 +6,7 @@ config BRIDGE
>  	tristate "802.1d Ethernet Bridging"
>  	select LLC
>  	select STP
> +	depends on IPV6 || IPV6=n
>  	---help---
>  	  If you say Y here, then your Linux box will be able to act as an
>  	  Ethernet bridge, which means that the different Ethernet segments it

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Jan Beulich]

>>> On 16.03.11 at 18:34, Brian Haley <brian.haley@hp.com> wrote:
> On 03/16/2011 08:34 AM, Jan Beulich wrote:
>> With BRIDGE=y and IPV6=m commit
>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>> link-local address for multicast listener queries") causes the build to
>> break.
>> 
>> Similary, even if both are =m, but ipv6.ko got blacklisted (as is
>> happening in various SuSE distros when disabling IPv6), there's
>> a runtime problem since bridge.ko then won't load anymore due
>> to the missing symbol.
> 
> Load the ipv6 module with disable=1, which is why I added it :)

Indeed, I realized there is such an option only after I sent
that mail. Nevertheless, I think it is overkill to load a huge
module like this just to satisfy never actually used symbol
references.

In fact, just like it seems bogus to load ipv6.ko in a pure IPv4
environment, I think the opposite is also true: IPv4 support
should be in a module, and it should be possible to not load
it in a pure IPv6 environment.

Jan

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Jan Beulich]

>>> On 16.03.11 at 16:24, Stephen Hemminger <shemminger@linux-foundation.org>
wrote:
> On Wed, 16 Mar 2011 12:34:19 +0000
> "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> With BRIDGE=y and IPV6=m commit
>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>> link-local address for multicast listener queries") causes the build to
>> break.
> 
> Rather than continue with the config games, lets just make the necessary
> ipv6 pieces accessible.

The below (however ugly it may look) seems to do the trick for me,
for this particular symbol. Possibly other symbols need doing the
same (didn't check which ones e.g. infiniband depends on), so
some sort of abstraction might be desirable to make the whole
thing look less ugly.

Jan

--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -78,7 +78,21 @@ extern struct inet6_ifaddr      *ipv6_ge
 						 struct net_device *dev,
 						 int strict);
 
-extern int			ipv6_dev_get_saddr(struct net *net,
+#ifdef CONFIG_IPV6_MODULE
+static inline int ipv6_dev_get_saddr_stub(struct net *net,
+					  struct net_device *dev,
+					  const struct in6_addr *daddr,
+					  unsigned int srcprefs,
+					  struct in6_addr *saddr)
+{
+	return -EPROTONOSUPPORT;
+}
+
+extern int			(*ipv6_dev_get_saddr)
+#else
+extern int			ipv6_dev_get_saddr
+#endif
+					      (struct net *net,
 					       struct net_device *dev,
 					       const struct in6_addr *daddr,
 					       unsigned int srcprefs,
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1119,9 +1119,14 @@ out:
 	return ret;
 }
 
-int ipv6_dev_get_saddr(struct net *net, struct net_device *dst_dev,
-		       const struct in6_addr *daddr, unsigned int prefs,
-		       struct in6_addr *saddr)
+#ifdef MODULE
+static int _ipv6_dev_get_saddr
+#else
+int ipv6_dev_get_saddr
+#endif
+	(struct net *net, struct net_device *dst_dev,
+	 const struct in6_addr *daddr, unsigned int prefs,
+	 struct in6_addr *saddr)
 {
 	struct ipv6_saddr_score scores[2],
 				*score = &scores[0], *hiscore = &scores[1];
@@ -1244,7 +1249,6 @@ try_nextdev:
 	in6_ifa_put(hiscore->ifa);
 	return 0;
 }
-EXPORT_SYMBOL(ipv6_dev_get_saddr);
 
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
 		    unsigned char banned_flags)
@@ -4720,6 +4724,10 @@ int __init addrconf_init(void)
 
 	ipv6_addr_label_rtnl_register();
 
+#ifdef MODULE
+	ipv6_dev_get_saddr = _ipv6_dev_get_saddr;
+#endif
+
 	return 0;
 errout:
 	rtnl_af_unregister(&inet6_ops);
@@ -4738,6 +4746,10 @@ void addrconf_cleanup(void)
 	struct net_device *dev;
 	int i;
 
+#ifdef MODULE
+	ipv6_dev_get_saddr = ipv6_dev_get_saddr_stub;
+#endif
+
 	unregister_netdevice_notifier(&ipv6_dev_notf);
 	unregister_pernet_subsys(&addrconf_ops);
 	ipv6_addr_label_cleanup();
--- a/net/ipv6/addrconf_core.c
+++ b/net/ipv6/addrconf_core.c
@@ -3,10 +3,17 @@
  * not configured or static.
  */
 
+#include <net/addrconf.h>
 #include <net/ipv6.h>
 
 #define IPV6_ADDR_SCOPE_TYPE(scope)	((scope) << 16)
 
+#ifdef CONFIG_IPV6_MODULE
+typeof(ipv6_dev_get_saddr) __read_mostly ipv6_dev_get_saddr
+	= ipv6_dev_get_saddr_stub;
+#endif
+EXPORT_SYMBOL_GPL(ipv6_dev_get_saddr);
+
 static inline unsigned ipv6_addr_scope2type(unsigned scope)
 {
 	switch(scope) {

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Eric Dumazet]

Le jeudi 17 mars 2011 à 08:01 +0000, Jan Beulich a écrit :
> >>> On 16.03.11 at 16:24, Stephen Hemminger <shemminger@linux-foundation.org>
> wrote:
> > On Wed, 16 Mar 2011 12:34:19 +0000
> > "Jan Beulich" <JBeulich@novell.com> wrote:
> > 
> >> With BRIDGE=y and IPV6=m commit
> >> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
> >> link-local address for multicast listener queries") causes the build to
> >> break.
> > 
> > Rather than continue with the config games, lets just make the necessary
> > ipv6 pieces accessible.
> 
> The below (however ugly it may look) seems to do the trick for me,
> for this particular symbol. Possibly other symbols need doing the
> same (didn't check which ones e.g. infiniband depends on), so
> some sort of abstraction might be desirable to make the whole
> thing look less ugly.

You should check how things are properly done with RCU, because you must
make sure the module unload wont delete text another cpu is using.

net code usually use synchronize_{rcu|net}() calls.


example :

net/ipv4/netfilter/nf_nat_sip.c

static void __exit nf_nat_sip_fini(void)
{
        rcu_assign_pointer(nf_nat_sip_hook, NULL);
        rcu_assign_pointer(nf_nat_sip_seq_adjust_hook, NULL);
        rcu_assign_pointer(nf_nat_sip_expect_hook, NULL);
        rcu_assign_pointer(nf_nat_sdp_addr_hook, NULL);
        rcu_assign_pointer(nf_nat_sdp_port_hook, NULL);
        rcu_assign_pointer(nf_nat_sdp_session_hook, NULL);
        rcu_assign_pointer(nf_nat_sdp_media_hook, NULL);
        synchronize_rcu();
}

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[David Miller]

From: "Jan Beulich" <JBeulich@novell.com>
Date: Thu, 17 Mar 2011 08:01:42 +0000

>>>> On 16.03.11 at 16:24, Stephen Hemminger <shemminger@linux-foundation.org>
> wrote:
>> On Wed, 16 Mar 2011 12:34:19 +0000
>> "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>> With BRIDGE=y and IPV6=m commit
>>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>>> link-local address for multicast listener queries") causes the build to
>>> break.
>> 
>> Rather than continue with the config games, lets just make the necessary
>> ipv6 pieces accessible.
> 
> The below (however ugly it may look) seems to do the trick for me,
> for this particular symbol. Possibly other symbols need doing the
> same (didn't check which ones e.g. infiniband depends on), so
> some sort of abstraction might be desirable to make the whole
> thing look less ugly.

Sorry, we won't be doing things this way.  We created the disable
option exactly to handle this cleanly, and that is the way you
should take care of the situation you're in where you want to built
ipv6 modular yet have it disabled.

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Brian Haley]

On 03/17/2011 03:57 AM, Jan Beulich wrote:
>>>> On 16.03.11 at 18:34, Brian Haley <brian.haley@hp.com> wrote:
>> On 03/16/2011 08:34 AM, Jan Beulich wrote:
>>> With BRIDGE=y and IPV6=m commit
>>> fe29ec41aaa51902aebd63658dfb04fe6fea8be5 ("bridge: Use IPv6
>>> link-local address for multicast listener queries") causes the build to
>>> break.
>>>
>>> Similary, even if both are =m, but ipv6.ko got blacklisted (as is
>>> happening in various SuSE distros when disabling IPv6), there's
>>> a runtime problem since bridge.ko then won't load anymore due
>>> to the missing symbol.
>>
>> Load the ipv6 module with disable=1, which is why I added it :)
> 
> Indeed, I realized there is such an option only after I sent
> that mail. Nevertheless, I think it is overkill to load a huge
> module like this just to satisfy never actually used symbol
> references.

I could also argue that the bridge code could change into IPv4-only
and IPv6-only pieces to avoid this too (without actually looking
at the code of course).  But in this case you actually built the kernel
with IPV6=m right?

> In fact, just like it seems bogus to load ipv6.ko in a pure IPv4
> environment, I think the opposite is also true: IPv4 support
> should be in a module, and it should be possible to not load
> it in a pure IPv6 environment.

That's a much bigger nut to crack...

-Brian

Re: build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

[Linus Lüssing]

> One unrelated other observation with this change of yours:
> daddr is an input argument to ipv6_dev_get_saddr(), yet
> it gets initialized only after the function was called. Is that
> really correct?
Hmm, that wasn't intentional. I tested that again and so far I still always
got the right source address. I had a little deeper look at ipv6_dev_get_saddr()
and seems like it could get racy the way it is now, so I'm attaching a patch
for that. Thanks for reporting, Jan.

And I hope I didn't cause too much inconvience with the build breakage,
didn't think of testing BRIDGE=y and IPV6=m.

Cheers, Linus

[PATCH] bridge: Fix possibly wrong MLD queries' ethernet source address

[Linus Lüssing]

The ipv6_dev_get_saddr() is currently called with an uninitialized
destination address. Although in tests it usually seemed to nevertheless
always fetch the right source address, there seems to be a possible race
condition.

Therefore this commit changes this, first setting the destination
address and only after that fetching the source address.

Reported-by: Jan Beulich <JBeulich@novell.com>
Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 030a002..f61eb2e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -445,9 +445,9 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
 	ip6h->payload_len = htons(8 + sizeof(*mldq));
 	ip6h->nexthdr = IPPROTO_HOPOPTS;
 	ip6h->hop_limit = 1;
+	ipv6_addr_set(&ip6h->daddr, htonl(0xff020000), 0, 0, htonl(1));
 	ipv6_dev_get_saddr(dev_net(br->dev), br->dev, &ip6h->daddr, 0,
 			   &ip6h->saddr);
-	ipv6_addr_set(&ip6h->daddr, htonl(0xff020000), 0, 0, htonl(1));
 	ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 
 	hopopt = (u8 *)(ip6h + 1);
-- 
1.5.6.5

Re: [PATCH] bridge: Fix possibly wrong MLD queries' ethernet source address

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Tue, 22 Mar 2011 22:40:32 +0100

> The ipv6_dev_get_saddr() is currently called with an uninitialized
> destination address. Although in tests it usually seemed to nevertheless
> always fetch the right source address, there seems to be a possible race
> condition.
> 
> Therefore this commit changes this, first setting the destination
> address and only after that fetching the source address.
> 
> Reported-by: Jan Beulich <JBeulich@novell.com>
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Applied, thank you!