On 3/17/26 16:08, Audra Mitchell wrote:
> Sorry! I missed this email so never responded!
>
> On Tue, Feb 24, 2026 at 05:15:14PM +0100, David Hildenbrand (Arm) wrote:
>> On 2/18/26 19:42, Audra Mitchell wrote:
>>> On architectures with separate user address space, such as s390 or
>>> those without an MMU, the call to __access_ok will return true.
>>
>> Where is this __access_ok() you mention here? Somewhere in
>> fs/proc/task_mmu.c?
>>
>> Where in the soft-dirty test is that triggered?
>>
>> I'm wondering whether the soft-dirty test should be adjusted, but I did
>> not yet understand from where this behavior is triggered.
>
> The problem arises when we are checking to see what features/categories are
> supported. The call chain for the soft-dirty program goes:
>
> main()
> ->test_simple()
> ->pagemap_is_softdirty()
> ->page_entry_is()
> ->pagemap_scan_supported()
> ->__pagemap_scan_get_categories()
> ->ioctl()
>
> We enter the kernel with an ioctl, expecting to have an EFAULT returned (see
> the comment from pagemap_scan_get_categories():
>
> /* Provide an invalid address in order to trigger EFAULT. */
> ret = __pagemap_scan_get_categories(fd, start, (struct page_region *)
> ~0UL);
>
> Once we enter the kernel, we will check the arguments passed which includes
> the
> call to access_ok:
>
> do_pagemap_cmd()
> ->do_pagemap_scan()
> ->pagemap_scan_get_args()
> ->access_ok()
>
> Here is the path within pagemap_scan_get_args where we expect to fail return
> the EFAULT:
>
> if (arg->vec && !access_ok((void __user *)(long)arg->vec,
> size_mul(arg->vec_len, sizeof(struct
> page_region))))
> return -EFAULT;
>
> However, if CONFIG_ALTERNATE_USER_ADDRESS_SPACE is enabled or if CONFIG_MMU is
> NOT enabled, then we just return true:
>
> if (IS_ENABLED(CONFIG_ALTERNATE_USER_ADDRESS_SPACE) ||
> !IS_ENABLED(CONFIG_MMU))
> return true;
>
> The intent appears to be just getting the categories available to us and
> verifying that we have the feature available for testing. However, this corner
> case means the soft-dirty test will fail with the following:
>
Thanks for the information, we should clarify that in the patch description.
> # --------------------
> # running ./soft-dirty
> # --------------------
> # TAP version 13
> # 1..15
> # Bail out! PAGEMAP_SCAN succeeded unexpectedly
> # # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0
> # [FAIL]
> not ok 1 soft-dirty # exit=1
> # SUMMARY: PASS=0 SKIP=0 FAIL=1
> 1..1
>
> Since the intent is just to validate that the features are available to us for
> testing, I think we can just modify the check so that we don't fail if we
> return 0.
>
> Let me know what you think, or if you have more questions!
What about simply testing for success on a test area, wouldn't that be more
reliable
and clearer?
diff --git a/tools/testing/selftests/mm/vm_util.c
b/tools/testing/selftests/mm/vm_util.c
index a6d4ff7dfdc0..489a8d4d915d 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -67,21 +67,26 @@ static uint64_t pagemap_scan_get_categories(int fd, char
*start)
}
/* `start` is any valid address. */
-static bool pagemap_scan_supported(int fd, char *start)
+static bool pagemap_scan_supported(int fd)
{
+ const size_t pagesize = getpagesize();
static int supported = -1;
- int ret;
+ struct page_region r;
+ void *test_area;
if (supported != -1)
return supported;
- /* Provide an invalid address in order to trigger EFAULT. */
- ret = __pagemap_scan_get_categories(fd, start, (struct page_region *)
~0UL);
- if (ret == 0)
- ksft_exit_fail_msg("PAGEMAP_SCAN succeeded unexpectedly\n");
-
- supported = errno == EFAULT;
-
+ test_area = mmap(0, pagesize, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
+ if (test_area == MAP_FAILED) {
+ ksft_print_msg("WARN: mmap() failed: %s\n", strerror(errno));
+ supported = 0;
+ } else {
+ supported = __pagemap_scan_get_categories(fd, test_area, &r) >=
0;
+ ksft_print_msg("errno: %d\n", errno);
+ munmap(test_area, pagesize);
+ }
return supported;
}
@@ -90,7 +95,7 @@ static bool page_entry_is(int fd, char *start, char *desc,
{
bool m = pagemap_get_entry(fd, start) & pagemap_flags;
- if (pagemap_scan_supported(fd, start)) {
+ if (pagemap_scan_supported(fd)) {
bool s = pagemap_scan_get_categories(fd, start) &
pagescan_flags;
if (m == s)
--
2.43.0
>
>> Do we have a Fixes: tag?
>
> I always hesistate to add a Fixes tag on situations like this since this is a
> corner case that was not considered by the original author. If we need a
> fixes tag, then it would be:
>
> Fixes: 600bca580579 ("selftests/mm: check that PAGEMAP_SCAN returns correct
> categories")
Yes, please add that. We nowadays also add proper Fixes tags for tests.
--
Cheers,
David
