[PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[Andy Gospodarek]

The following patch aimed to resolve an issue where secondary, tertiary,
etc. addresses added to bond interfaces could overwrite the
bond->master_ip and vlan_ip values.

	commit 917fbdb32f37e9a93b00bb12ee83532982982df3
	Author: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
	Date:   Wed Nov 23 23:37:15 2011 +0000

	    bonding: only use primary address for ARP

That patch was good because it prevented bonds using ARP monitoring from
sending frames with an invalid source IP address.  Unfortunately, it
didn't always work as expected.

When using an ioctl (like ifconfig does) to set the IP address and
netmask, 2 separate ioctls are actually called to set the IP and netmask
if the mask chosen doesn't match the standard mask for that class of
address.  The first ioctl did not have a mask that matched the one in
the primary address and would still cause the device address to be
overwritten.  The second ioctl that was called to set the mask would
then detect as secondary and ignored, but the damage was already done.

This was not an issue when using an application that used netlink
sockets as the setting of IP and netmask came down at once.  The
inconsistent behavior between those two interfaces was something that
needed to be resolved.

While I was thinking about how I wanted to resolve this, Ralf Zeidler
came with a patch that resolved this on a RHEL kernel by keeping a full
shadow of the entries in dev->ifa_list for the bonding device and vlan
devices in the bonding driver.  I didn't like the duplication of the
list as I want to see the 'bonding' struct and code shrink rather than
grow, but liked the general idea.

As the Subject indicates this patch drops the master_ip and vlan_ip
elements from the 'bonding' and 'vlan_entry' structs, respectively.
This can be done because a device's address-list is now traversed to
determine the optimal source IP address for ARP requests and for checks
to see if the bonding device has a particular IP address.  This code
could have all be contained inside the bonding driver, but it made more
sense to me to EXPORT and call inet_confirm_addr since it did exactly
what was needed.

I tested this and a backported patch and everything works as expected.
Ralf also helped with verification of the backported patch.

Thanks to Ralf for all his help on this.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
CC: Ralf Zeidler <ralf.zeidler@nsn.com>

---
 drivers/net/bonding/bond_main.c |   86 +++++++++------------------------------
 drivers/net/bonding/bonding.h   |   17 +++++++-
 net/ipv4/devinet.c              |    1 +
 3 files changed, 35 insertions(+), 69 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 435984a..67d58cd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2561,12 +2561,18 @@ re_arm:
 static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 {
 	struct vlan_entry *vlan;
+	struct net_device *vlan_dev;
 
-	if (ip == bond->master_ip)
+	if (ip == bond_confirm_addr(bond->dev, 0, ip))
 		return 1;
 
 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		if (ip == vlan->vlan_ip)
+		rcu_read_lock();
+		vlan_dev = __vlan_find_dev_deep(bond->dev,
+						vlan->vlan_id);
+		rcu_read_unlock();
+		if (vlan_dev &&
+		    ip == bond_confirm_addr(vlan_dev, 0, ip))
 			return 1;
 	}
 
@@ -2608,7 +2614,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 	int i, vlan_id;
 	__be32 *targets = bond->params.arp_targets;
 	struct vlan_entry *vlan;
-	struct net_device *vlan_dev;
+	struct net_device *vlan_dev = NULL;
 	struct rtable *rt;
 
 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		if (!bond_vlan_used(bond)) {
 			pr_debug("basa: empty vlan: arp_send\n");
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
-				      bond->master_ip, 0);
+				      bond_confirm_addr(bond->dev,
+							targets[i],
+							0), 0);
 			continue;
 		}
 
@@ -2644,7 +2652,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			ip_rt_put(rt);
 			pr_debug("basa: rtdev == bond->dev: arp_send\n");
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
-				      bond->master_ip, 0);
+				      bond_confirm_addr(bond->dev,
+							targets[i],
+							0), 0);
 			continue;
 		}
 
@@ -2662,10 +2672,12 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			}
 		}
 
-		if (vlan_id) {
+		if (vlan_id && vlan_dev) {
 			ip_rt_put(rt);
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
-				      vlan->vlan_ip, vlan_id);
+				      bond_confirm_addr(vlan_dev,
+							targets[i],
+							0), vlan_id);
 			continue;
 		}
 
