Re: [PATCH v2 3/3] selftests/mm: virtual_address_range: Avoid reading VVAR mappings

[David Hildenbrand <david@redhat.com>]

On 13.01.25 10:09, Thomas Weißschuh wrote:
> On Fri, Jan 10, 2025 at 04:41:03PM +0100, David Hildenbrand wrote:
>> On 10.01.25 14:05, Thomas Weißschuh wrote:
>>> The virtual_address_range selftest reads from the start of each mapping
>>> listed in /proc/self/maps.
>>> However not all mappings are valid to be arbitrarily accessed.
>>> For example the vvar data used for virtual clocks on x86 [vvar_vclock]
>>> can only be accessed if 1) the kernel configuration enables virtual
>>> clocks and 2) the hypervisor provided the data for it.
>>> Only the VDSO itself has the necessary information to know this.
>>> Since commit e93d2521b27f ("x86/vdso: Split virtual clock pages into dedicated mapping")
>>> the virtual clock data was split out into its own mapping, leading
>>> to EFAULT from read() during the validation.
>>>
>>> Skip the various vvar mappings in virtual_address_range to avoid the issue.
>>>
>>> Fixes: e93d2521b27f ("x86/vdso: Split virtual clock pages into dedicated mapping")
>>> Fixes: 010409649885 ("selftests/mm: confirm VA exhaustion without reliance on correctness of mmap()")
>>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>> Closes: https://lore.kernel.org/oe-lkp/202412271148.2656e485-lkp@intel.com
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>> ---
>>>    tools/testing/selftests/mm/virtual_address_range.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/mm/virtual_address_range.c b/tools/testing/selftests/mm/virtual_address_range.c
>>> index 4fc1c21a5e218eaec4d059b75c31a21dd4e8a215..993990aba56fc986c42084ffa91973558aa07e87 100644
>>> --- a/tools/testing/selftests/mm/virtual_address_range.c
>>> +++ b/tools/testing/selftests/mm/virtual_address_range.c
>>> @@ -152,6 +152,10 @@ static int validate_complete_va_space(void)
>>>    		if (prot[0] != 'r')
>>>    			continue;
>>> +		/* Only the VDSO can know if a VVAR mapping is really readable */
>>> +		if (vma_name && !strncmp(vma_name, "[vvar", 5))
>>> +			continue;
>>
>> I'm wondering if there is a more generic way ... but likely not when staring
>> at /proc/self/maps.
>>
>> /proc/self/smaps would indicate this as
>>
>> VM_IO: "io"
>> VM_DONTDUMP: "dd"
>> VM_PFNMAP: "pf"
>>
>> Especially checking for VM_IO sounds reasonable ...
> 
> Agreed.
> 
> Can we instead rely on madvise(MADV_DOFORK) returning EINVAL iff VM_IO?

That might be one option indeed, although it feels like this is 
something that might change in the future ... not sure.

MADV_POPULATE_READ fails with -EFAULT on VM_IO, but also on VM_PFNMAP
and some other conditions, and might not be completely what we want.

We do have some initial smaps parsing in vm_util.c:__check_huge(), maybe 
that could be factored out / reused, not sure how hard that would be to 
extend to return the flags.

-- 
Cheers,

David / dhildenb