Re: [RFC PATCH net] bonding: Work around lockdep_is_held false positives

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Maxim Mikityanskiy <maximmi@nvidia.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@nvidia.com>
>>> Reviewed-by: Tariq Toukan <tariqt@nvidia.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@google.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@canonical.com