netdev - [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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 22 Mar 2021 14:38:46 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Nikolay Aleksandrov <nikolay@...hat.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <tariqt@...dia.com>, <netdev@...r.kernel.org>,
        "Maxim Mikityanskiy" <maximmi@...dia.com>
Subject: [RFC PATCH net] bonding: Work around lockdep_is_held false positives

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().

However, there is no reliable way to check that some lock is not held
without taking it ourselves (because the lock may be taken by another
thread after the check). Could someone explain why this code tries to
make this check? Maybe we could figure out some better way to achieve
the original goal.

 drivers/net/bonding/bond_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 74cbbb22470b..b2fe4e93cb8e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4391,9 +4391,7 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 	int agg_id = 0;
 	int ret = 0;

-#ifdef CONFIG_LOCKDEP
-	WARN_ON(lockdep_is_held(&bond->mode_lock));
-#endif
+	WARN_ON(spin_is_locked(&bond->mode_lock));

 	usable_slaves = kzalloc(struct_size(usable_slaves, arr,
 					    bond->slave_cnt), GFP_KERNEL);
-- 
2.25.1