From cb32f2a0d194212e4e750a8cdedcc610c9ca4876 Mon Sep 17 00:00:00 2001 From: Jiri Bohac Date: Thu, 2 Sep 2010 05:45:54 +0000 Subject: bonding: Fix jiffies overflow problems (again) The time_before_eq()/time_after_eq() functions operate on unsigned long and only work if the difference between the two compared values is smaller than half the range of unsigned long (31 bits on i386). Some of the variables (slave->jiffies, dev->trans_start, dev->last_rx) used by bonding store a copy of jiffies and may not be updated for a long time. With HZ=1000, time_before_eq()/time_after_eq() will start giving bad results after ~25 days. jiffies will never be before slave->jiffies, dev->trans_start, dev->last_rx by more than possibly a couple ticks caused by preemption of this code. This allows us to detect/prevent these overflows by replacing time_before_eq()/time_after_eq() with time_in_range(). Signed-off-by: Jiri Bohac Signed-off-by: Jean Delvare Signed-off-by: David S. Miller --- drivers/net/bonding/bond_main.c | 56 ++++++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 17 deletions(-) (limited to 'drivers/net/bonding') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 2cc4cfc31892..3b16f62d5606 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2797,9 +2797,15 @@ void bond_loadbalance_arp_mon(struct work_struct *work) * so it can wait */ bond_for_each_slave(bond, slave, i) { + unsigned long trans_start = dev_trans_start(slave->dev); + if (slave->link != BOND_LINK_UP) { - if (time_before_eq(jiffies, dev_trans_start(slave->dev) + delta_in_ticks) && - time_before_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) { + if (time_in_range(jiffies, + trans_start - delta_in_ticks, + trans_start + delta_in_ticks) && + time_in_range(jiffies, + slave->dev->last_rx - delta_in_ticks, + slave->dev->last_rx + delta_in_ticks)) { slave->link = BOND_LINK_UP; slave->state = BOND_STATE_ACTIVE; @@ -2827,8 +2833,12 @@ void bond_loadbalance_arp_mon(struct work_struct *work) * when the source ip is 0, so don't take the link down * if we don't know our ip yet */ - if (time_after_eq(jiffies, dev_trans_start(slave->dev) + 2*delta_in_ticks) || - (time_after_eq(jiffies, slave->dev->last_rx + 2*delta_in_ticks))) { + if (!time_in_range(jiffies, + trans_start - delta_in_ticks, + trans_start + 2 * delta_in_ticks) || + !time_in_range(jiffies, + slave->dev->last_rx - delta_in_ticks, + slave->dev->last_rx + 2 * delta_in_ticks)) { slave->link = BOND_LINK_DOWN; slave->state = BOND_STATE_BACKUP; @@ -2883,13 +2893,16 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks) { struct slave *slave; int i, commit = 0; + unsigned long trans_start; bond_for_each_slave(bond, slave, i) { slave->new_link = BOND_LINK_NOCHANGE; if (slave->link != BOND_LINK_UP) { - if (time_before_eq(jiffies, slave_last_rx(bond, slave) + - delta_in_ticks)) { + if (time_in_range(jiffies, + slave_last_rx(bond, slave) - delta_in_ticks, + slave_last_rx(bond, slave) + delta_in_ticks)) { + slave->new_link = BOND_LINK_UP; commit++; } @@ -2902,8 +2915,9 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks) * active. This avoids bouncing, as the last receive * times need a full ARP monitor cycle to be updated. */ - if (!time_after_eq(jiffies, slave->jiffies + - 2 * delta_in_ticks)) + if (time_in_range(jiffies, + slave->jiffies - delta_in_ticks, + slave->jiffies + 2 * delta_in_ticks)) continue; /* @@ -2921,8 +2935,10 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks) */ if (slave->state == BOND_STATE_BACKUP && !bond->current_arp_slave && - time_after(jiffies, slave_last_rx(bond, slave) + - 3 * delta_in_ticks)) { + !time_in_range(jiffies, + slave_last_rx(bond, slave) - delta_in_ticks, + slave_last_rx(bond, slave) + 3 * delta_in_ticks)) { + slave->new_link = BOND_LINK_DOWN; commit++; } @@ -2933,11 +2949,15 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks) * - (more than 2*delta since receive AND * the bond has an IP address) */ + trans_start = dev_trans_start(slave->dev); if ((slave->state == BOND_STATE_ACTIVE) && - (time_after_eq(jiffies, dev_trans_start(slave->dev) + - 2 * delta_in_ticks) || - (time_after_eq(jiffies, slave_last_rx(bond, slave) - + 2 * delta_in_ticks)))) { + (!time_in_range(jiffies, + trans_start - delta_in_ticks, + trans_start + 2 * delta_in_ticks) || + !time_in_range(jiffies, + slave_last_rx(bond, slave) - delta_in_ticks, + slave_last_rx(bond, slave) + 2 * delta_in_ticks))) { + slave->new_link = BOND_LINK_DOWN; commit++; } @@ -2956,6 +2976,7 @@ static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks) { struct slave *slave; int i; + unsigned long trans_start; bond_for_each_slave(bond, slave, i) { switch (slave->new_link) { @@ -2963,10 +2984,11 @@ static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks) continue; case BOND_LINK_UP: + trans_start = dev_trans_start(slave->dev); if ((!bond->curr_active_slave && - time_before_eq(jiffies, - dev_trans_start(slave->dev) + - delta_in_ticks)) || + time_in_range(jiffies, + trans_start - delta_in_ticks, + trans_start + delta_in_ticks)) || bond->curr_active_slave != slave) { slave->link = BOND_LINK_UP; bond->current_arp_slave = NULL; -- cgit From ab12811c89e88f2e66746790b1fe4469ccb7bdd9 Mon Sep 17 00:00:00 2001 From: Andy Gospodarek Date: Fri, 10 Sep 2010 11:43:20 +0000 Subject: bonding: correctly process non-linear skbs It was recently brought to my attention that 802.3ad mode bonds would no longer form when using some network hardware after a driver update. After snooping around I realized that the particular hardware was using page-based skbs and found that skb->data did not contain a valid LACPDU as it was not stored there. That explained the inability to form an 802.3ad-based bond. For balance-alb mode bonds this was also an issue as ARPs would not be properly processed. This patch fixes the issue in my tests and should be applied to 2.6.36 and as far back as anyone cares to add it to stable. Thanks to Alexander Duyck and Jesse Brandeburg for the suggestions on this one. Signed-off-by: Andy Gospodarek CC: Alexander Duyck CC: Jesse Brandeburg CC: stable@kerne.org Signed-off-by: Jay Vosburgh Signed-off-by: David S. Miller --- drivers/net/bonding/bond_3ad.c | 3 +++ drivers/net/bonding/bond_alb.c | 3 +++ 2 files changed, 6 insertions(+) (limited to 'drivers/net/bonding') diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 822f586d72af..0ddf4c66afe2 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -2466,6 +2466,9 @@ int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct pac if (!(dev->flags & IFF_MASTER)) goto out; + if (!pskb_may_pull(skb, sizeof(struct lacpdu))) + goto out; + read_lock(&bond->lock); slave = bond_get_slave_by_dev((struct bonding *)netdev_priv(dev), orig_dev); diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index c746b331771d..26bb118c4533 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -362,6 +362,9 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct goto out; } + if (!pskb_may_pull(skb, arp_hdr_len(bond_dev))) + goto out; + if (skb->len < sizeof(struct arp_pkt)) { pr_debug("Packet is too small to be an ARP\n"); goto out; -- cgit