Linux Ethernet Bridging - Mar 2011 - [Bridge] [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

IPv6 address autoconfiguration relies on an UP interface to processes
traffic normally. A bridge that is UP enters LEARNING state and this
state "eats" any Router Advertising packets sent to the
bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
when it changes state allows autoconfiguration code to retry
querying router information.

Signed-off-by: Adam Majer <adamm at zombino.com>
---
 net/bridge/br_if.c      |   20 ++++++++++++++++++++
 net/bridge/br_private.h |    2 ++
 net/bridge/br_stp.c     |    2 ++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..9604d52 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -25,6 +25,22 @@
 
 #include "br_private.h"
 
+
+/*
+ * Notifies that the bridge has changed state
+ */
+static void br_port_change_notifier_handler(struct work_struct *work)
+{
+	struct net_bridge *br = container_of(work,
+					     struct net_bridge,
+					     change_notification_worker);
+
+	rtnl_lock();
+	netdev_state_change(br->dev);
+	rtnl_unlock();
+}
+
+
 /*
  * Determine initial path cost based on speed.
  * using recommendations from 802.1d standard
@@ -163,6 +179,7 @@ static void del_br(struct net_bridge *br, struct list_head
*head)
 {
 	struct net_bridge_port *p, *n;
 
+	flush_work(&br->change_notification_worker);
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
 		del_nbp(p);
 	}
@@ -203,6 +220,9 @@ static struct net_device *new_bridge_dev(struct net *net,
const char *name)
 
 	memcpy(br->group_addr, br_group_address, ETH_ALEN);
 
+	INIT_WORK(&br->change_notification_worker,
+		  &br_port_change_notifier_handler);
+
 	br->feature_mask = dev->features;
 	br->stp_enabled = BR_NO_STP;
 	br->designated_root = br->bridge_id;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..7f8ef41 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -179,6 +179,8 @@ struct net_bridge
 	struct list_head		port_list;
 	struct net_device		*dev;
 
+	struct work_struct		change_notification_worker;
+
 	struct br_cpu_netstats __percpu *stats;
 	spinlock_t			hash_lock;
 	struct hlist_head		hash[BR_HASH_SIZE];
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 57186d8..b5be3e3 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -296,6 +296,8 @@ void br_topology_change_detection(struct net_bridge *br)
 {
 	int isroot = br_is_root_bridge(br);
 
+	queue_work(system_long_wq, &br->change_notification_worker);
+
 	if (br->stp_enabled != BR_KERNEL_STP)
 		return;
 
-- 
1.7.2.3

A bridged interface will timeout trying to receive Router Advert as
these packets are not forwarded when bridge is UP but not in the
FORWARDING state (eg. LEARNING state). Bridge code issues
NETDEV_CHANGE when bridge's internal state is changed. It is then
possible to retry Router Solicitation.

Signed-off-by: Adam Majer <adamm at zombino.com>
---
 net/ipv6/addrconf.c |   74 +++++++++++++++++++++++++++++++++-----------------
 1 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index fd6782e..f8018b3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -152,6 +152,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32
flags);
 static void addrconf_dad_timer(unsigned long data);
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp);
 static void addrconf_dad_run(struct inet6_dev *idev);
+static void addrconf_rs_start(struct inet6_ifaddr *ifp);
 static void addrconf_rs_timer(unsigned long data);
 static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
 static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
@@ -2368,6 +2369,18 @@ static void addrconf_add_linklocal(struct inet6_dev
*idev, struct in6_addr *addr
 		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, idev->dev, 0,
0);
 		addrconf_dad_start(ifp, 0);
 		in6_ifa_put(ifp);
+	} else if (PTR_ERR(ifp) == -EEXIST &&
+		list_is_singular(&idev->addr_list)) {
+		/*
+		 * first address must be link local, but no router - re-solicit.
+		 * This code path is called when bridge exits LEARNING state
+		 */
+
+		ifp = list_first_entry(&idev->addr_list,
+				       struct inet6_ifaddr,
+				       if_list);
+
+		addrconf_rs_start(ifp);
 	}
 }
 
@@ -2532,8 +2545,11 @@ static int addrconf_notify(struct notifier_block *this,
unsigned long event,
 			}
 
 			if (idev) {
-				if (idev->if_flags & IF_READY)
-					/* device is already configured. */
+				if (idev->if_flags & IF_READY &&
+				    idev->if_flags & IF_RA_RCVD)
+					/* device is already configured and
+					 * RA was received.
+					 */
 					break;
 				idev->if_flags |= IF_READY;
 			}
@@ -2775,6 +2791,35 @@ static int addrconf_ifdown(struct net_device *dev, int
how)
 	return 0;
 }
 
