| 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 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Pine.LNX.4.64.0712182052490.24949@bizon.gios.gov.pl>
Date: Tue, 18 Dec 2007 20:53:39 +0100 (CET)
From: Krzysztof Oledzki <olel@....pl>
To: Andy Gospodarek <andy@...yhouse.net>
cc: Jay Vosburgh <fubar@...ibm.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Andrew Morton <akpm@...ux-foundation.org>,
bugme-daemon@...zilla.kernel.org, shemminger@...ux-foundation.org,
davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [Bugme-new] [Bug 9543] New: RTNL: assertion failed at
net/ipv6/addrconf.c (2164)/RTNL: assertion failed at net/ipv4/devinet.c
(1055)
On Fri, 14 Dec 2007, Andy Gospodarek wrote:
> On Fri, Dec 14, 2007 at 07:57:42PM +0100, Krzysztof Oledzki wrote:
>>
>>
>> On Fri, 14 Dec 2007, Andy Gospodarek wrote:
>>
>>> On Fri, Dec 14, 2007 at 05:14:57PM +0100, Krzysztof Oledzki wrote:
>>>>
>>>>
>>>> On Wed, 12 Dec 2007, Jay Vosburgh wrote:
>>>>
>>>>> Herbert Xu <herbert@...dor.apana.org.au> 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 <fubar@...ibm.com>
>>>>>
>>>>> 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:
>> <CUT>
>>
>> No luck.
>>
>
>
> I'm guessing if we go back to using a write-lock for bond->lock this
> will go back to working again, but I'm not totally convinced since there
> are plenty of places where we used a read-lock with it.
Should I check this patch or rather, based on a future discussion, wait
for another version?
>
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 11b76b3..635b857 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);
>
> + rtnl_lock();
> write_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->curr_slave_lock);
> write_unlock_bh(&bond->lock);
> -
> rtnl_unlock();
>
> return count;
> @@ -1191,6 +1194,7 @@ static ssize_t bonding_store_active_slave(struct device *d,
>
> rtnl_lock();
> write_lock_bh(&bond->lock);
> + write_lock_bh(&bond->curr_slave_lock);
>
> if (!USES_PRIMARY(bond->params.mode)) {
> printk(KERN_INFO DRV_NAME
> @@ -1247,6 +1251,7 @@ static ssize_t bonding_store_active_slave(struct device *d,
> }
> }
> out:
> + write_unlock_bh(&bond->curr_slave_lock);
> write_unlock_bh(&bond->lock);
> rtnl_unlock();
>
Best regards,
Krzysztof Olędzki
Powered by blists - more mailing lists