On 5 Aug 2025, at 15:00, Lorenzo Stoakes wrote:
On Tue, Aug 05, 2025 at 01:51:40PM -0400, Zi Yan wrote:
FORCE_READ() converts input value x to its pointer type then reads from address x. This is wrong. If x is a non-pointer, it would be caught it easily. But all FORCE_READ() callers are trying to read from a pointer and FORCE_READ() basically reads a pointer to a pointer instead of the original typed pointer. Almost no access violation was found, except the one from split_huge_page_test.
Oops, sorry about that! I had incorrectly assumed typeof() decayed the type when I wrote the guard-regions test code, and hadn't considered that we were casting to (t **) and dereffing that.
And as discussed off-list, if you deref a char array like that, and are at the end of an array, that's err... not brilliant :)
Fix it by implementing a simplified READ_ONCE() instead.
I sort of intended to make this easier for pointers, but the semantics of this are actually potentially a bit nicer - it's more like READ_ONCE() and you're passing in the value you're actually reading so this is probably better.
Fixes: 3f6bfd4789a0 ("selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"") Signed-off-by: Zi Yan ziy@nvidia.com
LGTM, so:
Reviewed-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
But see nits below.
FORCE_READ() comes from commit 876320d71f51 ("selftests/mm: add self tests for guard page feature"). I will a separate patch to stable tree.
tools/testing/selftests/mm/cow.c | 4 ++-- tools/testing/selftests/mm/guard-regions.c | 2 +- tools/testing/selftests/mm/hugetlb-madvise.c | 4 +++- tools/testing/selftests/mm/migration.c | 2 +- tools/testing/selftests/mm/pagemap_ioctl.c | 2 +- tools/testing/selftests/mm/split_huge_page_test.c | 7 +++++-- tools/testing/selftests/mm/vm_util.h | 2 +- 7 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index d30625c18259..c744c603d688 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -1554,8 +1554,8 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc) }
/* Read from the page to populate the shared zeropage. */
- FORCE_READ(mem);
- FORCE_READ(smem);
FORCE_READ(*mem);
FORCE_READ(*smem);
fn(mem, smem, pagesize);
munmap: diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c index b0d42eb04e3a..8dd81c0a4a5a 100644 --- a/tools/testing/selftests/mm/guard-regions.c +++ b/tools/testing/selftests/mm/guard-regions.c @@ -145,7 +145,7 @@ static bool try_access_buf(char *ptr, bool write) if (write) *ptr = 'x'; else
FORCE_READ(ptr);
FORCE_READ(*ptr);}
signal_jump_set = false;
diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c index 1afe14b9dc0c..c5940c0595be 100644 --- a/tools/testing/selftests/mm/hugetlb-madvise.c +++ b/tools/testing/selftests/mm/hugetlb-madvise.c @@ -50,8 +50,10 @@ void read_fault_pages(void *addr, unsigned long nr_pages) unsigned long i;
for (i = 0; i < nr_pages; i++) {
unsigned long *addr2 =((unsigned long *)(addr + (i * huge_page_size)));
FORCE_READ(((unsigned long *)(addr + (i * huge_page_size))));
FORCE_READ(*addr2);}
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c index c5a73617796a..ea945eebec2f 100644 --- a/tools/testing/selftests/mm/migration.c +++ b/tools/testing/selftests/mm/migration.c @@ -110,7 +110,7 @@ void *access_mem(void *ptr) * the memory access actually happens and prevents the compiler * from optimizing away this entire loop. */
FORCE_READ((uint64_t *)ptr);
FORCE_READ(*(uint64_t *)ptr);}
return NULL;
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c b/tools/testing/selftests/mm/pagemap_ioctl.c index 0d4209eef0c3..e6face7c0166 100644 --- a/tools/testing/selftests/mm/pagemap_ioctl.c +++ b/tools/testing/selftests/mm/pagemap_ioctl.c @@ -1525,7 +1525,7 @@ void zeropfn_tests(void)
ret = madvise(mem, hpage_size, MADV_HUGEPAGE); if (!ret) {
FORCE_READ(mem);
FORCE_READ(*mem);ret = pagemap_ioctl(mem, hpage_size, &vec, 1, 0, 0, PAGE_IS_PFNZERO, 0, 0, PAGE_IS_PFNZERO);
diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c index 718daceb5282..3c761228e451 100644 --- a/tools/testing/selftests/mm/split_huge_page_test.c +++ b/tools/testing/selftests/mm/split_huge_page_test.c @@ -440,8 +440,11 @@ 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++)
FORCE_READ((*addr + i));
for (size_t i = 0; i < fd_size; i++) {
char *addr2 = *addr + i;FORCE_READ(*addr2);}
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 c20298ae98ea..b55d1809debc 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -23,7 +23,7 @@
- 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) +#define FORCE_READ(x) (*(const volatile typeof(x) *)&(x))
NIT: but wonder if const is necessary, and also (as discussed off-list
I just used READ_ONCE() code, but it is not necessary.
again :) will this work with a (void) prefixed, just to a. make it clear we're reading but discarding and b. to avoid any possible compiler warning on this?
Adding (void) makes no difference, at least from godbolt.
I know for some reason this form doesn't generate one currently (not sure why), but we may hit that in future.
Neither gcc nor clang complains without (void). My guess is that volatile is doing something there.
Best Regards, Yan, Zi