linux-kernel - Re: [PATCH v9 5/6] KVM: arm64: Allow cacheable stage 2 mapping using VMA flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SA1PR12MB719988B006EBB121DF0CB07BB042A@SA1PR12MB7199.namprd12.prod.outlook.com>
Date: Fri, 4 Jul 2025 16:51:25 +0000
From: Ankit Agrawal <ankita@...dia.com>
To: David Hildenbrand <david@...hat.com>, Jason Gunthorpe <jgg@...dia.com>
CC: "maz@...nel.org" <maz@...nel.org>, "oliver.upton@...ux.dev"
	<oliver.upton@...ux.dev>, "joey.gouly@....com" <joey.gouly@....com>,
	"suzuki.poulose@....com" <suzuki.poulose@....com>, "yuzenghui@...wei.com"
	<yuzenghui@...wei.com>, "catalin.marinas@....com" <catalin.marinas@....com>,
	"will@...nel.org" <will@...nel.org>, "ryan.roberts@....com"
	<ryan.roberts@....com>, "shahuang@...hat.com" <shahuang@...hat.com>,
	"lpieralisi@...nel.org" <lpieralisi@...nel.org>, "ddutile@...hat.com"
	<ddutile@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>, Aniket Agashe
	<aniketa@...dia.com>, Neo Jia <cjia@...dia.com>, Kirti Wankhede
	<kwankhede@...dia.com>, Krishnakant Jaju <kjaju@...dia.com>, "Tarun Gupta
 (SW-GPU)" <targupta@...dia.com>, Vikram Sethi <vsethi@...dia.com>, Andy
 Currid <acurrid@...dia.com>, Alistair Popple <apopple@...dia.com>, John
 Hubbard <jhubbard@...dia.com>, Dan Williams <danw@...dia.com>, Zhi Wang
	<zhiw@...dia.com>, Matt Ochs <mochs@...dia.com>, Uday Dhoke
	<udhoke@...dia.com>, Dheeraj Nigam <dnigam@...dia.com>,
	"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
	"sebastianene@...gle.com" <sebastianene@...gle.com>, "coltonlewis@...gle.com"
	<coltonlewis@...gle.com>, "kevin.tian@...el.com" <kevin.tian@...el.com>,
	"yi.l.liu@...el.com" <yi.l.liu@...el.com>, "ardb@...nel.org"
	<ardb@...nel.org>, "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"gshan@...hat.com" <gshan@...hat.com>, "linux-mm@...ck.org"
	<linux-mm@...ck.org>, "tabba@...gle.com" <tabba@...gle.com>,
	"qperret@...gle.com" <qperret@...gle.com>, "kvmarm@...ts.linux.dev"
	<kvmarm@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "maobibo@...ngson.cn"
	<maobibo@...ngson.cn>
Subject: Re: [PATCH v9 5/6] KVM: arm64: Allow cacheable stage 2 mapping using
 VMA flags

Thank you Jason and David to review this and other patches in the series!
Comments inline.

>>> +             * COW VM_PFNMAP is possible when doing a MAP_PRIVATE
>>> +             * /dev/mem mapping on systems that allow such mapping.
>>> +             * Reject such case.
>>>               */
>>> -            s2_force_noncacheable = true;
>>> +            if (is_cow_mapping(vm_flags))
>>> +                    return -EINVAL;
>>
>> I still would like an explanation why we need to block this.
>>
>> COW PFNMAP is like MIXEDMAP, you end up with a VMA where there is a
>> mixture of MMIO and normal pages. Arguably you are supposed to use
>> vm_normal_page() not pfn_is_map_memory(), but that seems difficult for
>> KVM.
>>
>> Given we exclude the cachable case with the pfn_is_map_memory() we
>> know this is the non-struct page memory already, so why do we need to
>> block the COW?
>>
>> I think the basic rule we are going for is that within the VMA the
>> non-normal/special PTE have to follow the vma->vm_pgprot while the
>> normal pages have to be cachable.
>>
>> So if we find a normal page (ie pfn_is_map_memory()) then we know it
>> is cachable and s2_force_noncacheable = false. Otherwise we use the
>> vm_pgprot to decide if the special PTE is cachable.
>>
>> David can you think of any reason to have this is_cow_mapping() test?
>
> I think with that reasoning, it should be fine to drop it.
>
> I think, the COW test made sense when we were talking about limiting it
> to VM_PFNMAP only and simplifying by dropping other checks. Then, it
> would have identified that something is certainly not "normal" memory.
>

Ack, I'll drop the check in the next version.

> Then this logic that immediately follows:
>
>        if (is_vma_cacheable && s2_force_noncacheable)
>                return -EINVAL;
>
> Doesn't make alot of sense either, the only cases that set
> s2_force_noncacheable=true are the else block of 'if (is_vma_cacheable)'
> so this is dead code too.

I had left it there so that that condition doesn't get violated in the
future. But perhaps it doesn't make much sense to keep it. I'll remove in
the next version.

> Seems like this still needs some cleanup to remove these impossible
> conditions. The logic make sense to me otherwise though.

Sure, I'll fix that in the next version.