linux-kernel - Re: [PATCH 1/2] selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"

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

Message-ID: <4A8C53B9-694C-4313-BB65-A406AABD6B8B@nvidia.com>
Date: Thu, 07 Aug 2025 08:21:38 -0400
From: Zi Yan <ziy@...dia.com>
To: David Laight <david.laight.linux@...il.com>
Cc: wang lian <lianux.mm@...il.com>, akpm@...ux-foundation.org,
 broonie@...nel.org, david@...hat.com, lorenzo.stoakes@...cle.com,
 sj@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 brauner@...nel.org, gkwang@...x-info.com, jannh@...gle.com,
 Liam.Howlett@...cle.com, ludovico.zy.wu@...il.com, p1ucky0923@...il.com,
 richard.weiyang@...il.com, ryncsn@...il.com, shuah@...nel.org,
 vbabka@...e.cz, zijing.zhang@...ton.me
Subject: Re: [PATCH 1/2] selftests/mm: reuse FORCE_READ to replace "asm
 volatile("" : "+r" (XXX));"

On 7 Aug 2025, at 8:16, David Laight wrote:

> On Tue, 05 Aug 2025 10:26:17 -0400
> Zi Yan <ziy@...dia.com> wrote:
>
>> On 17 Jul 2025, at 9:18, wang lian wrote:
>>
>>> Several mm selftests use the `asm volatile("" : "+r" (variable));`
>>> construct to force a read of a variable, preventing the compiler from
>>> optimizing away the memory access. This idiom is cryptic and duplicated
>>> across multiple test files.
>>>
>>> Following a suggestion from David[1], this patch refactors this
>>> common pattern into a FORCE_READ() macro
>>>
>>> [1] https://lore.kernel.org/lkml/4a3e0759-caa1-4cfa-bc3f-402593f1eee3@redhat.com/
>>>
>>> Signed-off-by: wang lian <lianux.mm@...il.com>
>>> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>>> ---
>>>  tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
>>>  tools/testing/selftests/mm/guard-regions.c    |  7 -----
>>>  tools/testing/selftests/mm/hugetlb-madvise.c  |  5 +---
>>>  tools/testing/selftests/mm/migration.c        | 13 ++++----
>>>  tools/testing/selftests/mm/pagemap_ioctl.c    |  4 +--
>>>  .../selftests/mm/split_huge_page_test.c       |  4 +--
>>>  tools/testing/selftests/mm/vm_util.h          |  7 +++++
>>>  7 files changed, 31 insertions(+), 39 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>>> index f0d9c035641d..05de1fc0005b 100644
>>> --- a/tools/testing/selftests/mm/split_huge_page_test.c
>>> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
>>> @@ -399,7 +399,6 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
>>>  		char **addr)
>>>  {
>>>  	size_t i;
>>> -	int dummy = 0;
>>>  	unsigned char buf[1024];
>>>
>>>  	srand(time(NULL));
>>> @@ -441,8 +440,7 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
>>>  	madvise(*addr, fd_size, MADV_HUGEPAGE);
>>>
>>>  	for (size_t i = 0; i < fd_size; i++)
>>> -		dummy += *(*addr + i);
>>> -	asm volatile("" : "+r" (dummy));
>>> +		FORCE_READ((*addr + i));
>>
>> I encountered a segfault when running the test on x86_64.
>> i is 4194297 and fd_size is 4194304.
>> It seems that FORCE_READ() is reading (*addr + i) in 8 byte size
>> and i is only 7 bytes away from the end of the memory address.
>> This led to segfault.
>>
>> (*(volatile char*)(*addr + i)); works fine.
>>
>> Both gcc-12 and gcc-14 have the issue.
>
> The definition of FORCE_READ in 6.16 is:
> #define FORCE_READ(x) (*(volatile typeof(x) *)x)
> this is clearly bogus.
> 'x' is a pointer - follow it through.
> Possibly:
> #define FORCE_READ(x) (*(volatile typeof(*(x)) *)(x))
> is better,
> But why not use READ_ONCE(*addr[i]) ?

Yeah, that is my fix to this:
https://lore.kernel.org/linux-mm/20250805175140.241656-1-ziy@nvidia.com/


>
>>
>>>
>>>  	if (!check_huge_file(*addr, fd_size / pmd_pagesize, pmd_pagesize)) {
>>>  		ksft_print_msg("No large pagecache folio generated, please provide a filesystem supporting large folio\n");
>>> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
>>> index 2b154c287591..c20298ae98ea  100644
>>> --- a/tools/testing/selftests/mm/vm_util.h
>>> +++ b/tools/testing/selftests/mm/vm_util.h
>>> @@ -18,6 +18,13 @@
>>>  #define PM_SWAP                       BIT_ULL(62)
>>>  #define PM_PRESENT                    BIT_ULL(63)
>>>
>>> +/*
>>> + * Ignore the checkpatch warning, we must read from x but don't want to do
>>> + * anything with it in order to trigger a read page fault. We therefore must use
>>> + * volatile to stop the compiler from optimising this away.
>>> + */
>>> +#define FORCE_READ(x) (*(volatile typeof(x) *)x)
>>> +
>>
>> Also, look at FORCE_READ again, it converts x to a pointer to x and
>> deferences x as a point. It does not seem right to me.
>>
>> Best Regards,
>> Yan, Zi
>>


--
Best Regards,
Yan, Zi