@@ -3287,68 +3299,10 @@ static int bond_netdev_event(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
-/*
- * bond_inetaddr_event: handle inetaddr notifier chain events.
- *
- * We keep track of device IPs primarily to use as source addresses in
- * ARP monitor probes (rather than spewing out broadcasts all the time).
- *
- * We track one IP for the main device (if it has one), plus one per VLAN.
- */
-static int bond_inetaddr_event(struct notifier_block *this, unsigned long event, void *ptr)
-{
-	struct in_ifaddr *ifa = ptr;
-	struct net_device *vlan_dev, *event_dev = ifa->ifa_dev->dev;
-	struct bond_net *bn = net_generic(dev_net(event_dev), bond_net_id);
-	struct bonding *bond;
-	struct vlan_entry *vlan;
-
-	/* we only care about primary address */
-	if(ifa->ifa_flags & IFA_F_SECONDARY)
-		return NOTIFY_DONE;
-
-	list_for_each_entry(bond, &bn->dev_list, bond_list) {
-		if (bond->dev == event_dev) {
-			switch (event) {
-			case NETDEV_UP:
-				bond->master_ip = ifa->ifa_local;
-				return NOTIFY_OK;
-			case NETDEV_DOWN:
-				bond->master_ip = 0;
-				return NOTIFY_OK;
-			default:
-				return NOTIFY_DONE;
-			}
-		}
-
-		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-			vlan_dev = __vlan_find_dev_deep(bond->dev,
-							vlan->vlan_id);
-			if (vlan_dev == event_dev) {
-				switch (event) {
-				case NETDEV_UP:
-					vlan->vlan_ip = ifa->ifa_local;
-					return NOTIFY_OK;
-				case NETDEV_DOWN:
-					vlan->vlan_ip = 0;
-					return NOTIFY_OK;
-				default:
-					return NOTIFY_DONE;
-				}
-			}
-		}
-	}
-	return NOTIFY_DONE;
-}
-
 static struct notifier_block bond_netdev_notifier = {
 	.notifier_call = bond_netdev_event,
 };
 
