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.
Fix it by implementing a simplified READ_ONCE() instead.
Fixes: 3f6bfd4789a0 ("selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"") Signed-off-by: Zi Yan ziy@nvidia.com --- 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))); /* Prevent the compiler from optimizing out the entire loop: */ - 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))
extern unsigned int __page_size; extern unsigned int __page_shift;
On Tue, Aug 5, 2025 at 7:51 PM Zi Yan ziy@nvidia.com 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.
[...]
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))
So is the problem with the old code basically that it should have been something like
#define FORCE_READ(x) (*(volatile typeof(*(x)) *)(x))
to actually cast the normal pointer to a volatile pointer?
On 5 Aug 2025, at 14:38, Jann Horn wrote:
On Tue, Aug 5, 2025 at 7:51 PM Zi Yan ziy@nvidia.com 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.
[...]
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))
So is the problem with the old code basically that it should have been something like
#define FORCE_READ(x) (*(volatile typeof(*(x)) *)(x))
to actually cast the normal pointer to a volatile pointer?
Yeah. That works too. I would rename it to FORCE_READ_PTR to avoid misuse. :)
Best Regards, Yan, Zi
On Tue, Aug 05, 2025 at 02:48:25PM -0400, Zi Yan wrote:
On 5 Aug 2025, at 14:38, Jann Horn wrote:
On Tue, Aug 5, 2025 at 7:51 PM Zi Yan ziy@nvidia.com 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.
[...]
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))
So is the problem with the old code basically that it should have been something like
#define FORCE_READ(x) (*(volatile typeof(*(x)) *)(x))
to actually cast the normal pointer to a volatile pointer?
Yeah. That works too. I would rename it to FORCE_READ_PTR to avoid misuse. :)
We were having this discussion on IRC, generally I'm fine with it as-is, though my version sort of intended to make life easier by passing a ptr, but this version makes it closer to READ_ONCE() so probably semantically a little better. :)
Best Regards, Yan, Zi
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 =
/* Prevent the compiler from optimizing out the entire loop: */((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 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?
I know for some reason this form doesn't generate one currently (not sure why), but we may hit that in future.
extern unsigned int __page_size; extern unsigned int __page_shift; -- 2.47.2
Cheers, Lorenzo
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 =
/* Prevent the compiler from optimizing out the entire loop: */((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
+cc Pedro
On Tue, Aug 05, 2025 at 03:09:54PM -0400, Zi Yan wrote:
On 5 Aug 2025, at 15:00, Lorenzo Stoakes wrote:
On Tue, Aug 05, 2025 at 01:51:40PM -0400, Zi Yan wrote:
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.
It's not end of the world though.
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.
Yeah I won't pretend to understand _exactly_ what the compiler is doing here, if this is working in practice across multiple compilers and read-faulting the page that's good enough for me :)
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.
Indeed possibly, be interesting if you or Pedro who's also playing with this could nail down exactly what's going on here.
Best Regards, Yan, Zi
But from my point of view this patch is fine - ship it! :)
Cheers, Lorenzo
On Tue, Aug 05, 2025 at 08:21:23PM +0100, Lorenzo Stoakes wrote:
+cc Pedro
On Tue, Aug 05, 2025 at 03:09:54PM -0400, Zi Yan wrote:
On 5 Aug 2025, at 15:00, Lorenzo Stoakes wrote:
On Tue, Aug 05, 2025 at 01:51:40PM -0400, Zi Yan wrote:
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.
It's not end of the world though.
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 disagree with adding (void), because volatile being properly propagated into the type should hide any Wunused-value warnings (because volatile reads can have side effects, so discarding a read is most definitely valid).
And as I was seeing in https://godbolt.org/z/jnWsET1vx yesterday, GCC (and clang) can silently drop the volatile qualifier For Some Reason.
On Wed, Aug 06, 2025 at 11:34:57AM +0100, Pedro Falcato wrote:
On Tue, Aug 05, 2025 at 08:21:23PM +0100, Lorenzo Stoakes wrote:
+cc Pedro
On Tue, Aug 05, 2025 at 03:09:54PM -0400, Zi Yan wrote:
On 5 Aug 2025, at 15:00, Lorenzo Stoakes wrote:
On Tue, Aug 05, 2025 at 01:51:40PM -0400, Zi Yan wrote:
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.
It's not end of the world though.
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 disagree with adding (void), because volatile being properly propagated into the type should hide any Wunused-value warnings (because volatile reads can have side effects, so discarding a read is most definitely valid).
Yeah, I just wondered _why_.
I mean this is fine as-is. I believe Andrew's already taken the patch as a hotfix anyway.
And as I was seeing in https://godbolt.org/z/jnWsET1vx yesterday, GCC (and clang) can silently drop the volatile qualifier For Some Reason.
Ack, would love to know why, but don't have the time to explore so was hoping you/someone else could figure it out and tell me :P
-- Pedro
Cheers, Lorenzo
Hi Zi Yan, Lorenzo,
Thank you for the detailed discussion. I have been following the thread closely and it has been very insightful.
Zi Yan's fix is excellent and I appreciate the rigorous analysis. Lorenzo's feedback has also deepened my own understanding of the subtleties around the FORCE_READ macro.
Out of curiosity, I also checked the `(void)` prefixing on Godbolt. As Zi Yan concluded, the resulting assembly appears identical.
I will be happy to join any future discussions regarding the exact behavior of volatile in this context.
For this patch, it's definitely LGTM from my side as well, so. Reviewed-by:wang lian lianux.mm@gmail.com
Thanks, wang lian
On 05.08.25 19:51, 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.
Fix it by implementing a simplified READ_ONCE() instead.
Fixes: 3f6bfd4789a0 ("selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"") Signed-off-by: Zi Yan ziy@nvidia.com
Ouch, thank!
Acked-by: David Hildenbrand david@redhat.com
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.
Fix it by implementing a simplified READ_ONCE() instead.
Fixes: 3f6bfd4789a0 ("selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"") Signed-off-by: Zi Yan ziy@nvidia.com
Reviewed-by: Wei Yang richard.weiyang@gmail.com
linux-kselftest-mirror@lists.linaro.org