Re: [PATCH v2 8/8] selftests/mm: report SKIP in pfnmap if a check fails

[Ryan Roberts <ryan.roberts@arm.com>]

On 19/01/2026 14:32, David Hildenbrand (Red Hat) wrote:
> On 1/19/26 15:26, Ryan Roberts wrote:
>> On 19/01/2026 11:16, David Hildenbrand (Red Hat) wrote:
>>> On 1/7/26 17:48, Kevin Brodsky wrote:
>>>> pfnmap currently checks the target file in FIXTURE_SETUP(pfnmap),
>>>> meaning once for every test, and skips the test if any check fails.
>>>>
>>>> The target file is the same for every test so this is a little
>>>> overkill. More importantly, this approach means that the whole suite
>>>> will report PASS even if all the tests are skipped because kernel
>>>> configuration (e.g. CONFIG_STRICT_DEVMEM=y) prevented /dev/mem from
>>>> being mapped, for instance.
>>>>
>>>> Let's ensure that KSFT_SKIP is returned as exit code if any check
>>>> fails by performing the checks in pfnmap_init(), run once. That
>>>> function also takes care of finding the offset of the pages to be
>>>> mapped and saves it in a global. The file is still mapped/unmapped
>>>> for every test, as some of them modify the mapping.
>>>>
>>>> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
>>>> ---
>>>>    tools/testing/selftests/mm/pfnmap.c | 81 ++++++++++++++++++++---------
>>>>    1 file changed, 55 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/pfnmap.c b/tools/testing/selftests/mm/
>>>> pfnmap.c
>>>> index 35b0e3ed54cd..e41d5464130b 100644
>>>> --- a/tools/testing/selftests/mm/pfnmap.c
>>>> +++ b/tools/testing/selftests/mm/pfnmap.c
>>>> @@ -25,8 +25,11 @@
>>>>    #include "kselftest_harness.h"
>>>>    #include "vm_util.h"
>>>>    +#define DEV_MEM_NPAGES    2
>>>> +
>>>>    static sigjmp_buf sigjmp_buf_env;
>>>>    static char *file = "/dev/mem";
>>>> +static off_t file_offset;
>>>>      static void signal_handler(int sig)
>>>>    {
>>>> @@ -88,7 +91,7 @@ static int find_ram_target(off_t *offset,
>>>>                break;
>>>>              /* We need two pages. */
>>>> -        if (end > start + 2 * pagesize) {
>>>> +        if (end > start + DEV_MEM_NPAGES * pagesize) {
>>>>                fclose(file);
>>>>                *offset = start;
>>>>                return 0;
>>>> @@ -97,9 +100,49 @@ static int find_ram_target(off_t *offset,
>>>>        return -ENOENT;
>>>>    }
>>>>    +static void pfnmap_init(void)
>>>> +{
>>>> +    size_t pagesize = getpagesize();
>>>> +    size_t size = DEV_MEM_NPAGES * pagesize;
>>>> +    int fd;
>>>> +    void *addr;
>>>> +
>>>> +    if (strncmp(file, "/dev/mem", strlen("/dev/mem")) == 0) {
>>>> +        int err = find_ram_target(&file_offset, pagesize);
>>>> +
>>>> +        if (err)
>>>> +            ksft_exit_skip("Cannot find ram target in '/proc/iomem': %s\n",
>>>> +                       strerror(-err));
>>>> +    } else {
>>>> +        file_offset = 0;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Make sure we can open and map the file, and perform some basic
>>>> +     * checks; skip the whole suite if anything goes wrong.
>>>> +     * A fresh mapping is then created for every test case by
>>>> +     * FIXTURE_SETUP(pfnmap).
>>>> +     */
>>>> +    fd = open(file, O_RDONLY);
>>>> +    if (fd < 0)
>>>> +        ksft_exit_skip("Cannot open '%s': %s\n", file, strerror(errno));
>>>> +
>>>> +    addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, file_offset);
>>>> +    if (addr == MAP_FAILED)
>>>> +        ksft_exit_skip("Cannot mmap '%s': %s\n", file, strerror(errno));
>>>> +
>>>> +    if (!check_vmflag_pfnmap(addr))
>>>> +        ksft_exit_skip("Invalid file: '%s'. Not pfnmap'ed\n", file);
>>>> +
>>>> +    if (test_read_access(addr, size))
>>>> +        ksft_exit_skip("Cannot read-access mmap'ed '%s'\n", file);
>>>> +
>>>> +    munmap(addr, size);
>>>
>>> Why not keep the fd open then and supply that to all tests without the need for
>>> them to open/close?
>>>
>>> Then, also the file cannot change etc.
>>
>> I had a private conversation with Kevin about this before he posted; my very
>> minor, theorectical concern about that was that it's possible to pass in a
>> custom file to be pfnmapped and I wondered if such a file could map a device
>> region that has read side effects? In that case I think you'd want to open it
>> fresh for each test to ensure consistent starting state?
> 
> Are we aware of devices where we would actually require a new open, and not just
> a new mmap()?

Nope; as I said all hypothetical. I was just being cautious.

> 
> The reason we added support for other files was "other pfnmap'ed memory like
> NVIDIA's EGM". I'd assume that people rather should not pass in something that
> has any side-effects.
> 
>>
>> But if you think that concern is unfounded, certainly just opening it once and
>> reusing will simplify.
> 
> I would just keep it simple here, yes. If this ever becomes a real problem, my
> intuition would tell me that probably the caller is doing something unsupported
> that we just cannot easily identify+reject.

Yeah fair enough.


Thanks,
Ryan