-static struct notifier_block bond_inetaddr_notifier = {
-	.notifier_call = bond_inetaddr_event,
-};
-
 /*---------------------------- Hashing Policies -----------------------------*/
 
 /*
@@ -4917,7 +4871,6 @@ static int __init bonding_init(void)
 	}
 
 	register_netdevice_notifier(&bond_netdev_notifier);
-	register_inetaddr_notifier(&bond_inetaddr_notifier);
 out:
 	return res;
 err:
@@ -4931,7 +4884,6 @@ err_link:
 static void __exit bonding_exit(void)
 {
 	unregister_netdevice_notifier(&bond_netdev_notifier);
-	unregister_inetaddr_notifier(&bond_inetaddr_notifier);
 
 	bond_destroy_debugfs();
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 1aecc37..4fc9659 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -21,6 +21,7 @@
 #include <linux/cpumask.h>
 #include <linux/in6.h>
 #include <linux/netpoll.h>
+#include <linux/inetdevice.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
 
@@ -166,7 +167,6 @@ struct bond_parm_tbl {
 
 struct vlan_entry {
 	struct list_head vlan_list;
-	__be32 vlan_ip;
 	unsigned short vlan_id;
 };
 
@@ -232,7 +232,6 @@ struct bonding {
 	struct   list_head bond_list;
 	struct   netdev_hw_addr_list mc_list;
 	int      (*xmit_hash_policy)(struct sk_buff *, int);
-	__be32   master_ip;
 	u16      rr_tx_counter;
 	struct   ad_bond_info ad_info;
 	struct   alb_bond_info alb_info;
@@ -378,6 +377,20 @@ static inline bool bond_is_slave_inactive(struct slave *slave)
 	return slave->inactive;
 }
 
+static inline __be32 bond_confirm_addr(struct net_device *dev, __be32 dst, __be32 local)
+{
+	struct in_device *in_dev;
+	__be32 addr = 0;
+
+	rcu_read_lock();
+	in_dev = __in_dev_get_rcu(dev);
+	rcu_read_unlock();
+
+	if (in_dev)
+		addr = inet_confirm_addr(in_dev, dst, local, RT_SCOPE_HOST);
+	return addr;
+}
+
 struct bond_net;
 
 struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e41c40f..d4fad5c 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1079,6 +1079,7 @@ __be32 inet_confirm_addr(struct in_device *in_dev,
 
 	return addr;
 }
+EXPORT_SYMBOL(inet_confirm_addr);
 
 /*
  *	Device notifier
-- 
1.7.4.4

Re: [PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[Jay Vosburgh]

Andy Gospodarek <andy@greyhouse.net> wrote:

>The following patch aimed to resolve an issue where secondary, tertiary,
>etc. addresses added to bond interfaces could overwrite the
>bond->master_ip and vlan_ip values.
>
>	commit 917fbdb32f37e9a93b00bb12ee83532982982df3
>	Author: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
>	Date:   Wed Nov 23 23:37:15 2011 +0000
>
>	    bonding: only use primary address for ARP
>
>That patch was good because it prevented bonds using ARP monitoring from
>sending frames with an invalid source IP address.  Unfortunately, it
>didn't always work as expected.
>
>When using an ioctl (like ifconfig does) to set the IP address and
>netmask, 2 separate ioctls are actually called to set the IP and netmask
>if the mask chosen doesn't match the standard mask for that class of
>address.  The first ioctl did not have a mask that matched the one in
>the primary address and would still cause the device address to be
>overwritten.  The second ioctl that was called to set the mask would
>then detect as secondary and ignored, but the damage was already done.
>
>This was not an issue when using an application that used netlink
>sockets as the setting of IP and netmask came down at once.  The
>inconsistent behavior between those two interfaces was something that
>needed to be resolved.
>
>While I was thinking about how I wanted to resolve this, Ralf Zeidler
>came with a patch that resolved this on a RHEL kernel by keeping a full
>shadow of the entries in dev->ifa_list for the bonding device and vlan
>devices in the bonding driver.  I didn't like the duplication of the
>list as I want to see the 'bonding' struct and code shrink rather than
>grow, but liked the general idea.
>
>As the Subject indicates this patch drops the master_ip and vlan_ip
>elements from the 'bonding' and 'vlan_entry' structs, respectively.
>This can be done because a device's address-list is now traversed to
>determine the optimal source IP address for ARP requests and for checks
>to see if the bonding device has a particular IP address.  This code
>could have all be contained inside the bonding driver, but it made more
>sense to me to EXPORT and call inet_confirm_addr since it did exactly
>what was needed.
>
>I tested this and a backported patch and everything works as expected.
>Ralf also helped with verification of the backported patch.
>
>Thanks to Ralf for all his help on this.

	I did not test this, but it looks good to me from inspection.
I've got a couple of minor style questions, though, below.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>


>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>CC: Ralf Zeidler <ralf.zeidler@nsn.com>
>
>---
> drivers/net/bonding/bond_main.c |   86 +++++++++------------------------------
> drivers/net/bonding/bonding.h   |   17 +++++++-
> net/ipv4/devinet.c              |    1 +
> 3 files changed, 35 insertions(+), 69 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 435984a..67d58cd 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2561,12 +2561,18 @@ re_arm:
> static int bond_has_this_ip(struct bonding *bond, __be32 ip)
> {
> 	struct vlan_entry *vlan;
>+	struct net_device *vlan_dev;
>
>-	if (ip == bond->master_ip)
>+	if (ip == bond_confirm_addr(bond->dev, 0, ip))
> 		return 1;
>
> 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>-		if (ip == vlan->vlan_ip)
>+		rcu_read_lock();
>+		vlan_dev = __vlan_find_dev_deep(bond->dev,
>+						vlan->vlan_id);

	Can the function call arguments fit on one line?

>+		rcu_read_unlock();
>+		if (vlan_dev &&
>+		    ip == bond_confirm_addr(vlan_dev, 0, ip))

	Similarly, can these be combined on to one line?

> 			return 1;
> 	}
>
>@@ -2608,7 +2614,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> 	int i, vlan_id;
> 	__be32 *targets = bond->params.arp_targets;
> 	struct vlan_entry *vlan;
>-	struct net_device *vlan_dev;
>+	struct net_device *vlan_dev = NULL;
> 	struct rtable *rt;
>
> 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> 		if (!bond_vlan_used(bond)) {
> 			pr_debug("basa: empty vlan: arp_send\n");
> 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>-				      bond->master_ip, 0);
>+				      bond_confirm_addr(bond->dev,
>+							targets[i],
>+							0), 0);

	Same comment here and for the later calls to bond_confirm_addr,
here putting "targets[i]" and perhaps the 0 on the previous line,
although I'm less sure that it won't look funky.

	-J

> 			continue;
> 		}
>
>@@ -2644,7 +2652,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> 			ip_rt_put(rt);
> 			pr_debug("basa: rtdev == bond->dev: arp_send\n");
> 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>-				      bond->master_ip, 0);
>+				      bond_confirm_addr(bond->dev,
>+							targets[i],
>+							0), 0);
> 			continue;
> 		}
>
>@@ -2662,10 +2672,12 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> 			}
> 		}
>
>-		if (vlan_id) {
>+		if (vlan_id && vlan_dev) {
> 			ip_rt_put(rt);
> 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>-				      vlan->vlan_ip, vlan_id);
>+				      bond_confirm_addr(vlan_dev,
>+							targets[i],
>+							0), vlan_id);
> 			continue;
> 		}
>
>@@ -3287,68 +3299,10 @@ static int bond_netdev_event(struct notifier_block *this,
> 	return NOTIFY_DONE;
> }
>
>-/*
>- * bond_inetaddr_event: handle inetaddr notifier chain events.
>- *
>- * We keep track of device IPs primarily to use as source addresses in
>- * ARP monitor probes (rather than spewing out broadcasts all the time).
>- *
>- * We track one IP for the main device (if it has one), plus one per VLAN.
>- */
>-static int bond_inetaddr_event(struct notifier_block *this, unsigned long event, void *ptr)
>-{
>-	struct in_ifaddr *ifa = ptr;
>-	struct net_device *vlan_dev, *event_dev = ifa->ifa_dev->dev;
>-	struct bond_net *bn = net_generic(dev_net(event_dev), bond_net_id);
>-	struct bonding *bond;
>-	struct vlan_entry *vlan;
>-
>-	/* we only care about primary address */
>-	if(ifa->ifa_flags & IFA_F_SECONDARY)
>-		return NOTIFY_DONE;
>-
>-	list_for_each_entry(bond, &bn->dev_list, bond_list) {
>-		if (bond->dev == event_dev) {
>-			switch (event) {
>-			case NETDEV_UP:
>-				bond->master_ip = ifa->ifa_local;
>-				return NOTIFY_OK;
>-			case NETDEV_DOWN:
>-				bond->master_ip = 0;
>-				return NOTIFY_OK;
>-			default:
>-				return NOTIFY_DONE;
>-			}
>-		}
>-
>-		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>-			vlan_dev = __vlan_find_dev_deep(bond->dev,
>-							vlan->vlan_id);
>-			if (vlan_dev == event_dev) {
>-				switch (event) {
>-				case NETDEV_UP:
>-					vlan->vlan_ip = ifa->ifa_local;
>-					return NOTIFY_OK;
>-				case NETDEV_DOWN:
>-					vlan->vlan_ip = 0;
>-					return NOTIFY_OK;
>-				default:
>-					return NOTIFY_DONE;
>-				}
>-			}
>-		}
>-	}
>-	return NOTIFY_DONE;
>-}
>-
> static struct notifier_block bond_netdev_notifier = {
> 	.notifier_call = bond_netdev_event,
> };
>
>-static struct notifier_block bond_inetaddr_notifier = {
>-	.notifier_call = bond_inetaddr_event,
>-};
>-
> /*---------------------------- Hashing Policies -----------------------------*/
>
> /*
>@@ -4917,7 +4871,6 @@ static int __init bonding_init(void)
> 	}
>
> 	register_netdevice_notifier(&bond_netdev_notifier);
>-	register_inetaddr_notifier(&bond_inetaddr_notifier);
> out:
> 	return res;
> err:
>@@ -4931,7 +4884,6 @@ err_link:
> static void __exit bonding_exit(void)
> {
> 	unregister_netdevice_notifier(&bond_netdev_notifier);
>-	unregister_inetaddr_notifier(&bond_inetaddr_notifier);
>
> 	bond_destroy_debugfs();
>
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 1aecc37..4fc9659 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -21,6 +21,7 @@
> #include <linux/cpumask.h>
> #include <linux/in6.h>
> #include <linux/netpoll.h>
>+#include <linux/inetdevice.h>
> #include "bond_3ad.h"
> #include "bond_alb.h"
>
>@@ -166,7 +167,6 @@ struct bond_parm_tbl {
>
> struct vlan_entry {
> 	struct list_head vlan_list;
>-	__be32 vlan_ip;
> 	unsigned short vlan_id;
> };
>
>@@ -232,7 +232,6 @@ struct bonding {
> 	struct   list_head bond_list;
> 	struct   netdev_hw_addr_list mc_list;
> 	int      (*xmit_hash_policy)(struct sk_buff *, int);
>-	__be32   master_ip;
> 	u16      rr_tx_counter;
> 	struct   ad_bond_info ad_info;
> 	struct   alb_bond_info alb_info;
>@@ -378,6 +377,20 @@ static inline bool bond_is_slave_inactive(struct slave *slave)
> 	return slave->inactive;
> }
>
>+static inline __be32 bond_confirm_addr(struct net_device *dev, __be32 dst, __be32 local)
>+{
>+	struct in_device *in_dev;
>+	__be32 addr = 0;
>+
>+	rcu_read_lock();
>+	in_dev = __in_dev_get_rcu(dev);
>+	rcu_read_unlock();
>+
>+	if (in_dev)
>+		addr = inet_confirm_addr(in_dev, dst, local, RT_SCOPE_HOST);
>+	return addr;
>+}
>+
> struct bond_net;
>
> struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
>diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>index e41c40f..d4fad5c 100644
>--- a/net/ipv4/devinet.c
>+++ b/net/ipv4/devinet.c
>@@ -1079,6 +1079,7 @@ __be32 inet_confirm_addr(struct in_device *in_dev,
>
> 	return addr;
> }
>+EXPORT_SYMBOL(inet_confirm_addr);
>
> /*
>  *	Device notifier
>-- 
>1.7.4.4

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[Andy Gospodarek]

On Thu, Mar 15, 2012 at 9:03 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
> Andy Gospodarek <andy@greyhouse.net> wrote:
>
>>The following patch aimed to resolve an issue where secondary, tertiary,
>>etc. addresses added to bond interfaces could overwrite the
>>bond->master_ip and vlan_ip values.
>>
>>       commit 917fbdb32f37e9a93b00bb12ee83532982982df3
>>       Author: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
>>       Date:   Wed Nov 23 23:37:15 2011 +0000
>>
>>           bonding: only use primary address for ARP
>>
>>That patch was good because it prevented bonds using ARP monitoring from
>>sending frames with an invalid source IP address.  Unfortunately, it
>>didn't always work as expected.
>>
>>When using an ioctl (like ifconfig does) to set the IP address and
>>netmask, 2 separate ioctls are actually called to set the IP and netmask
>>if the mask chosen doesn't match the standard mask for that class of
>>address.  The first ioctl did not have a mask that matched the one in
>>the primary address and would still cause the device address to be
>>overwritten.  The second ioctl that was called to set the mask would
>>then detect as secondary and ignored, but the damage was already done.
>>
>>This was not an issue when using an application that used netlink
>>sockets as the setting of IP and netmask came down at once.  The
>>inconsistent behavior between those two interfaces was something that
>>needed to be resolved.
>>
>>While I was thinking about how I wanted to resolve this, Ralf Zeidler
>>came with a patch that resolved this on a RHEL kernel by keeping a full
>>shadow of the entries in dev->ifa_list for the bonding device and vlan
>>devices in the bonding driver.  I didn't like the duplication of the
>>list as I want to see the 'bonding' struct and code shrink rather than
>>grow, but liked the general idea.
>>
>>As the Subject indicates this patch drops the master_ip and vlan_ip
>>elements from the 'bonding' and 'vlan_entry' structs, respectively.
>>This can be done because a device's address-list is now traversed to
>>determine the optimal source IP address for ARP requests and for checks
>>to see if the bonding device has a particular IP address.  This code
>>could have all be contained inside the bonding driver, but it made more
>>sense to me to EXPORT and call inet_confirm_addr since it did exactly
>>what was needed.
>>
>>I tested this and a backported patch and everything works as expected.
>>Ralf also helped with verification of the backported patch.
>>
>>Thanks to Ralf for all his help on this.
>
>        I did not test this, but it looks good to me from inspection.
> I've got a couple of minor style questions, though, below.
>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
>
>
>>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>>CC: Ralf Zeidler <ralf.zeidler@nsn.com>
>>
>>---
>> drivers/net/bonding/bond_main.c |   86 +++++++++------------------------------
>> drivers/net/bonding/bonding.h   |   17 +++++++-
>> net/ipv4/devinet.c              |    1 +
>> 3 files changed, 35 insertions(+), 69 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 435984a..67d58cd 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -2561,12 +2561,18 @@ re_arm:
>> static int bond_has_this_ip(struct bonding *bond, __be32 ip)
>> {
>>       struct vlan_entry *vlan;
>>+      struct net_device *vlan_dev;
>>
>>-      if (ip == bond->master_ip)
>>+      if (ip == bond_confirm_addr(bond->dev, 0, ip))
>>               return 1;
>>
>>       list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>>-              if (ip == vlan->vlan_ip)
>>+              rcu_read_lock();
>>+              vlan_dev = __vlan_find_dev_deep(bond->dev,
>>+                                              vlan->vlan_id);
>
>        Can the function call arguments fit on one line?
>
>>+              rcu_read_unlock();
>>+              if (vlan_dev &&
>>+                  ip == bond_confirm_addr(vlan_dev, 0, ip))
>
>        Similarly, can these be combined on to one line?
>

I could do that.  I only put the newline in there as I wanted to keep
the code under 72 chars wide (more of a personal preference than
anything else).  I'm happy to extend it if a limit of 80 would be fine
with you.

>>                       return 1;
>>       }
>>
>>@@ -2608,7 +2614,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>       int i, vlan_id;
>>       __be32 *targets = bond->params.arp_targets;
>>       struct vlan_entry *vlan;
>>-      struct net_device *vlan_dev;
>>+      struct net_device *vlan_dev = NULL;
>>       struct rtable *rt;
>>
>>       for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>>@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>               if (!bond_vlan_used(bond)) {
>>                       pr_debug("basa: empty vlan: arp_send\n");
>>                       bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>>-                                    bond->master_ip, 0);
>>+                                    bond_confirm_addr(bond->dev,
>>+                                                      targets[i],
>>+                                                      0), 0);
>
>        Same comment here and for the later calls to bond_confirm_addr,
> here putting "targets[i]" and perhaps the 0 on the previous line,
> although I'm less sure that it won't look funky.
>
>        -J
>

These we a bit tough to get looking great.  What I did really seemed
like the best I could do and keep it to a reasonable length.  If you
want me to just keep the length of these lines <100 characters wide, I
could combine them into the same line.  Either way is fine with me,
but I really just didn't want the code to get too wide and hard to
read when using a standard size terminal.

>>                       continue;
>>               }
>>
>>@@ -2644,7 +2652,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>                       ip_rt_put(rt);
>>                       pr_debug("basa: rtdev == bond->dev: arp_send\n");
>>                       bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>>-                                    bond->master_ip, 0);
>>+                                    bond_confirm_addr(bond->dev,
>>+                                                      targets[i],
>>+                                                      0), 0);
>>                       continue;
>>               }
>>
>>@@ -2662,10 +2672,12 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>                       }
>>               }
>>
>>-              if (vlan_id) {
>>+              if (vlan_id && vlan_dev) {
>>                       ip_rt_put(rt);
>>                       bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>>-                                    vlan->vlan_ip, vlan_id);
>>+                                    bond_confirm_addr(vlan_dev,
>>+                                                      targets[i],
>>+                                                      0), vlan_id);
>>                       continue;
>>               }
>>
>>@@ -3287,68 +3299,10 @@ static int bond_netdev_event(struct notifier_block *this,
>>       return NOTIFY_DONE;
>> }
>>
>>-/*
>>- * bond_inetaddr_event: handle inetaddr notifier chain events.
>>- *
>>- * We keep track of device IPs primarily to use as source addresses in
>>- * ARP monitor probes (rather than spewing out broadcasts all the time).
>>- *
>>- * We track one IP for the main device (if it has one), plus one per VLAN.
>>- */
>>-static int bond_inetaddr_event(struct notifier_block *this, unsigned long event, void *ptr)
>>-{
>>-      struct in_ifaddr *ifa = ptr;
>>-      struct net_device *vlan_dev, *event_dev = ifa->ifa_dev->dev;
>>-      struct bond_net *bn = net_generic(dev_net(event_dev), bond_net_id);
>>-      struct bonding *bond;
>>-      struct vlan_entry *vlan;
>>-
>>-      /* we only care about primary address */
>>-      if(ifa->ifa_flags & IFA_F_SECONDARY)
>>-              return NOTIFY_DONE;
>>-
>>-      list_for_each_entry(bond, &bn->dev_list, bond_list) {
>>-              if (bond->dev == event_dev) {
>>-                      switch (event) {
>>-                      case NETDEV_UP:
>>-                              bond->master_ip = ifa->ifa_local;
>>-                              return NOTIFY_OK;
>>-                      case NETDEV_DOWN:
>>-                              bond->master_ip = 0;
>>-                              return NOTIFY_OK;
>>-                      default:
>>-                              return NOTIFY_DONE;
>>-                      }
>>-              }
>>-
>>-              list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>>-                      vlan_dev = __vlan_find_dev_deep(bond->dev,
>>-                                                      vlan->vlan_id);
>>-                      if (vlan_dev == event_dev) {
>>-                              switch (event) {
>>-                              case NETDEV_UP:
>>-                                      vlan->vlan_ip = ifa->ifa_local;
>>-                                      return NOTIFY_OK;
>>-                              case NETDEV_DOWN:
>>-                                      vlan->vlan_ip = 0;
>>-                                      return NOTIFY_OK;
>>-                              default:
>>-                                      return NOTIFY_DONE;
>>-                              }
>>-                      }
>>-              }
>>-      }
>>-      return NOTIFY_DONE;
>>-}
>>-
>> static struct notifier_block bond_netdev_notifier = {
>>       .notifier_call = bond_netdev_event,
>> };
>>
>>-static struct notifier_block bond_inetaddr_notifier = {
>>-      .notifier_call = bond_inetaddr_event,
>>-};
>>-
>> /*---------------------------- Hashing Policies -----------------------------*/
>>
>> /*
>>@@ -4917,7 +4871,6 @@ static int __init bonding_init(void)
>>       }
>>
>>       register_netdevice_notifier(&bond_netdev_notifier);
>>-      register_inetaddr_notifier(&bond_inetaddr_notifier);
>> out:
>>       return res;
>> err:
>>@@ -4931,7 +4884,6 @@ err_link:
>> static void __exit bonding_exit(void)
>> {
>>       unregister_netdevice_notifier(&bond_netdev_notifier);
>>-      unregister_inetaddr_notifier(&bond_inetaddr_notifier);
>>
>>       bond_destroy_debugfs();
>>
>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>index 1aecc37..4fc9659 100644
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -21,6 +21,7 @@
>> #include <linux/cpumask.h>
>> #include <linux/in6.h>
>> #include <linux/netpoll.h>
>>+#include <linux/inetdevice.h>
>> #include "bond_3ad.h"
>> #include "bond_alb.h"
>>
>>@@ -166,7 +167,6 @@ struct bond_parm_tbl {
>>
>> struct vlan_entry {
>>       struct list_head vlan_list;
>>-      __be32 vlan_ip;
>>       unsigned short vlan_id;
>> };
>>
>>@@ -232,7 +232,6 @@ struct bonding {
>>       struct   list_head bond_list;
>>       struct   netdev_hw_addr_list mc_list;
>>       int      (*xmit_hash_policy)(struct sk_buff *, int);
>>-      __be32   master_ip;
>>       u16      rr_tx_counter;
>>       struct   ad_bond_info ad_info;
>>       struct   alb_bond_info alb_info;
>>@@ -378,6 +377,20 @@ static inline bool bond_is_slave_inactive(struct slave *slave)
>>       return slave->inactive;
>> }
>>
>>+static inline __be32 bond_confirm_addr(struct net_device *dev, __be32 dst, __be32 local)
>>+{
>>+      struct in_device *in_dev;
>>+      __be32 addr = 0;
>>+
>>+      rcu_read_lock();
>>+      in_dev = __in_dev_get_rcu(dev);
>>+      rcu_read_unlock();
>>+
>>+      if (in_dev)
>>+              addr = inet_confirm_addr(in_dev, dst, local, RT_SCOPE_HOST);
>>+      return addr;
>>+}
>>+
>> struct bond_net;
>>
>> struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
>>diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>>index e41c40f..d4fad5c 100644
>>--- a/net/ipv4/devinet.c
>>+++ b/net/ipv4/devinet.c
>>@@ -1079,6 +1079,7 @@ __be32 inet_confirm_addr(struct in_device *in_dev,
>>
>>       return addr;
>> }
>>+EXPORT_SYMBOL(inet_confirm_addr);
>>
>> /*
>>  *    Device notifier
>>--
>>1.7.4.4
>
> ---
>        -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

Re: [PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[David Miller]

From: Andy Gospodarek <andy@greyhouse.net>
Date: Fri, 16 Mar 2012 09:48:23 -0400

> On Thu, Mar 15, 2012 at 9:03 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>>>@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>>               if (!bond_vlan_used(bond)) {
>>>                       pr_debug("basa: empty vlan: arp_send\n");
>>>                       bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>>>-                                    bond->master_ip, 0);
>>>+                                    bond_confirm_addr(bond->dev,
>>>+                                                      targets[i],
>>>+                                                      0), 0);
>>
>>        Same comment here and for the later calls to bond_confirm_addr,
>> here putting "targets[i]" and perhaps the 0 on the previous line,
>> although I'm less sure that it won't look funky.
>>
>>        -J
>>
> 
> These we a bit tough to get looking great.  What I did really seemed
> like the best I could do and keep it to a reasonable length.  If you
> want me to just keep the length of these lines <100 characters wide, I
> could combine them into the same line.  Either way is fine with me,
> but I really just didn't want the code to get too wide and hard to
> read when using a standard size terminal.

It seems to me that the easiest thing to do is:

	__be32 addr = bond_confirm_addr(bond->dev, targets[i], 0);
	bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], addr, 0);

And actually this sequence is used in three places, so even better
to put it into a helper function.

Re: [PATCH net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead

[Andy Gospodarek]

On Fri, Mar 16, 2012 at 10:55:33PM -0700, David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Fri, 16 Mar 2012 09:48:23 -0400
> 
> > On Thu, Mar 15, 2012 at 9:03 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
> >>>@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> >>>               if (!bond_vlan_used(bond)) {
> >>>                       pr_debug("basa: empty vlan: arp_send\n");
> >>>                       bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> >>>-                                    bond->master_ip, 0);
> >>>+                                    bond_confirm_addr(bond->dev,
> >>>+                                                      targets[i],
> >>>+                                                      0), 0);
> >>
> >>        Same comment here and for the later calls to bond_confirm_addr,
> >> here putting "targets[i]" and perhaps the 0 on the previous line,
> >> although I'm less sure that it won't look funky.
> >>
> >>        -J
> >>
> > 
> > These we a bit tough to get looking great.  What I did really seemed
> > like the best I could do and keep it to a reasonable length.  If you
> > want me to just keep the length of these lines <100 characters wide, I
> > could combine them into the same line.  Either way is fine with me,
> > but I really just didn't want the code to get too wide and hard to
> > read when using a standard size terminal.
> 
> It seems to me that the easiest thing to do is:
> 
> 	__be32 addr = bond_confirm_addr(bond->dev, targets[i], 0);
> 	bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], addr, 0);
> 
> And actually this sequence is used in three places, so even better
> to put it into a helper function.

For readability it makes sense to pop this function out like you have
suggested.  I'm not sure I want to make a helper function for both
calls, but I'll take a look at post an update patch.