lkml.org 
[lkml]   [2023]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 18/39] mm: Handle faultless write upgrades for shstk
    On 25.01.23 19:43, Edgecombe, Rick P wrote:
    > On Wed, 2023-01-25 at 10:27 +0100, David Hildenbrand wrote:
    >>>> 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 ...
    >
    > pte_modify() is handled like this currently:
    >
    > https://lore.kernel.org/lkml/20230119212317.8324-12-rick.p.edgecombe@intel.com/
    >
    > There has been a couple iterations on that. The current solution is to
    > do the Dirty->SavedDirty fixup if needed after the new prots are added.
    >
    > Of course pte_modify() can't know whether you are are attempting to
    > create a shadow stack PTE with the prot you are passing in. But the
    > callers today explicitly call pte_mkwrite() after filling in the other
    > bits with pte_modify().

    See below on my MAP_PRIVATE vs. MAP_SHARED comment.

    > Today this patch causes the pte_mkwrite() to be
    > skipped and another fault may be required in the mprotect() and numa
    > cases, but if we change pte_mkwrite() to take a VMA we can just make it
    > shadow stack to start.
    >
    > It might be worth mentioning, there was a suggestion in the past to try
    > to have the shadow stack bits come out of vm_get_page_prot(), but MM
    > code would then try to map the zero page as (shadow stack) writable
    > when there was a normal (non-shadow stack) read access. So I had to
    > abandon that approach and rely on explicit calls to pte_mkwrite/shstk()
    > to make it shadow stack.

    Thanks, do you have a pointer?

    >
    >>
    >> 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 ...
    >
    > Not sure I'm following what the mk_pte() problem would be. You mean if
    > Write=0,Dirty=1 is manually added to the prot?
    >
    > Shouldn't people generally use the pte_mkwrite() helpers unless they
    > are drawing from a prot that was already created with the helpers or
    > vm_get_page_prot()?

    pte_mkwrite() is mostly only used (except for writenotify ...) for
    MAP_PRIVATE memory ("COW-able"). For MAP_SHARED memory,
    vma->vm_page_prot in a VM_WRITE mapping already contains the write
    permissions. pte_mkwrite() is not necessary (again, unless writenotify
    is active).

    I assume shstk VMAs don't apply to MAP_SHARED VMAs, which is why you
    didn't stumble over that issue yet? Because I don't see how it could
    work with MAP_SHARED VMAs.


    The other thing I had in mind was that we have to make sure that we're
    not accidentally setting "Write=0,Dirty=1" in mk_pte() / pte_modify().

    Assume we had a "Write=1,Dirty=1" PTE, and we effectively wrprotect
    using pte_modify(), we have to make sure to move the dirty bit to the
    saved_dirty bit.

    > I think they can't manually create prot's from bits
    > in core mm code, right? And x86 arch code already has to be aware of
    > shadow stack. It's a bit of an assumption I guess, but I think maybe
    > not too crazy of one?

    I think that's true. Arch code is supposed to deal with that IIRC.

    >
    >>
    >>
    >> Further, ptep_modify_prot_commit() might have to be adjusted to
    >> properly
    >> flush in all relevant cases IIRC.
    >
    > Sorry, I'm not following. Can you elaborate? There is an adjustment
    > made in pte_flags_need_flush().

    Note that I did not fully review all bits of this patch set, just
    throwing out what was on my mind. If already handled, great.

    >
    >>
    >>>
    >>> 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.
    >
    > Yea, true.
    >
    > Thanks for your comments and ideas here, I'll give the:
    > pte_t pte_mkwrite(struct vm_area_struct *vma, pte_t pte)
    > ...solution a try.

    Good!

    --
    Thanks,

    David / dhildenb

    \
     
     \ /
      Last update: 2023-03-26 23:56    [W:4.411 / U:1.696 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site