linux-kernel - Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

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]

Date:	Thu, 03 Nov 2011 23:10:53 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	Lin Ming <ming.m.lin@...el.com>
CC:	Rakib Mullick <rakib.mullick@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-acpi <linux-acpi@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Brown, Len" <len.brown@...el.com>,
	"Moore, Robert" <robert.moore@...el.com>
Subject: Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

Hi,

On 11/03/2011 05:32 PM, Lin Ming wrote:
> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status, 
> 
> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
> there is a recursive lock.
> 

At first look, it definitely doesn't look like a recursive lock, as Lin said.
But, quoting Documentation/lockdep-design.txt:

"Multi-lock dependency rules:
----------------------------

The same lock-class must not be acquired twice, because this could lead
to lock recursion deadlocks."

So, Rakib, do the 2 locks belong to the same lock-class? If yes, then I think
that is the reason for the lockdep splat. Could you show the lockdep warning?

By the way, another way to look at this patch is as an optimization..
i.e., if acpi_gbl_hardware_lock doesn't need to be held to call
acpi_ev_walk_gpe_list(), then we can move from the coarse-grained locking
to finer-grained locking by releasing it earlier, as you did in your patch.
[Note that you will have to update the goto label also, i.e., rename it as
'exit' or something like that]

>> acpi_os_acquire_lock holds the lock acpi_gbl_hardware_lock before calling acpi_hw_register_write(), then 
>> without releasing acpi_gbl_hardware_lock, this function calls acpi_ev_walk_gpe_list, which tries to hold 
>> acpi_gbl_gpe_lock and thus might causes possible recursive lock.
>>
>> Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.
>>
>>  Changes since v0(https://lkml.org/lkml/2011/9/21/355):
>> 	- Fix changelog, thanks to Lin Ming.
>>
>> Signed-off-by: Rakib Mullick <rakib.mullick@...il.com>
>> Cc: Lin Ming <ming.m.lin@...el.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Len Brown <len.brown@...el.com>
>> ---
>>
>> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
>> index 55accb7..e3110ac 100644
>> --- a/drivers/acpi/acpica/hwregs.c
>> +++ b/drivers/acpi/acpica/hwregs.c
>> @@ -269,6 +269,9 @@ acpi_status acpi_hw_clear_acpi_status(void)
>>  
>>  	status = acpi_hw_register_write(ACPI_REGISTER_PM1_STATUS,
>>  					ACPI_BITMASK_ALL_FIXED_STATUS);
>> +
>> +	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
>> +
>>  	if (ACPI_FAILURE(status)) {
>>  		goto unlock_and_exit;
>>  	}
>> @@ -278,7 +281,6 @@ acpi_status acpi_hw_clear_acpi_status(void)
>>  	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>  
>>        unlock_and_exit:
>> -	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
>>  	return_ACPI_STATUS(status);
>>  }
>>  
>>


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@...ux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/