Re: [PATCH v5 18/39] mm: Handle faultless write upgrades for shstk

[David Hildenbrand <david@redhat.com>]

On 24.01.23 19:14, Edgecombe, Rick P wrote:
> On Tue, 2023-01-24 at 17:24 +0100, David Hildenbrand wrote:
>> On 23.01.23 21:47, Edgecombe, Rick P wrote:
>>> On Mon, 2023-01-23 at 10:50 +0100, David Hildenbrand wrote:
>>>> On 19.01.23 22:22, Rick Edgecombe wrote:
>>>>> The x86 Control-flow Enforcement Technology (CET) feature
>>>>> includes
>>>>> a new
>>>>> type of memory called shadow stack. This shadow stack memory
>>>>> has
>>>>> some
>>>>> unusual properties, which requires some core mm changes to
>>>>> function
>>>>> properly.
>>>>>
>>>>> Since shadow stack memory can be changed from userspace, is
>>>>> both
>>>>> VM_SHADOW_STACK and VM_WRITE. But it should not be made
>>>>> conventionally
>>>>> writable (i.e. pte_mkwrite()). So some code that calls
>>>>> pte_mkwrite() needs
>>>>> to be adjusted.
>>>>>
>>>>> One such case is when memory is made writable without an actual
>>>>> write
>>>>> fault. This happens in some mprotect operations, and also
>>>>> prot_numa
>>>>> faults.
>>>>> In both cases code checks whether it should be made
>>>>> (conventionally)
>>>>> writable by calling vma_wants_manual_pte_write_upgrade().
>>>>>
>>>>> One way to fix this would be have code actually check if memory
>>>>> is
>>>>> also
>>>>> VM_SHADOW_STACK and in that case call pte_mkwrite_shstk(). But
>>>>> since
>>>>> most memory won't be shadow stack, just have simpler logic and
>>>>> skip
>>>>> this
>>>>> optimization by changing vma_wants_manual_pte_write_upgrade()
>>>>> to
>>>>> not
>>>>> return true for VM_SHADOW_STACK_MEMORY. This will simply handle
>>>>> all
>>>>> cases of this type.
>>>>>
>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
>>>>> Tested-by: John Allen <john.allen@amd.com>
>>>>> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
>>>>> Reviewed-by: Kirill A. Shutemov <
>>>>> kirill.shutemov@linux.intel.com>
>>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>>> ---
>>>>
>>>> Instead of having these x86-shadow stack details all over the MM
>>>> space,
>>>> was the option explored to handle this more in arch specific
>>>> code?
>>>>
>>>> IIUC, one way to get it working would be
>>>>
>>>> 1) Have a SW "shadowstack" PTE flag.
>>>> 2) Have an "SW-dirty" PTE flag, to store "dirty=1" when
>>>> "write=0".
>>>
>>> I don't think that idea came up. So vma->vm_page_prot would have
>>> the SW
>>> shadow stack flag for VM_SHADOW_STACK, and pte_mkwrite() could do
>>> Write=0,Dirty=1 part. It seems like it should work.
>>>
>>
>> Right, if we include it in vma->vm_page_prot, we'd immediately let
>> mk_pte() just handle that.
>>
>> Otherwise, we'd have to refactor e.g., mk_pte() to consume a vma
>> instead
>> of the vma->vm_page_prot. Let's see if we can avoid that for now.
>>
>>>>
>>>> pte_mkwrite(), pte_write(), pte_dirty ... can then make decisions
>>>> based
>>>> on the "shadowstack" PTE flag and hide all these details from
>>>> core-
>>>> mm.
>>>>
>>>> When mapping a shadowstack page (new page, migration, swapin,
>>>> ...),
>>>> which can be obtained by looking at the VMA flags, the first
>>>> thing
>>>> you'd
>>>> do is set the "shadowstack" PTE flag.
>>>
>>> I guess the downside is that it uses an extra software bit. But the
>>> other positive is that it's less error prone, so that someone
>>> writing
>>> core-mm code won't introduce a change that makes shadow stack VMAs
>>> Write=1 if they don't know to also check for VM_SHADOW_STACK.
>>
>> Right. And I think this mimics the what I would have expected HW to
>> provide: a dedicated HW bit, not somehow mangling this into semantics
>> of
>> existing bits.
> 
> Yea.
> 
>>
>> Roughly speaking: if we abstract it that way and get all of the "how
>> to
>> set it writable now?" out of core-MM, it not only is cleaner and
>> less
>> error prone, it might even allow other architectures that implement
>> something comparable (e.g., using a dedicated HW bit) to actually
>> reuse
>> some of that work. Otherwise most of that "shstk" is really just x86
>> specific ...
>>
>> I guess the only cases we have to special case would be page pinning
>> code where pte_write() would indicate that the PTE is writable (well,
>> it
>> is, just not by "ordinary CPU instruction" context directly): but you
>> do
>> that already, so ... :)
>>
>> Sorry for stumbling over that this late, I only started looking into
>> this when you CCed me on that one patch.
> 
> Sorry for not calling more attention to it earlier. Appreciate your
> comments.
> 
> Previously versions of this series had changed some of these
> pte_mkwrite() calls to maybe_mkwrite(), which of course takes a vma.
> This way an x86 implementation could use the VM_SHADOW_STACK vma flag
> to decide between pte_mkwrite() and pte_mkwrite_shstk(). The feedback
> was that in some of these code paths "maybe" isn't really an option, it
> *needs* to make it writable. Even though the logic was the same, the
> name of the function made it look wrong.
> 
> But another option could be to change pte_mkwrite() to take a vma. This
> would save using another software bit on x86, but instead requires a
> small change to each arch's pte_mkwrite().

I played with that idea shortly as well, but discarded it. I was not 
able to convince myself that it wouldn't be required to pass in the VMA 
as well for things like pte_dirty(), pte_mkdirty(), pte_write(), ... 
which would end up fairly ugly (or even impossible in thing slike GUP-fast).

For example, I wonder how we'd be handling stuff like do_numa_page() 
cleanly correctly, where we use pte_modify() + pte_mkwrite(), and either 
call might set the PTE writable and maintain dirty bit ...

Having that said, maybe it could work with only a single saved-dirty bit 
and passing in the VMA for pte_mkwrite() only.

pte_wrprotect() would detect "writable=0,dirty=1" and move the dirty bit 
to the soft-dirty bit instead, resulting in 
"writable=0,dirty=0,saved-dirty=1",

pte_dirty() would return dirty==1||saved-dirty==1.

pte_mkdirty() would set either set dirty=1 or saved-dirty=1, depending 
on the writable bit.

pte_mkclean() would clean both bits.

pte_write() would detect "writable == 1 || (writable==0 && dirty==1)"

pte_mkwrite() would act according to the VMA, and in addition, merge the 
saved-dirty bit into the dirty bit.

pte_modify() and mk_pte() .... would require more thought ...


Further, ptep_modify_prot_commit() might have to be adjusted to properly 
flush in all relevant cases IIRC.

> 
> x86's pte_mkwrite() would then be pretty close to maybe_mkwrite(), but
> maybe it could additionally warn if the vma is not writable. It also
> seems more aligned with your changes to stop taking hints from PTE bits
> and just look at the VMA? (I'm thinking about the dropping of the dirty
> check in GUP and dropping pte_saved_write())

The soft-shstk bit wouldn't be a hint, it would be logically changing 
the "type" of the PTE such that any other PTE functions can do the right 
thing without having to consume the VMA.


-- 
Thanks,

David / dhildenb