netdev - Re: [RFC PATCH net] bonding: Work around lockdep_is

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27509.1616522187@famine>
Date:   Tue, 23 Mar 2021 10:56:27 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
cc:     Leon Romanovsky <leon@...nel.org>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Nikolay Aleksandrov <nikolay@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <tariqt@...dia.com>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net] bonding: Work around lockdep_is_held false positives

Maxim Mikityanskiy <maximmi@...dia.com> wrote:

>On 2021-03-22 16:09, Leon Romanovsky wrote:
>> On Mon, Mar 22, 2021 at 02:38:46PM +0200, Maxim Mikityanskiy wrote:
>>> After lockdep gets triggered for the first time, it gets disabled, and
>>> lockdep_enabled() will return false. It will affect lockdep_is_held(),
>>> which will start returning true all the time. Normally, it just disables
>>> checks that expect a lock to be held. However, the bonding code checks
>>> that a lock is NOT held, which triggers a false positive in WARN_ON.
>>>
>>> This commit addresses the issue by replacing lockdep_is_held with
>>> spin_is_locked, which should have the same effect, but without suffering
>>> from disabling lockdep.
>>>
>>> Fixes: ee6377147409 ("bonding: Simplify the xmit function for modes that use xmit_hash")
>>> Signed-off-by: Maxim Mikityanskiy <maximmi@...dia.com>
>>> Reviewed-by: Tariq Toukan <tariqt@...dia.com>
>>> ---
>>> While this patch works around the issue, I would like to discuss better
>>> options. Another straightforward approach is to extend lockdep API with
>>> lockdep_is_not_held(), which will be basically !lockdep_is_held() when
>>> lockdep is enabled, but will return true when !lockdep_enabled().
>>
>> lockdep_assert_not_held() was added in this cycle to tip: locking/core
>> https://yhbt.net/lore/all/161475935945.20312.2870945278690244669.tip-bot2@tip-bot2/
>> https://yhbt.net/lore/all/878s779s9f.fsf@codeaurora.org/
>
>Thanks for this suggestion - I wasn't aware that this macro was recently
>added and I could use it instead of spin_is_locked.
>
>Still, I would like to figure out why the bonding code does this test at
>all. This lock is not taken by bond_update_slave_arr() itself, so why is
>that a problem in this code?

	The goal, I believe, is to insure that the mode_lock is not held
by the caller when entering bond_update_slave_arr.  I suspect this is
because bond_update_slave_arr may sleep.  One calling context notes this
in a comment:

void bond_3ad_handle_link_change(struct slave *slave, char link)
{
[...]
	/* RTNL is held and mode_lock is released so it's safe
	 * to update slave_array here.
	 */
	bond_update_slave_arr(slave->bond, NULL);

	However, as far as I can tell, lockdep_is_held() does not test
for "lock held by this particular context" but instead is "lock held by
any context at all."  As such, I think the test is not valid, and should
be removed.

	The code in question was added by:

commit ee6377147409a00c071b2da853059a7d59979fbc
Author: Mahesh Bandewar <maheshb@...gle.com>
Date:   Sat Oct 4 17:45:01 2014 -0700

    bonding: Simplify the xmit function for modes that use xmit_hash

	Mahesh, Nikolay, any thoughts?

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com