linux-kernel - Re: [PATCH v3 3/6] powerpc: fadump: use lock guard for mutex

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: <9f18e699-4819-4d2f-a932-fc5e399e8abd@linux.ibm.com>
Date: Thu, 8 May 2025 11:23:17 +0530
From: Sourabh Jain <sourabhjain@...ux.ibm.com>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>, maddy@...ux.ibm.com,
        linuxppc-dev@...ts.ozlabs.org
Cc: npiggin@...il.com, christophe.leroy@...roup.eu, mpe@...erman.id.au,
        peterz@...radead.org, ajd@...ux.ibm.com, mahesh@...ux.ibm.com,
        hbathini@...ux.ibm.com, linux-kernel@...r.kernel.org,
        Srikar Dronamraju <srikar@...ux.ibm.com>
Subject: Re: [PATCH v3 3/6] powerpc: fadump: use lock guard for mutex



On 05/05/25 13:23, Shrikanth Hegde wrote:
> use scoped_guard for scope based resource management of mutex.
> This would make the code simpler and easier to maintain.
>
> More details on lock guards can be found at
> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>
> Reviewed-by: Srikar Dronamraju <srikar@...ux.ibm.com>
> Signed-off-by: Shrikanth Hegde <sshegde@...ux.ibm.com>
> ---
>   arch/powerpc/kernel/fadump.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index df16c7f547ab..b8c7993c5bb1 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1375,15 +1375,12 @@ static void fadump_free_elfcorehdr_buf(void)
>   
>   static void fadump_invalidate_release_mem(void)
>   {
> -	mutex_lock(&fadump_mutex);
> -	if (!fw_dump.dump_active) {
> -		mutex_unlock(&fadump_mutex);
> -		return;
> +	scoped_guard(mutex, &fadump_mutex) {
> +		if (!fw_dump.dump_active)
> +			return;
> +		fadump_cleanup();
>   	}
>   
> -	fadump_cleanup();
> -	mutex_unlock(&fadump_mutex);
> -
>   	fadump_free_elfcorehdr_buf();
>   	fadump_release_memory(fw_dump.boot_mem_top, memblock_end_of_DRAM());
>   	fadump_free_cpu_notes_buf();

I tried to understand how scoped_guard gets unwrapped and what changes
it brings to the assembly of the update function. However, with GCC version
11.5.0 20240719 (Red Hat 11.5.0-5), identical assembly was generated for the
fadump_invalidate_release_mem function with or without this patch.

Which was a surprise to me because there are lot macros and compiler
magic involved here to call destructor ( for example 
https://clang.llvm.org/docs/AttributeReference.html#cleanup)
when a variable goes out of scope.

c000000000053978 <fadump_invalidate_release_mem.part.0>:
c000000000053978:       ae 01 4c 3c     addis   r2,r12,430
c00000000005397c:       88 47 42 38     addi    r2,r2,18312
c000000000053980:       a6 02 08 7c     mflr    r0
c000000000053984:       11 57 02 48     bl      c000000000079094 <_mcount>
c000000000053988:       a6 02 08 7c     mflr    r0
c00000000005398c:       f8 ff e1 fb     std     r31,-8(r1)
c000000000053990:       f0 ff c1 fb     std     r30,-16(r1)
c000000000053994:       1f 01 e2 3f     addis   r31,r2,287
c000000000053998:       30 ea ff 3b     addi    r31,r31,-5584
c00000000005399c:       10 00 01 f8     std     r0,16(r1)
c0000000000539a0:       81 ff 21 f8     stdu    r1,-128(r1)
c0000000000539a4:       18 00 41 f8     std     r2,24(r1)
c0000000000539a8:       ad fe ff 4b     bl      c000000000053854 
<fadump_cleanup+0x8>
c0000000000539ac:       c2 00 62 3c     addis   r3,r2,194
c0000000000539b0:       98 c3 63 38     addi    r3,r3,-15464
c0000000000539b4:       c9 1d 06 49     bl      c0000000010b577c 
<mutex_unlock+0x8>
c0000000000539b8:       00 00 00 60     nop
c0000000000539bc:       1f 01 22 3d     addis   r9,r2,287
snip...


Also, fadump_invalidate_release_mem() is only called in the fadump 
kernel in two scenarios
to release the reserved memory:

1. After dump collection
2. When fadump fails to process the dump

So even if the compiler messes up something here, there is no impact on 
dump collection as such.

So changes looks good to me:
Reviewed-by: Sourabh Jain <sourabhjain@...ux.ibm.com>