lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 16 Mar 2012 09:48:23 -0400
From:	Andy Gospodarek <andy@...yhouse.net>
To:	Jay Vosburgh <fubar@...ibm.com>
Cc:	netdev@...r.kernel.org, Ralf Zeidler <ralf.zeidler@....com>
Subject: Re: [PATCH net-next] bonding: remove entries for master_ip and
 vlan_ip and query devices instead

On Thu, Mar 15, 2012 at 9:03 PM, Jay Vosburgh <fubar@...ibm.com> wrote:
> Andy Gospodarek <andy@...yhouse.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@...csson.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@...ibm.com>
>
>
>>Signed-off-by: Andy Gospodarek <andy@...yhouse.net>
>>CC: Ralf Zeidler <ralf.zeidler@....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@...ibm.com
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists