On Fri, Dec 14, 2007 at 05:14:57PM +0100, Krzysztof Oledzki wrote:
>
>
> On Wed, 12 Dec 2007, Jay Vosburgh wrote:
>
> >Herbert Xu <[EMAIL PROTECTED]> wrote:
> >
> >>>diff -puN drivers/net/bonding/bond_sysfs.c~bonding-locking-fix
> >>>drivers/net/bonding/bond_sysfs.c
> >>>--- a/drivers/net/bonding/bond_sysfs.c~bonding-locking-fix
> >>>+++ a/drivers/net/bonding/bond_sysfs.c
> >>>@@ -1111,8 +1111,6 @@ static ssize_t bonding_store_primary(str
> >>>out:
> >>> write_unlock_bh(&bond->lock);
> >>>
> >>>- rtnl_unlock();
> >>>-
> >>
> >>Looking at the changeset that added this perhaps the intention
> >>is to hold the lock? If so we should add an rtnl_lock to the start
> >>of the function.
> >
> > Yes, this function needs to hold locks, and more than just
> >what's there now. I believe the following should be correct; I haven't
> >tested it, though (I'm supposedly on vacation right now).
> >
> > The following change should be correct for the
> >bonding_store_primary case discussed in this thread, and also corrects
> >the bonding_store_active case which performs similar functions.
> >
> > The bond_change_active_slave and bond_select_active_slave
> >functions both require rtnl, bond->lock for read and curr_slave_lock for
> >write_bh, and no other locks. This is so that the lower level
> >mode-specific functions can release locks down to just rtnl in order to
> >call, e.g., dev_set_mac_address with the locks it expects (rtnl only).
> >
> >Signed-off-by: Jay Vosburgh <[EMAIL PROTECTED]>
> >
> >diff --git a/drivers/net/bonding/bond_sysfs.c
> >b/drivers/net/bonding/bond_sysfs.c
> >index 11b76b3..28a2d80 100644
> >--- a/drivers/net/bonding/bond_sysfs.c
> >+++ b/drivers/net/bonding/bond_sysfs.c
> >@@ -1075,7 +1075,10 @@ static ssize_t bonding_store_primary(struct device
> >*d,
> > struct slave *slave;
> > struct bonding *bond = to_bond(d);
> >
> >- write_lock_bh(&bond->lock);
> >+ rtnl_lock();
> >+ read_lock(&bond->lock);
> >+ write_lock_bh(&bond->curr_slave_lock);
> >+
> > if (!USES_PRIMARY(bond->params.mode)) {
> > printk(KERN_INFO DRV_NAME
> > ": %s: Unable to set primary slave; %s is in mode
> > %d\n",
> >@@ -1109,8 +1112,8 @@ static ssize_t bonding_store_primary(struct device
> >*d,
> > }
> > }
> >out:
> >- write_unlock_bh(&bond->lock);
> >-
> >+ write_unlock_bh(&bond->curr_slave_lock);
> >+ read_unlock(&bond->lock);
> > rtnl_unlock();
> >
> > return count;
> >@@ -1190,7 +1193,8 @@ static ssize_t bonding_store_active_slave(struct
> >device *d,
> > struct bonding *bond = to_bond(d);
> >
> > rtnl_lock();
> >- write_lock_bh(&bond->lock);
> >+ read_lock(&bond->lock);
> >+ write_lock_bh(&bond->curr_slave_lock);
> >
> > if (!USES_PRIMARY(bond->params.mode)) {
> > printk(KERN_INFO DRV_NAME
> >@@ -1247,7 +1251,8 @@ static ssize_t bonding_store_active_slave(struct
> >device *d,
> > }
> > }
> >out:
> >- write_unlock_bh(&bond->lock);
> >+ write_unlock_bh(&bond->curr_slave_lock);
> >+ read_unlock(&bond->lock);
> > rtnl_unlock();
> >
> > return count;
>
> Vanilla 2.6.24-rc5 plus this patch:
>
> =========================================================
> [ INFO: possible irq lock inversion dependency detected ]
> 2.6.24-rc5 #1
> ---------------------------------------------------------
> events/0/9 just changed the state of lock:
> (&mc->mca_lock){-+..}, at: [<c0411c7a>] mld_ifc_timer_expire+0x130/0x1fb
> but this lock took another, soft-read-irq-unsafe lock in the past:
> (&bond->lock){-.--}
>
> and interrupts could create inverse lock ordering between them.
>
>
Grrr, I should have seen that -- sorry. Try your luck with this instead:
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 11b76b3..0694254 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1075,7 +1075,10 @@ static ssize_t bonding_store_primary(struct device *d,
struct slave *slave;
struct bonding *bond = to_bond(d);
- write_lock_bh(&bond->lock);
+ rtnl_lock();
+ read_lock_bh(&bond->lock);
+ write_lock_bh(&bond->curr_slave_lock);
+
if (!USES_PRIMARY(bond->params.mode)) {
printk(KERN_INFO DRV_NAME
": %s: Unable to set primary slave; %s is in mode %d\n",
@@ -1109,8 +1112,8 @@ static ssize_t bonding_store_primary(struct device *d,
}
}
out:
- write_unlock_bh(&bond->lock);
-
+ write_unlock_bh(&bond->curr_slave_lock);
+ read_unlock_bh(&bond->lock);
rtnl_unlock();
return count;
@@ -1190,7 +1193,8 @@ static ssize_t bonding_store_active_slave(struct device
*d,
struct bonding *bond = to_bond(d);
rtnl_lock();
- write_lock_bh(&bond->lock);
+ read_lock_bh(&bond->lock);
+ write_lock_bh(&bond->curr_slave_lock);
if (!USES_PRIMARY(bond->params.mode)) {
printk(KERN_INFO DRV_NAME
@@ -1247,7 +1251,8 @@ static ssize_t bonding_store_active_slave(struct device
*d,
}
}
out:
- write_unlock_bh(&bond->lock);
+ write_unlock_bh(&bond->curr_slave_lock);
+ read_unlock_bh(&bond->lock);
rtnl_unlock();
return count;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
