linux-kernel - Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6d8c7b8-f69d-01ef-6d66-8a33ea98920f@linuxfoundation.org>
Date:   Wed, 10 Feb 2021 08:57:04 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Kalle Valo <kvalo@...eaurora.org>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        ath10k@...ts.infradead.org, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx()
 calls

On 2/10/21 1:25 AM, Kalle Valo wrote:
> Shuah Khan <skhan@...uxfoundation.org> writes:
> 
>> ath10k_drain_tx() must not be called with conf_mutex held as workers can
>> use that also. Add check to detect conf_mutex held calls.
>>
>> Signed-off-by: Shuah Khan <skhan@...uxfoundation.org>
> 
> The commit log does not answer to "Why?". How did you find this? What
> actual problem are you trying to solve?
> 

I came across the comment block above the ath10k_drain_tx() as I was
reviewing at conf_mutex holds while I was debugging the conf_mutex
lock assert in ath10k_debug_fw_stats_request().

My reasoning is that having this will help detect incorrect usages
of ath10k_drain_tx() while holding conf_mutex which could lead to
locking problems when async worker routines try to call this routine.

>> ---
>>   drivers/net/wireless/ath/ath10k/mac.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 53f92945006f..3545ce7dce0a 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4566,6 +4566,7 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
>>   /* Must not be called with conf_mutex held as workers can use that also. */
>>   void ath10k_drain_tx(struct ath10k *ar)
>>   {
>> +	WARN_ON(lockdep_is_held(&ar->conf_mutex));
> 
> Empty line after WARN_ON().
> 

Will do.

> Shouldn't this check debug_locks similarly lockdep_assert_held() does?
> 
> #define lockdep_assert_held(l)	do {				\
> 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
> 	} while (0)
> 
> And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
> bot error.
> 

Yes.

> But honestly I would prefer to have lockdep_assert_not_held() in
> include/linux/lockdep.h, much cleaner that way. Also
> i915_gem_object_lookup_rcu() could then use the same macro.
> 

Right. This is the right way to go. That was first instinct and
decided to have the discussion evolve in that direction. Now that
it has, I will combine this change with
include/linux/lockdep.h and add lockdep_assert_not_held()

I think we might have other places in the kernel that could use
lockdep_assert_not_held() in addition to i915_gem_object_lookup_rcu()

thanks,
-- Shuah