+static void addrconf_rs_start(struct inet6_ifaddr *ifp)
+{
+	/* If added prefix is link local and forwarding is off,
+	   start sending router solicitations.
+	 */
+	struct net_device *dev = ifp->idev->dev;
+
+	if ((ifp->idev->cnf.forwarding == 0 ||
+	     ifp->idev->cnf.forwarding == 2) &&
+	    ifp->idev->cnf.rtr_solicits > 0 &&
+	    (dev->flags&IFF_LOOPBACK) == 0 &&
+	    (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) {
+		/*
+		 *	If a host as already performed a random delay
+		 *	[...] as part of DAD [...] there is no need
+		 *	to delay again before sending the first RS
+		 */
+		ndisc_send_rs(ifp->idev->dev, &ifp->addr,
+			      &in6addr_linklocal_allrouters);
+
+		spin_lock_bh(&ifp->lock);
+		ifp->probes = 1;
+		ifp->idev->if_flags |= IF_RS_SENT;
+		addrconf_mod_timer(ifp, AC_RS,
+				   ifp->idev->cnf.rtr_solicit_interval);
+		spin_unlock_bh(&ifp->lock);
+	}
+}
+
 static void addrconf_rs_timer(unsigned long data)
 {
 	struct inet6_ifaddr *ifp = (struct inet6_ifaddr *) data;
@@ -2935,36 +2980,15 @@ out:
 
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp)
 {
-	struct net_device *dev = ifp->idev->dev;
-
 	/*
 	 *	Configure the address for reception. Now it is valid.
 	 */
 
 	ipv6_ifa_notify(RTM_NEWADDR, ifp);
 
-	/* If added prefix is link local and forwarding is off,
-	   start sending router solicitations.
-	 */
+	/* start sending router solicitations. */
 
-	if ((ifp->idev->cnf.forwarding == 0 ||
-	     ifp->idev->cnf.forwarding == 2) &&
-	    ifp->idev->cnf.rtr_solicits > 0 &&
-	    (dev->flags&IFF_LOOPBACK) == 0 &&
-	    (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) {
-		/*
-		 *	If a host as already performed a random delay
-		 *	[...] as part of DAD [...] there is no need
-		 *	to delay again before sending the first RS
-		 */
-		ndisc_send_rs(ifp->idev->dev, &ifp->addr,
&in6addr_linklocal_allrouters);
-
-		spin_lock_bh(&ifp->lock);
-		ifp->probes = 1;
-		ifp->idev->if_flags |= IF_RS_SENT;
-		addrconf_mod_timer(ifp, AC_RS, ifp->idev->cnf.rtr_solicit_interval);
-		spin_unlock_bh(&ifp->lock);
-	}
+	addrconf_rs_start(ifp);
 }
 
 static void addrconf_dad_run(struct inet6_dev *idev)
-- 
1.7.2.3

Hello,

As background for these 2 patches, the current bridge code does not
play very nicely with IPv6 autoconfiguration. What happens is,

[   35.117030] device eth0 entered promiscuous mode
[   35.120898] br0: port 1(eth0) entering learning state
[   35.120901] br0: port 1(eth0) entering learning state

Here bridge is setup and both interface eth0 and br0 are indicated as
up via NETDEV_UP notification. The IPv6 autoconfiguration code
configures linklocal address and start to issue RS packets. These
timeout because bridge is in learning state, not forwarding state.

[   45.168007] br0: no IPv6 routers present
[   45.312020] eth0: no IPv6 routers present

RS timeouts and IPv6 is abandoned at this point.

[   50.144011] br0: port 1(eth0) entering forwarding state

Now bridge entered forwarding state. Userland sets up IPv4 via DHCP or
otherwise.

[   50.144487] ADDRCONF(NETDEV_CHANGE): br0: link becomes ready

With the patches, NETDEV_CHANGE is issued by the bridge once it
changes state. The autoconfiguration code then checks that no RA
packets are received and only linklocal interface is setup. It then
retries soliciting router information.


This is my first attempt at a kernel patch so please be gentle :) Part
of the autoconf patch splits out the addrconf_rs_start() code from the
addrconf_dad_completed() function.

Cheers,
Adam


-- 
Adam Majer
adamm at zombino.com

Why not set forwarding delay to zero? I don't think you are using STP?

P.s: removing linux-kernel mailing list, since there is no point
in copying the whole world on this thread.

On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer
wrote:
>
>IPv6 address autoconfiguration relies on an UP interface to processes
>traffic normally. A bridge that is UP enters LEARNING state and this
>state "eats" any Router Advertising packets sent to the
>bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
>when it changes state allows autoconfiguration code to retry
>querying router information.
>
...
>+static void br_port_change_notifier_handler(struct work_struct *work)
>+{
>+	struct net_bridge *br = container_of(work,
>+					     struct net_bridge,
>+					     change_notification_worker);
>+
>+	rtnl_lock();
>+	netdev_state_change(br->dev);
>+	rtnl_unlock();
>+}
>+
Do you really want user-space to get this notification too?
Why do you put it into a workqueue? Maybe it has to be called in
process-context?

Thanks.