Linux Ethernet Bridging - Mar 2011 - [Bridge] build breakage due to br_multicast.c referencing ipv6_dev_get_saddr()

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

On Wed, 16 Mar 2011 12:34:19 +0000
"Jan Beulich" <JBeulich at 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.

--

On Wed, 16 Mar 2011 12:34:19 +0000
"Jan Beulich" <JBeulich at 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.

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: "Jan Beulich" <JBeulich at 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 at 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 at bof.de>
    Signed-off-by: Randy Dunlap <randy.dunlap at oracle.com>
    Signed-off-by: David S. Miller <davem at 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

>>> On 16.03.11 at 18:34, Brian Haley <brian.haley at 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

> 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

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 at novell.com>
Signed-off-by: Linus L?ssing <linus.luessing at 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