Re: [PATCH v4 0/5] implement lightweight guard pages

[Alexander Mikhalitsyn]

On Mon, Oct 28, 2024 at 02:13:26PM +0000, Lorenzo Stoakes wrote:
> Userland library functions such as allocators and threading implementations
> often require regions of memory to act as 'guard pages' - mappings which,
> when accessed, result in a fatal signal being sent to the accessing
> process.
> 
> The current means by which these are implemented is via a PROT_NONE mmap()
> mapping, which provides the required semantics however incur an overhead of
> a VMA for each such region.
> 
> With a great many processes and threads, this can rapidly add up and incur
> a significant memory penalty. It also has the added problem of preventing
> merges that might otherwise be permitted.
> 
> This series takes a different approach - an idea suggested by Vlasimil
> Babka (and before him David Hildenbrand and Jann Horn - perhaps more - the
> provenance becomes a little tricky to ascertain after this - please forgive
> any omissions!)  - rather than locating the guard pages at the VMA layer,
> instead placing them in page tables mapping the required ranges.
> 
> Early testing of the prototype version of this code suggests a 5 times
> speed up in memory mapping invocations (in conjunction with use of
> process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> system and unoptimised code.
> 
> We expect with optimisation and a loaded system with a larger number of
> guard pages this could significantly increase, but in any case these
> numbers are encouraging.
> 
> This way, rather than having separate VMAs specifying which parts of a
> range are guard pages, instead we have a VMA spanning the entire range of
> memory a user is permitted to access and including ranges which are to be
> 'guarded'.
> 
> After mapping this, a user can specify which parts of the range should
> result in a fatal signal when accessed.
> 
> By restricting the ability to specify guard pages to memory mapped by
> existing VMAs, we can rely on the mappings being torn down when the
> mappings are ultimately unmapped and everything works simply as if the
> memory were not faulted in, from the point of view of the containing VMAs.
> 
> This mechanism in effect poisons memory ranges similar to hardware memory
> poisoning, only it is an entirely software-controlled form of poisoning.
> 
> The mechanism is implemented via madvise() behaviour - MADV_GUARD_INSTALL
> which installs page table-level guard page markers - and
> MADV_GUARD_REMOVE - which clears them.
> 
> Guard markers can be installed across multiple VMAs and any existing
> mappings will be cleared, that is zapped, before installing the guard page
> markers in the page tables.
> 
> There is no concept of 'nested' guard markers, multiple attempts to install
> guard markers in a range will, after the first attempt, have no effect.
> 
> Importantly, removing guard markers over a range that contains both guard
> markers and ordinary backed memory has no effect on anything but the guard
> markers (including leaving huge pages un-split), so a user can safely
> remove guard markers over a range of memory leaving the rest intact.
> 
> The actual mechanism by which the page table entries are specified makes
> use of existing logic - PTE markers, which are used for the userfaultfd
> UFFDIO_POISON mechanism.
> 
> Unfortunately PTE_MARKER_POISONED is not suited for the guard page
> mechanism as it results in VM_FAULT_HWPOISON semantics in the fault
> handler, so we add our own specific PTE_MARKER_GUARD and adapt existing
> logic to handle it.
> 
> We also extend the generic page walk mechanism to allow for installation of
> PTEs (carefully restricted to memory management logic only to prevent
> unwanted abuse).
> 
> We ensure that zapping performed by MADV_DONTNEED and MADV_FREE do not
> remove guard markers, nor does forking (except when VM_WIPEONFORK is
> specified for a VMA which implies a total removal of memory
> characteristics).
> 
> It's important to note that the guard page implementation is emphatically
> NOT a security feature, so a user can remove the markers if they wish. We
> simply implement it in such a way as to provide the least surprising
> behaviour.
> 
> An extensive set of self-tests are provided which ensure behaviour is as
> expected and additionally self-documents expected behaviour of guard
> ranges.

Dear Lorenzo,
Dear colleagues,

sorry about raising an old thread.

It looks like this feature is now used in glibc [1]. And we noticed failures in CRIU [2]
CI on Fedora Rawhide userspace. Now a question is how we can properly detect such 
"guarded" pages from user space. As I can see from MADV_GUARD_INSTALL implementation,
it does not modify VMA flags anyhow, but only page tables. It means that /proc/<pid>/maps
and /proc/<pid>/smaps interfaces are useless in this case. (Please, correct me if I'm missing
anything here.)

I wonder if you have any ideas / suggestions regarding Checkpoint/Restore here. We (CRIU devs) are happy
to develop some patches to bring some uAPI to expose MADV_GUARDs, but before going into this we decided
to raise this question in LKML.

+CC criu@xxxxxxxxxxxxxxx
+CC Andrei Vagin <avagin@xxxxxxxxx>
+CC Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx>

Kind regards,
Alex

[1] https://github.com/bminor/glibc/commit/a6fbe36b7f31292981422692236465ab56670ea9
[2] https://github.com/checkpoint-restore/criu/pull/2625

> 
> Suggested-by: Vlastimil Babka <vbabka@xxxxxxx>
> Suggested-by: Jann Horn <jannh@xxxxxxxxxx>
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> 
> v4
> * Use restart_syscall() to implement -ERESTARTNOINTR to ensure correctly
>   handled by kernel - tested this code path and confirmed it works
>   correctly. Thanks to Vlastimil for pointing this issue out!
> * Updated the vector_madvise() handler to not unnecessarily invoke
>   cond_resched() as suggested by Vlastimil.
> * Updated guard page tests to add a test for a vector operation which
>   overwrites existing mappings. Tested this against the -ERESTARTNOINTR
>   case and confirmed working.
> * Improved page walk logic further, refactoring handling logic as suggested
>   by Vlastimil.
> * Moved MAX_MADVISE_GUARD_RETRIES to mm/madvise.c as suggested by Vlastimil.
> 
> v3
> * Cleaned up mm/pagewalk.c logic a bit to make things clearer, as suggested
>   by Vlastiml.
> * Explicitly avoid splitting THP on PTE installation, as suggested by
>   Vlastimil. Note this has no impact on the guard pages logic, which has
>   page table entry handlers at PUD, PMD and PTE level.
> * Added WARN_ON_ONCE() to mm/hugetlb.c path where we don't expect a guard
>   marker, as suggested by Vlastimil.
> * Reverted change to is_poisoned_swp_entry() to exclude guard pages which
>   has the effect of MADV_FREE _not_ clearing guard pages. After discussion
>   with Vlastimil, it became apparent that the ability to 'cancel' the
>   freeing operation by writing to the mapping after having issued an
>   MADV_FREE would mean that we would risk unexpected behaviour should the
>   guard pages be removed, so we now do not remove markers here at all.
> * Added comment to PTE_MARKER_GUARD to highlight that memory tagged with
>   the marker behaves as if it were a region mapped PROT_NONE, as
>   highlighted by David.
> * Rename poison -> install, unpoison -> remove (i.e. MADV_GUARD_INSTALL /
>   MADV_GUARD_REMOVE over MADV_GUARD_POISON / MADV_GUARD_REMOVE) at the
>   request of David and John who both find the poison analogy
>   confusing/overloaded.
> * After a lot of discussion, replace the looping behaviour should page
>   faults race with guard page installation with a modest reattempt followed
>   by returning -ERESTARTNOINTR to have the operation abort and re-enter,
>   relieving lock contention and avoiding the possibility of allowing a
>   malicious sandboxed process to impact the mmap lock or stall the overall
>   process more than necessary, as suggested by Jann and Vlastimil having
>   raised the issue.
> * Adjusted the page table walker so a populated huge PUD or PMD is
>   correctly treated as being populated, necessitating a zap. In v2 we
>   incorrectly skipped over these, which would cause the logic to wrongly
>   proceed as if nothing were populated and the install succeeded.
>   Instead, explicitly check to see if a huge page - if so, do not split but
>   rather abort the operation and let zap take care of things.
> * Updated the guard remove logic to not unnecessarily split huge pages
>   either.
> * Added a debug check to assert that the number of installed PTEs matches
>   expectation, accounting for any existing guard pages.
> * Adapted vector_madvise() used by the process_madvise() system call to
>   handle -ERESTARTNOINTR correctly.
> https://lore.kernel.org/all/cover.1729699916.git.lorenzo.stoakes@xxxxxxxxxx/
> 
> v2
> * The macros in kselftest_harness.h seem to be broken - __EXPECT() is
>   terminated by '} while (0); OPTIONAL_HANDLER(_assert)' meaning it is not
>   safe in single line if / else or for /which blocks, however working
>   around this results in checkpatch producing invalid warnings, as reported
>   by Shuah.
> * Fixing these macros is out of scope for this series, so compromise and
>   instead rewrite test blocks so as to use multiple lines by separating out
>   a decl in most cases. This has the side effect of, for the most part,
>   making things more readable.
> * Heavily document the use of the volatile keyword - we can't avoid
>   checkpatch complaining about this, so we explain it, as reported by
>   Shuah.
> * Updated commit message to highlight that we skip tests we lack
>   permissions for, as reported by Shuah.
> * Replaced a perror() with ksft_exit_fail_perror(), as reported by Shuah.
> * Added user friendly messages to cases where tests are skipped due to lack
>   of permissions, as reported by Shuah.
> * Update the tool header to include the new MADV_GUARD_POISON/UNPOISON
>   defines and directly include asm-generic/mman.h to get the
>   platform-neutral versions to ensure we import them.
> * Finally fixed Vlastimil's email address in Suggested-by tags from suze to
>   suse, as reported by Vlastimil.
> * Added linux-api to cc list, as reported by Vlastimil.
> https://lore.kernel.org/all/cover.1729440856.git.lorenzo.stoakes@xxxxxxxxxx/
> 
> v1
> * Un-RFC'd as appears no major objections to approach but rather debate on
>   implementation.
> * Fixed issue with arches which need mmu_context.h and
>   tlbfush.h. header imports in pagewalker logic to be able to use
>   update_mmu_cache() as reported by the kernel test bot.
> * Added comments in page walker logic to clarify who can use
>   ops->install_pte and why as well as adding a check_ops_valid() helper
>   function, as suggested by Christoph.
> * Pass false in full parameter in pte_clear_not_present_full() as suggested
>   by Jann.
> * Stopped erroneously requiring a write lock for the poison operation as
>   suggested by Jann and Suren.
> * Moved anon_vma_prepare() to the start of madvise_guard_poison() to be
>   consistent with how this is used elsewhere in the kernel as suggested by
>   Jann.
> * Avoid returning -EAGAIN if we are raced on page faults, just keep looping
>   and duck out if a fatal signal is pending or a conditional reschedule is
>   needed, as suggested by Jann.
> * Avoid needlessly splitting huge PUDs and PMDs by specifying
>   ACTION_CONTINUE, as suggested by Jann.
> https://lore.kernel.org/all/cover.1729196871.git.lorenzo.stoakes@xxxxxxxxxx/
> 
> RFC
> https://lore.kernel.org/all/cover.1727440966.git.lorenzo.stoakes@xxxxxxxxxx/
> 
> Lorenzo Stoakes (5):
>   mm: pagewalk: add the ability to install PTEs
>   mm: add PTE_MARKER_GUARD PTE marker
>   mm: madvise: implement lightweight guard page mechanism
>   tools: testing: update tools UAPI header for mman-common.h
>   selftests/mm: add self tests for guard page feature
> 
>  arch/alpha/include/uapi/asm/mman.h           |    3 +
>  arch/mips/include/uapi/asm/mman.h            |    3 +
>  arch/parisc/include/uapi/asm/mman.h          |    3 +
>  arch/xtensa/include/uapi/asm/mman.h          |    3 +
>  include/linux/mm_inline.h                    |    2 +-
>  include/linux/pagewalk.h                     |   18 +-
>  include/linux/swapops.h                      |   24 +-
>  include/uapi/asm-generic/mman-common.h       |    3 +
>  mm/hugetlb.c                                 |    4 +
>  mm/internal.h                                |    6 +
>  mm/madvise.c                                 |  239 ++++
>  mm/memory.c                                  |   18 +-
>  mm/mprotect.c                                |    6 +-
>  mm/mseal.c                                   |    1 +
>  mm/pagewalk.c                                |  246 +++-
>  tools/include/uapi/asm-generic/mman-common.h |    3 +
>  tools/testing/selftests/mm/.gitignore        |    1 +
>  tools/testing/selftests/mm/Makefile          |    1 +
>  tools/testing/selftests/mm/guard-pages.c     | 1243 ++++++++++++++++++
>  19 files changed, 1751 insertions(+), 76 deletions(-)
>  create mode 100644 tools/testing/selftests/mm/guard-pages.c
> 
> --
> 2.47.0