* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 20:42 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <9c0991db-9bf8-414c-b3b0-446023df2a7a@suse.cz>
On Mon, Oct 21, 2024 at 03:45:31PM +0200, Vlastimil Babka wrote:
> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > Add a new PTE marker that results in any access causing the accessing
> > process to segfault.
>
> Should we distinguish it from other segfaults? Is there a way? I can see
> memory protection keys use SEGV_PKUERR, but no idea if we have any free values.
Wasn't even aware that existed!!
I'm not sure a process can do anything particularly useful with this
information though? Hitting a guard page would indicate a programming
error rather than something that might allow meaningful feedback to a user
like memory protection keys.
Do you think there's enough value int his to warrant digging in? And indeed
I imagine bits are in short supply for this and would need a strong
argument to get... so yeah I don't think too worthwhile most likely!
Thanks for the suggestion though!
>
> > This is preferable to PTE_MARKER_POISONED, which results in the same
> > handling as hardware poisoned memory, and is thus undesirable for cases
> > where we simply wish to 'soft' poison a range.
> >
> > This is in preparation for implementing the ability to specify guard pages
> > at the page table level, i.e. ranges that, when accessed, should cause
> > process termination.
> >
> > Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
> > function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
> > purpose was simply incorrect.
> >
> > We then reuse the same logic to determine whether a zap should clear a
> > guard entry - this should only be performed on teardown and never on
> > MADV_DONTNEED or the like.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> A nit below:
>
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 906294ac85dc..50e3f6ed73ac 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6353,6 +6353,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> > ret = VM_FAULT_HWPOISON_LARGE |
> > VM_FAULT_SET_HINDEX(hstate_index(h));
> > goto out_mutex;
> > + } else if (marker & PTE_MARKER_GUARD) {
> > + ret = VM_FAULT_SIGSEGV;
> > + goto out_mutex;
>
> Given we don't support hugetlb, should we WARN_ON_ONCE() if such unexpected
> marker appears there?
>
> > }
> > }
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 0f614523b9f4..551455cd453f 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1455,7 +1455,7 @@ static inline bool should_zap_folio(struct zap_details *details,
> > return !folio_test_anon(folio);
> > }
> >
> > -static inline bool zap_drop_file_uffd_wp(struct zap_details *details)
> > +static inline bool zap_drop_markers(struct zap_details *details)
> > {
> > if (!details)
> > return false;
> > @@ -1476,7 +1476,7 @@ zap_install_uffd_wp_if_needed(struct vm_area_struct *vma,
> > if (vma_is_anonymous(vma))
> > return;
> >
> > - if (zap_drop_file_uffd_wp(details))
> > + if (zap_drop_markers(details))
> > return;
> >
> > for (;;) {
> > @@ -1671,7 +1671,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
> > * drop the marker if explicitly requested.
> > */
> > if (!vma_is_anonymous(vma) &&
> > - !zap_drop_file_uffd_wp(details))
> > + !zap_drop_markers(details))
> > + continue;
> > + } else if (is_guard_swp_entry(entry)) {
> > + /*
> > + * Ordinary zapping should not remove guard PTE
> > + * markers. Only do so if we should remove PTE markers
> > + * in general.
> > + */
> > + if (!zap_drop_markers(details))
> > continue;
> > } else if (is_hwpoison_entry(entry) ||
> > is_poisoned_swp_entry(entry)) {
> > @@ -4003,6 +4011,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf)
> > if (marker & PTE_MARKER_POISONED)
> > return VM_FAULT_HWPOISON;
> >
> > + /* Hitting a guard page is always a fatal condition. */
> > + if (marker & PTE_MARKER_GUARD)
> > + return VM_FAULT_SIGSEGV;
> > +
> > if (pte_marker_entry_uffd_wp(entry))
> > return pte_marker_handle_uffd_wp(vmf);
> >
>
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 20:37 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <fedd19ce-ea15-4ded-a1b5-ff050de15bba@suse.cz>
On 21.10.24 22:25, Vlastimil Babka wrote:
> On 10/21/24 22:17, David Hildenbrand wrote:
>> On 21.10.24 22:11, Vlastimil Babka wrote:
>>> On 10/20/24 18:20, Lorenzo Stoakes wrote:
>>>
>>> <snip>
>>>
>>>> +static long madvise_guard_poison(struct vm_area_struct *vma,
>>>> + struct vm_area_struct **prev,
>>>> + unsigned long start, unsigned long end)
>>>> +{
>>>> + long err;
>>>> +
>>>> + *prev = vma;
>>>> + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
>>>> + return -EINVAL;
>>>> +
>>>> + /*
>>>> + * If we install poison markers, then the range is no longer
>>>> + * empty from a page table perspective and therefore it's
>>>> + * appropriate to have an anon_vma.
>>>> + *
>>>> + * This ensures that on fork, we copy page tables correctly.
>>>> + */
>>>> + err = anon_vma_prepare(vma);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + /*
>>>> + * Optimistically try to install the guard poison pages first. If any
>>>> + * non-guard pages are encountered, give up and zap the range before
>>>> + * trying again.
>>>> + */
>>>
>>> Should the page walker become powerful enough to handle this in one go? :)
>>> But sure, if it's too big a task to teach it to zap ptes with all the tlb
>>> flushing etc (I assume it's something page walkers don't do today), it makes
>>> sense to do it this way.
>>> Or we could require userspace to zap first (MADV_DONTNEED), but that would
>>> unnecessarily mean extra syscalls for the use case of an allocator debug
>>> mode that wants to turn freed memory to guards to catch use after free.
>>> So this seems like a good compromise...
>>
>> Yes please, KIS.
>
> You mean "require userspace to zap first (MADV_DONTNEED)" ?
Yes, I see from Lorenzo's reply that there is apparently some history to
this (maybe it's all nicely summarized in the cover letter / this patch,
have to dig further).
Not sure yet what the problem is, I would have thought it's all
protected by the PTL, and concurrent faults are user space doing
something stupid and we'd detect it.
Have to do some more reading on this.
>
> I'd normally agree with the KIS principle, but..
>
>> We can always implement support for that later if
>
> it would either mean later we change behavior (installing guards on
> non-zapped PTEs would have to be an error now but maybe start working later,
> which is user observable change thus can break somebody)
>
>> really required (leave behavior open when documenting).
>
> and leaving it open when documenting doesn't really mean anything for the
> "we don't break userspace" promise vs what the implementation actually does.
Not quite I think. You could start return -EEXIST or -EOPNOTSUPP and
document that this can change in the future to succeed if there is
something. User space can sense support.
Something failing that at one point starts working is not really
breaking user space, unless someone really *wants* to fail if there is
already something (e.g., concurrent fault -> bail out instead of hiding it).
Of course, a more elegant solution would be GUARD_INSTALL vs.
GUARD_FORCE_INSTALL.
.. but again, there seems to be more history to this.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 20:30 UTC (permalink / raw)
To: Vlastimil Babka
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <fedd19ce-ea15-4ded-a1b5-ff050de15bba@suse.cz>
On Mon, Oct 21, 2024 at 10:25:06PM +0200, Vlastimil Babka wrote:
> On 10/21/24 22:17, David Hildenbrand wrote:
> > On 21.10.24 22:11, Vlastimil Babka wrote:
> >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> >>
> >> <snip>
> >>
> >>> +static long madvise_guard_poison(struct vm_area_struct *vma,
> >>> + struct vm_area_struct **prev,
> >>> + unsigned long start, unsigned long end)
> >>> +{
> >>> + long err;
> >>> +
> >>> + *prev = vma;
> >>> + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
> >>> + return -EINVAL;
> >>> +
> >>> + /*
> >>> + * If we install poison markers, then the range is no longer
> >>> + * empty from a page table perspective and therefore it's
> >>> + * appropriate to have an anon_vma.
> >>> + *
> >>> + * This ensures that on fork, we copy page tables correctly.
> >>> + */
> >>> + err = anon_vma_prepare(vma);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + /*
> >>> + * Optimistically try to install the guard poison pages first. If any
> >>> + * non-guard pages are encountered, give up and zap the range before
> >>> + * trying again.
> >>> + */
> >>
> >> Should the page walker become powerful enough to handle this in one go? :)
> >> But sure, if it's too big a task to teach it to zap ptes with all the tlb
> >> flushing etc (I assume it's something page walkers don't do today), it makes
> >> sense to do it this way.
> >> Or we could require userspace to zap first (MADV_DONTNEED), but that would
> >> unnecessarily mean extra syscalls for the use case of an allocator debug
> >> mode that wants to turn freed memory to guards to catch use after free.
> >> So this seems like a good compromise...
> >
> > Yes please, KIS.
>
> You mean "require userspace to zap first (MADV_DONTNEED)" ?
What on earth are you talking about? This is crazy, we can detect if we need to
zap with the page walker then just zap? Why would we do this?
The solution as is is perfectly simple... What is the justification for
this on any level?
Again, if you think there's a _genuine_ security/DoS issue here you're
going to really need to demonstrate it rather than hand wave?
>
> I'd normally agree with the KIS principle, but..
>
> > We can always implement support for that later if
>
> it would either mean later we change behavior (installing guards on
> non-zapped PTEs would have to be an error now but maybe start working later,
> which is user observable change thus can break somebody)
>
> > really required (leave behavior open when documenting).
>
> and leaving it open when documenting doesn't really mean anything for the
> "we don't break userspace" promise vs what the implementation actually does.
>
> Or the changed behavior would need to come with a new MADVISE mode. Not
> appealing as it's a mess already.
>
> So since its uapi we should aim for the best from the start.
>
>
Best is 'call the madvise(), guard pages installed' which is what it is
now.
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 20:27 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <c37ada68-5bf5-4ca5-9de8-c0838160c443@suse.cz>
On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > Implement a new lightweight guard page feature, that is regions of userland
> > virtual memory that, when accessed, cause a fatal signal to arise.
> >
> > Currently users must establish PROT_NONE ranges to achieve this.
> >
> > However this is very costly memory-wise - we need a VMA for each and every
> > one of these regions AND they become unmergeable with surrounding VMAs.
> >
> > In addition repeated mmap() calls require repeated kernel context switches
> > and contention of the mmap lock to install these ranges, potentially also
> > having to unmap memory if installed over existing ranges.
> >
> > The lightweight guard approach eliminates the VMA cost altogether - rather
> > than establishing a PROT_NONE VMA, it operates at the level of page table
> > entries - poisoning PTEs such that accesses to them cause a fault followed
> > by a SIGSGEV signal being raised.
> >
> > This is achieved through the PTE marker mechanism, which a previous commit
> > in this series extended to permit this to be done, installed via the
> > generic page walking logic, also extended by a prior commit for this
> > purpose.
> >
> > These poison ranges are established with MADV_GUARD_POISON, and if the
> > range in which they are installed contain any existing mappings, they will
> > be zapped, i.e. free the range and unmap memory (thus mimicking the
> > behaviour of MADV_DONTNEED in this respect).
> >
> > Any existing poison entries will be left untouched. There is no nesting of
> > poisoned pages.
> >
> > Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
> > unexpected behaviour, but are cleared on process teardown or unmapping of
> > memory ranges.
> >
> > Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
> > 'remedying' the poisoning. The ranges over which this is applied, should
> > they contain non-poison entries, will be untouched, only poison entries
> > will be cleared.
> >
> > We permit this operation on anonymous memory only, and only VMAs which are
> > non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> > drop locked pages which would be rather counterintuitive).
> >
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Suggested-by: Jann Horn <jannh@google.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> <snip>
>
> > +static long madvise_guard_poison(struct vm_area_struct *vma,
> > + struct vm_area_struct **prev,
> > + unsigned long start, unsigned long end)
> > +{
> > + long err;
> > +
> > + *prev = vma;
> > + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
> > + return -EINVAL;
> > +
> > + /*
> > + * If we install poison markers, then the range is no longer
> > + * empty from a page table perspective and therefore it's
> > + * appropriate to have an anon_vma.
> > + *
> > + * This ensures that on fork, we copy page tables correctly.
> > + */
> > + err = anon_vma_prepare(vma);
> > + if (err)
> > + return err;
> > +
> > + /*
> > + * Optimistically try to install the guard poison pages first. If any
> > + * non-guard pages are encountered, give up and zap the range before
> > + * trying again.
> > + */
>
> Should the page walker become powerful enough to handle this in one go? :)
I can tell you've not read previous threads...
I've addressed this in discussion with Jann - we'd have to do a full fat
huge comprehensive thing to do an in-place replace.
It'd either have to be fully duplicative of the multiple copies of the very
lengthily code to do this sort of thing right (some in mm/madvise.c itself)
or I'd have to go off and do a totally new pre-requisite series
centralising that in a way that people probably wouldn't accept... I'm not
sure the benefits outway the costs.
> But sure, if it's too big a task to teach it to zap ptes with all the tlb
> flushing etc (I assume it's something page walkers don't do today), it makes
> sense to do it this way.
> Or we could require userspace to zap first (MADV_DONTNEED), but that would
> unnecessarily mean extra syscalls for the use case of an allocator debug
> mode that wants to turn freed memory to guards to catch use after free.
> So this seems like a good compromise...
This is optimistic as the comment says, you very often won't need to do
this, so we do a little extra work in the case that you need to zap,
vs. the more likely case that you don't when you don't.
In the face of racing faults, which we can't reasonably prevent without
having to write _and_ VMA lock which is an egregious requirement, this
wouldn't really save us anythign anyway.
>
> > + while (true) {
> > + /* Returns < 0 on error, == 0 if success, > 0 if zap needed. */
> > + err = walk_page_range_mm(vma->vm_mm, start, end,
> > + &guard_poison_walk_ops, NULL);
> > + if (err <= 0)
> > + return err;
> > +
> > + /*
> > + * OK some of the range have non-guard pages mapped, zap
> > + * them. This leaves existing guard pages in place.
> > + */
> > + zap_page_range_single(vma, start, end - start, NULL);
>
> ... however the potentially endless loop doesn't seem great. Could a
> malicious program keep refaulting the range (ignoring any segfaults if it
> loses a race) with one thread while failing to make progress here with
> another thread? Is that ok because it would only punish itself?
Sigh. Again, I don't think you've read the previous series have you? Or
even the changelog... I added this as Jann asked for it. Originally we'd
-EAGAIN if we got raced. See the discussion over in v1 for details.
I did it that way specifically to avoid such things, but Jann didn't appear
to think it was a problem.
>
> I mean if we have to retry the guards page installation more than once, it
> means the program *is* racing faults with installing guard ptes in the same
> range, right? So it would be right to segfault it. But I guess when we
> detect it here, we have no way to send the signal to the right thread and it
> would be too late? So unless we can do the PTE zap+install marker
> atomically, maybe there's no better way and this is acceptable as a
> malicious program can harm only itself?
Yup you'd only be hurting yourself. I went over this with Jann, who didn't
appear to have a problem with this approach from a security perspective, in
fact he explicitly asked me to do this :)
>
> Maybe it would be just simpler to install the marker via zap_details rather
> than the pagewalk?
Ah the inevitable 'please completely rework how you do everything' comment
I was expecting at some point :)
Obviously I've considered this (and a number of other approaches), it would
fundamentally change what zap is - right now if it can't traverse a page
table level that job done (it's job is to remove PTEs not create).
We'd instead have to completely rework the logic to be able to _install_
page tables and then carefully check we don't break anything and only do it
in the specific cases we need.
Or we could add a mode that says 'replace with a poison marker' rather than
zap, but that has potential TLB concerns, splits it across two operations
(installation and zapping), and then could you really be sure that there
isn't a really really badly timed race that would mean you'd have to loop
again?
Right now it's simple, elegant, small and we can only make ourselves
wait. I don't think this is a huge problem.
I think I'll need an actual security/DoS-based justification to change this.
>
> > +
> > + if (fatal_signal_pending(current))
> > + return -EINTR;
> > + cond_resched();
> > + }
> > +}
> > +
> > +static int guard_unpoison_pte_entry(pte_t *pte, unsigned long addr,
> > + unsigned long next, struct mm_walk *walk)
> > +{
> > + pte_t ptent = ptep_get(pte);
> > +
> > + if (is_guard_pte_marker(ptent)) {
> > + /* Simply clear the PTE marker. */
> > + pte_clear_not_present_full(walk->mm, addr, pte, false);
> > + update_mmu_cache(walk->vma, addr, pte);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct mm_walk_ops guard_unpoison_walk_ops = {
> > + .pte_entry = guard_unpoison_pte_entry,
> > + .walk_lock = PGWALK_RDLOCK,
> > +};
> > +
> > +static long madvise_guard_unpoison(struct vm_area_struct *vma,
> > + struct vm_area_struct **prev,
> > + unsigned long start, unsigned long end)
> > +{
> > + *prev = vma;
> > + /*
> > + * We're ok with unpoisoning mlock()'d ranges, as this is a
> > + * non-destructive action.
> > + */
> > + if (!is_valid_guard_vma(vma, /* allow_locked = */true))
> > + return -EINVAL;
> > +
> > + return walk_page_range(vma->vm_mm, start, end,
> > + &guard_unpoison_walk_ops, NULL);
> > +}
> > +
> > /*
> > * Apply an madvise behavior to a region of a vma. madvise_update_vma
> > * will handle splitting a vm area into separate areas, each area with its own
> > @@ -1098,6 +1260,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> > break;
> > case MADV_COLLAPSE:
> > return madvise_collapse(vma, prev, start, end);
> > + case MADV_GUARD_POISON:
> > + return madvise_guard_poison(vma, prev, start, end);
> > + case MADV_GUARD_UNPOISON:
> > + return madvise_guard_unpoison(vma, prev, start, end);
> > }
> >
> > anon_name = anon_vma_name(vma);
> > @@ -1197,6 +1363,8 @@ madvise_behavior_valid(int behavior)
> > case MADV_DODUMP:
> > case MADV_WIPEONFORK:
> > case MADV_KEEPONFORK:
> > + case MADV_GUARD_POISON:
> > + case MADV_GUARD_UNPOISON:
> > #ifdef CONFIG_MEMORY_FAILURE
> > case MADV_SOFT_OFFLINE:
> > case MADV_HWPOISON:
> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 0c5d6d06107d..d0e3ebfadef8 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -236,7 +236,8 @@ static long change_pte_range(struct mmu_gather *tlb,
> > } else if (is_pte_marker_entry(entry)) {
> > /*
> > * Ignore error swap entries unconditionally,
> > - * because any access should sigbus anyway.
> > + * because any access should sigbus/sigsegv
> > + * anyway.
> > */
> > if (is_poisoned_swp_entry(entry))
> > continue;
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index ece977bd21e1..21bf5534bcf5 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -30,6 +30,7 @@ static bool is_madv_discard(int behavior)
> > case MADV_REMOVE:
> > case MADV_DONTFORK:
> > case MADV_WIPEONFORK:
> > + case MADV_GUARD_POISON:
> > return true;
> > }
> >
>
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Vlastimil Babka @ 2024-10-21 20:25 UTC (permalink / raw)
To: David Hildenbrand, Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <6c282299-506f-45c9-9ddc-9ef4de582394@redhat.com>
On 10/21/24 22:17, David Hildenbrand wrote:
> On 21.10.24 22:11, Vlastimil Babka wrote:
>> On 10/20/24 18:20, Lorenzo Stoakes wrote:
>>
>> <snip>
>>
>>> +static long madvise_guard_poison(struct vm_area_struct *vma,
>>> + struct vm_area_struct **prev,
>>> + unsigned long start, unsigned long end)
>>> +{
>>> + long err;
>>> +
>>> + *prev = vma;
>>> + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * If we install poison markers, then the range is no longer
>>> + * empty from a page table perspective and therefore it's
>>> + * appropriate to have an anon_vma.
>>> + *
>>> + * This ensures that on fork, we copy page tables correctly.
>>> + */
>>> + err = anon_vma_prepare(vma);
>>> + if (err)
>>> + return err;
>>> +
>>> + /*
>>> + * Optimistically try to install the guard poison pages first. If any
>>> + * non-guard pages are encountered, give up and zap the range before
>>> + * trying again.
>>> + */
>>
>> Should the page walker become powerful enough to handle this in one go? :)
>> But sure, if it's too big a task to teach it to zap ptes with all the tlb
>> flushing etc (I assume it's something page walkers don't do today), it makes
>> sense to do it this way.
>> Or we could require userspace to zap first (MADV_DONTNEED), but that would
>> unnecessarily mean extra syscalls for the use case of an allocator debug
>> mode that wants to turn freed memory to guards to catch use after free.
>> So this seems like a good compromise...
>
> Yes please, KIS.
You mean "require userspace to zap first (MADV_DONTNEED)" ?
I'd normally agree with the KIS principle, but..
> We can always implement support for that later if
it would either mean later we change behavior (installing guards on
non-zapped PTEs would have to be an error now but maybe start working later,
which is user observable change thus can break somebody)
> really required (leave behavior open when documenting).
and leaving it open when documenting doesn't really mean anything for the
"we don't break userspace" promise vs what the implementation actually does.
Or the changed behavior would need to come with a new MADVISE mode. Not
appealing as it's a mess already.
So since its uapi we should aim for the best from the start.
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 20:18 UTC (permalink / raw)
To: Lorenzo Stoakes, John Hubbard
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api
In-Reply-To: <a4b7ed07-3b14-4443-aa76-77d720912cd0@lucifer.local>
On 21.10.24 21:39, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 12:25:08PM -0700, John Hubbard wrote:
>> On 10/21/24 10:23 AM, David Hildenbrand wrote:
> [snip]
>>>>> Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth. like
>>>>> that would have been even clearer, at least to me.
>>
>> Yes, I think so.
>>
>>>>
>>>> :)
>>>>
>>>> It still feels like poisoning to me because we're explicitly putting
>>>> something in the page tables to make a range have different fault behaviour
>>>> like a HW poisoning, and 'installing' suggests backing or something like
>>>> this, I think that's more confusing.
>>>
>>> I connect "poison" to "SIGBUS" and "corrupt memory state", not to "there is nothing and there must not be anything". Thus my thinking. But again, not the end of the world, just wanted to raise it ...
>>
>> "Poison" is used so far for fairly distinct things, and I'd very much like
>> to avoid extending its meaning to guard pages. It makes the other things
>> less unique, and it misses a naming and classification opportunity.
>>
>> "Guard" and "guard page" are fairly unique names. That's valuable.
>>
>>
>> thanks,
>> --
>> John Hubbard
>>
>
> Guys you're breaking my heart... Will you not leave me with even a remnant
> of a cultural reference?? [0]
I'm sure you will recover from that loss :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 20:17 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <c37ada68-5bf5-4ca5-9de8-c0838160c443@suse.cz>
On 21.10.24 22:11, Vlastimil Babka wrote:
> On 10/20/24 18:20, Lorenzo Stoakes wrote:
>> Implement a new lightweight guard page feature, that is regions of userland
>> virtual memory that, when accessed, cause a fatal signal to arise.
>>
>> Currently users must establish PROT_NONE ranges to achieve this.
>>
>> However this is very costly memory-wise - we need a VMA for each and every
>> one of these regions AND they become unmergeable with surrounding VMAs.
>>
>> In addition repeated mmap() calls require repeated kernel context switches
>> and contention of the mmap lock to install these ranges, potentially also
>> having to unmap memory if installed over existing ranges.
>>
>> The lightweight guard approach eliminates the VMA cost altogether - rather
>> than establishing a PROT_NONE VMA, it operates at the level of page table
>> entries - poisoning PTEs such that accesses to them cause a fault followed
>> by a SIGSGEV signal being raised.
>>
>> This is achieved through the PTE marker mechanism, which a previous commit
>> in this series extended to permit this to be done, installed via the
>> generic page walking logic, also extended by a prior commit for this
>> purpose.
>>
>> These poison ranges are established with MADV_GUARD_POISON, and if the
>> range in which they are installed contain any existing mappings, they will
>> be zapped, i.e. free the range and unmap memory (thus mimicking the
>> behaviour of MADV_DONTNEED in this respect).
>>
>> Any existing poison entries will be left untouched. There is no nesting of
>> poisoned pages.
>>
>> Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
>> unexpected behaviour, but are cleared on process teardown or unmapping of
>> memory ranges.
>>
>> Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
>> 'remedying' the poisoning. The ranges over which this is applied, should
>> they contain non-poison entries, will be untouched, only poison entries
>> will be cleared.
>>
>> We permit this operation on anonymous memory only, and only VMAs which are
>> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
>> drop locked pages which would be rather counterintuitive).
>>
>> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
>> Suggested-by: Jann Horn <jannh@google.com>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> <snip>
>
>> +static long madvise_guard_poison(struct vm_area_struct *vma,
>> + struct vm_area_struct **prev,
>> + unsigned long start, unsigned long end)
>> +{
>> + long err;
>> +
>> + *prev = vma;
>> + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
>> + return -EINVAL;
>> +
>> + /*
>> + * If we install poison markers, then the range is no longer
>> + * empty from a page table perspective and therefore it's
>> + * appropriate to have an anon_vma.
>> + *
>> + * This ensures that on fork, we copy page tables correctly.
>> + */
>> + err = anon_vma_prepare(vma);
>> + if (err)
>> + return err;
>> +
>> + /*
>> + * Optimistically try to install the guard poison pages first. If any
>> + * non-guard pages are encountered, give up and zap the range before
>> + * trying again.
>> + */
>
> Should the page walker become powerful enough to handle this in one go? :)
> But sure, if it's too big a task to teach it to zap ptes with all the tlb
> flushing etc (I assume it's something page walkers don't do today), it makes
> sense to do it this way.
> Or we could require userspace to zap first (MADV_DONTNEED), but that would
> unnecessarily mean extra syscalls for the use case of an allocator debug
> mode that wants to turn freed memory to guards to catch use after free.
> So this seems like a good compromise...
Yes please, KIS. We can always implement support for that later if
really required (leave behavior open when documenting).
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Vlastimil Babka @ 2024-10-21 20:11 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <fce49bbbfe41b82161a37b022c8eb1e6c20e1d85.1729440856.git.lorenzo.stoakes@oracle.com>
On 10/20/24 18:20, Lorenzo Stoakes wrote:
> Implement a new lightweight guard page feature, that is regions of userland
> virtual memory that, when accessed, cause a fatal signal to arise.
>
> Currently users must establish PROT_NONE ranges to achieve this.
>
> However this is very costly memory-wise - we need a VMA for each and every
> one of these regions AND they become unmergeable with surrounding VMAs.
>
> In addition repeated mmap() calls require repeated kernel context switches
> and contention of the mmap lock to install these ranges, potentially also
> having to unmap memory if installed over existing ranges.
>
> The lightweight guard approach eliminates the VMA cost altogether - rather
> than establishing a PROT_NONE VMA, it operates at the level of page table
> entries - poisoning PTEs such that accesses to them cause a fault followed
> by a SIGSGEV signal being raised.
>
> This is achieved through the PTE marker mechanism, which a previous commit
> in this series extended to permit this to be done, installed via the
> generic page walking logic, also extended by a prior commit for this
> purpose.
>
> These poison ranges are established with MADV_GUARD_POISON, and if the
> range in which they are installed contain any existing mappings, they will
> be zapped, i.e. free the range and unmap memory (thus mimicking the
> behaviour of MADV_DONTNEED in this respect).
>
> Any existing poison entries will be left untouched. There is no nesting of
> poisoned pages.
>
> Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
> unexpected behaviour, but are cleared on process teardown or unmapping of
> memory ranges.
>
> Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
> 'remedying' the poisoning. The ranges over which this is applied, should
> they contain non-poison entries, will be untouched, only poison entries
> will be cleared.
>
> We permit this operation on anonymous memory only, and only VMAs which are
> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> drop locked pages which would be rather counterintuitive).
>
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Suggested-by: Jann Horn <jannh@google.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
<snip>
> +static long madvise_guard_poison(struct vm_area_struct *vma,
> + struct vm_area_struct **prev,
> + unsigned long start, unsigned long end)
> +{
> + long err;
> +
> + *prev = vma;
> + if (!is_valid_guard_vma(vma, /* allow_locked = */false))
> + return -EINVAL;
> +
> + /*
> + * If we install poison markers, then the range is no longer
> + * empty from a page table perspective and therefore it's
> + * appropriate to have an anon_vma.
> + *
> + * This ensures that on fork, we copy page tables correctly.
> + */
> + err = anon_vma_prepare(vma);
> + if (err)
> + return err;
> +
> + /*
> + * Optimistically try to install the guard poison pages first. If any
> + * non-guard pages are encountered, give up and zap the range before
> + * trying again.
> + */
Should the page walker become powerful enough to handle this in one go? :)
But sure, if it's too big a task to teach it to zap ptes with all the tlb
flushing etc (I assume it's something page walkers don't do today), it makes
sense to do it this way.
Or we could require userspace to zap first (MADV_DONTNEED), but that would
unnecessarily mean extra syscalls for the use case of an allocator debug
mode that wants to turn freed memory to guards to catch use after free.
So this seems like a good compromise...
> + while (true) {
> + /* Returns < 0 on error, == 0 if success, > 0 if zap needed. */
> + err = walk_page_range_mm(vma->vm_mm, start, end,
> + &guard_poison_walk_ops, NULL);
> + if (err <= 0)
> + return err;
> +
> + /*
> + * OK some of the range have non-guard pages mapped, zap
> + * them. This leaves existing guard pages in place.
> + */
> + zap_page_range_single(vma, start, end - start, NULL);
... however the potentially endless loop doesn't seem great. Could a
malicious program keep refaulting the range (ignoring any segfaults if it
loses a race) with one thread while failing to make progress here with
another thread? Is that ok because it would only punish itself?
I mean if we have to retry the guards page installation more than once, it
means the program *is* racing faults with installing guard ptes in the same
range, right? So it would be right to segfault it. But I guess when we
detect it here, we have no way to send the signal to the right thread and it
would be too late? So unless we can do the PTE zap+install marker
atomically, maybe there's no better way and this is acceptable as a
malicious program can harm only itself?
Maybe it would be just simpler to install the marker via zap_details rather
than the pagewalk?
> +
> + if (fatal_signal_pending(current))
> + return -EINTR;
> + cond_resched();
> + }
> +}
> +
> +static int guard_unpoison_pte_entry(pte_t *pte, unsigned long addr,
> + unsigned long next, struct mm_walk *walk)
> +{
> + pte_t ptent = ptep_get(pte);
> +
> + if (is_guard_pte_marker(ptent)) {
> + /* Simply clear the PTE marker. */
> + pte_clear_not_present_full(walk->mm, addr, pte, false);
> + update_mmu_cache(walk->vma, addr, pte);
> + }
> +
> + return 0;
> +}
> +
> +static const struct mm_walk_ops guard_unpoison_walk_ops = {
> + .pte_entry = guard_unpoison_pte_entry,
> + .walk_lock = PGWALK_RDLOCK,
> +};
> +
> +static long madvise_guard_unpoison(struct vm_area_struct *vma,
> + struct vm_area_struct **prev,
> + unsigned long start, unsigned long end)
> +{
> + *prev = vma;
> + /*
> + * We're ok with unpoisoning mlock()'d ranges, as this is a
> + * non-destructive action.
> + */
> + if (!is_valid_guard_vma(vma, /* allow_locked = */true))
> + return -EINVAL;
> +
> + return walk_page_range(vma->vm_mm, start, end,
> + &guard_unpoison_walk_ops, NULL);
> +}
> +
> /*
> * Apply an madvise behavior to a region of a vma. madvise_update_vma
> * will handle splitting a vm area into separate areas, each area with its own
> @@ -1098,6 +1260,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> break;
> case MADV_COLLAPSE:
> return madvise_collapse(vma, prev, start, end);
> + case MADV_GUARD_POISON:
> + return madvise_guard_poison(vma, prev, start, end);
> + case MADV_GUARD_UNPOISON:
> + return madvise_guard_unpoison(vma, prev, start, end);
> }
>
> anon_name = anon_vma_name(vma);
> @@ -1197,6 +1363,8 @@ madvise_behavior_valid(int behavior)
> case MADV_DODUMP:
> case MADV_WIPEONFORK:
> case MADV_KEEPONFORK:
> + case MADV_GUARD_POISON:
> + case MADV_GUARD_UNPOISON:
> #ifdef CONFIG_MEMORY_FAILURE
> case MADV_SOFT_OFFLINE:
> case MADV_HWPOISON:
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 0c5d6d06107d..d0e3ebfadef8 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -236,7 +236,8 @@ static long change_pte_range(struct mmu_gather *tlb,
> } else if (is_pte_marker_entry(entry)) {
> /*
> * Ignore error swap entries unconditionally,
> - * because any access should sigbus anyway.
> + * because any access should sigbus/sigsegv
> + * anyway.
> */
> if (is_poisoned_swp_entry(entry))
> continue;
> diff --git a/mm/mseal.c b/mm/mseal.c
> index ece977bd21e1..21bf5534bcf5 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -30,6 +30,7 @@ static bool is_madv_discard(int behavior)
> case MADV_REMOVE:
> case MADV_DONTFORK:
> case MADV_WIPEONFORK:
> + case MADV_GUARD_POISON:
> return true;
> }
>
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 19:57 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <9c0991db-9bf8-414c-b3b0-446023df2a7a@suse.cz>
On Mon, Oct 21, 2024 at 03:45:31PM +0200, Vlastimil Babka wrote:
[snip]
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
Thanks!
>
> A nit below:
>
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 906294ac85dc..50e3f6ed73ac 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6353,6 +6353,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> > ret = VM_FAULT_HWPOISON_LARGE |
> > VM_FAULT_SET_HINDEX(hstate_index(h));
> > goto out_mutex;
> > + } else if (marker & PTE_MARKER_GUARD) {
> > + ret = VM_FAULT_SIGSEGV;
> > + goto out_mutex;
>
> Given we don't support hugetlb, should we WARN_ON_ONCE() if such unexpected
> marker appears there?
Yes agreed, will add.
[snip]
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 19:39 UTC (permalink / raw)
To: John Hubbard
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api
In-Reply-To: <3baf8814-0a9a-4de0-b568-62d241dbba0e@nvidia.com>
On Mon, Oct 21, 2024 at 12:25:08PM -0700, John Hubbard wrote:
> On 10/21/24 10:23 AM, David Hildenbrand wrote:
[snip]
> > > > Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth. like
> > > > that would have been even clearer, at least to me.
>
> Yes, I think so.
>
> > >
> > > :)
> > >
> > > It still feels like poisoning to me because we're explicitly putting
> > > something in the page tables to make a range have different fault behaviour
> > > like a HW poisoning, and 'installing' suggests backing or something like
> > > this, I think that's more confusing.
> >
> > I connect "poison" to "SIGBUS" and "corrupt memory state", not to "there is nothing and there must not be anything". Thus my thinking. But again, not the end of the world, just wanted to raise it ...
>
> "Poison" is used so far for fairly distinct things, and I'd very much like
> to avoid extending its meaning to guard pages. It makes the other things
> less unique, and it misses a naming and classification opportunity.
>
> "Guard" and "guard page" are fairly unique names. That's valuable.
>
>
> thanks,
> --
> John Hubbard
>
Guys you're breaking my heart... Will you not leave me with even a remnant
of a cultural reference?? [0]
[0]:https://www.youtube.com/watch?v=_mej5wS7viw
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: John Hubbard @ 2024-10-21 19:25 UTC (permalink / raw)
To: David Hildenbrand, Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api
In-Reply-To: <c8272b9d-5c33-4b44-9d6d-1d25c7ac23dd@redhat.com>
On 10/21/24 10:23 AM, David Hildenbrand wrote:
> On 21.10.24 19:15, Lorenzo Stoakes wrote:
>> On Mon, Oct 21, 2024 at 07:05:27PM +0200, David Hildenbrand wrote:
>>> On 20.10.24 18:20, Lorenzo Stoakes wrote:
>>>> Implement a new lightweight guard page feature, that is regions of userland
>>>> virtual memory that, when accessed, cause a fatal signal to arise.
>>>>
>>>> Currently users must establish PROT_NONE ranges to achieve this.
>>>>
>>>> However this is very costly memory-wise - we need a VMA for each and every
>>>> one of these regions AND they become unmergeable with surrounding VMAs.
>>>>
>>>> In addition repeated mmap() calls require repeated kernel context switches
>>>> and contention of the mmap lock to install these ranges, potentially also
>>>> having to unmap memory if installed over existing ranges.
>>>>
>>>> The lightweight guard approach eliminates the VMA cost altogether - rather
>>>> than establishing a PROT_NONE VMA, it operates at the level of page table
>>>> entries - poisoning PTEs such that accesses to them cause a fault followed
>>>> by a SIGSGEV signal being raised.
>>>>
>>>> This is achieved through the PTE marker mechanism, which a previous commit
>>>> in this series extended to permit this to be done, installed via the
>>>> generic page walking logic, also extended by a prior commit for this
>>>> purpose.
>>>>
>>>> These poison ranges are established with MADV_GUARD_POISON, and if the
>>>> range in which they are installed contain any existing mappings, they will
>>>> be zapped, i.e. free the range and unmap memory (thus mimicking the
>>>> behaviour of MADV_DONTNEED in this respect).
>>>>
>>>> Any existing poison entries will be left untouched. There is no nesting of
>>>> poisoned pages.
>>>>
>>>> Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
>>>> unexpected behaviour, but are cleared on process teardown or unmapping of
>>>> memory ranges.
>>>>
>>>> Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
>>>> 'remedying' the poisoning. The ranges over which this is applied, should
>>>> they contain non-poison entries, will be untouched, only poison entries
>>>> will be cleared.
>>>>
>>>> We permit this operation on anonymous memory only, and only VMAs which are
>>>> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
>>>> drop locked pages which would be rather counterintuitive).
>>>>
>>>> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
>>>> Suggested-by: Jann Horn <jannh@google.com>
>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>>> ---
>>>> 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/uapi/asm-generic/mman-common.h | 3 +
>>>> mm/madvise.c | 168 +++++++++++++++++++++++++
>>>> mm/mprotect.c | 3 +-
>>>> mm/mseal.c | 1 +
>>>> 8 files changed, 186 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
>>>> index 763929e814e9..71e13f27742d 100644
>>>> --- a/arch/alpha/include/uapi/asm/mman.h
>>>> +++ b/arch/alpha/include/uapi/asm/mman.h
>>>> @@ -78,6 +78,9 @@
>>>> #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
>>>> +#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
>>>> +#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
>>>
>>> Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth. like
>>> that would have been even clearer, at least to me.
Yes, I think so.
>>
>> :)
>>
>> It still feels like poisoning to me because we're explicitly putting
>> something in the page tables to make a range have different fault behaviour
>> like a HW poisoning, and 'installing' suggests backing or something like
>> this, I think that's more confusing.
>
> I connect "poison" to "SIGBUS" and "corrupt memory state", not to "there is nothing and there must not be anything". Thus my thinking. But again, not the end of the world, just wanted to raise it ...
"Poison" is used so far for fairly distinct things, and I'd very much like
to avoid extending its meaning to guard pages. It makes the other things
less unique, and it misses a naming and classification opportunity.
"Guard" and "guard page" are fairly unique names. That's valuable.
thanks,
--
John Hubbard
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Vlastimil Babka @ 2024-10-21 17:26 UTC (permalink / raw)
To: Lorenzo Stoakes, David Hildenbrand
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <acf358a4-c503-4347-8156-9269c43bf796@lucifer.local>
On 10/21/24 19:14, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 07:00:53PM +0200, David Hildenbrand wrote:
>>
>> >
>> > >
>> > > >
>> > > > Also the existing logic is that existing markers (HW poison, uffd-simulated HW
>> > > > poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
>> > > > no error on MADV_FREE either, so it'd be consistent with existing behaviour.
>> > >
>> > >
>> > > HW poison / uffd-simulated HW poison are expected to be zapped: it's just
>> > > like a mapped page with HWPOISON. So that is correct.
>> >
>> > Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
>>
>> Huh?
>>
>> madvise_dontneed_single_vma()->zap_page_range_single(details=NULL)->unmap_single_vma(details=NULL)
>> ... zap_pte_range()
>>
>> } else if (is_hwpoison_entry(entry) ||
>> is_poisoned_swp_entry(entry)) {
>> if (!should_zap_cows(details))
>> continue;
>> ...
>>
>> Should just zap them.
>>
>> What am I missing?
>
> Yeah ok it's me who's missing something here, I hadn't noticed details == NULL
> so should_zap_cows() is true, my mistake!
Well, good to know it's consistent then. As I've explained I see why zapping
actual hwpoison makes sense for MADV_DONTNEED/MADV_FREE. That it's done also
for uffd poison is not completely clear, but maybe it was just easier to
implement. But it doesn't mean we have to do the same for GUARD PTEs. Either
behavior of zap/ignore/error could be valid, we just have to pick one and
then live with it as it can't change :) Zapping guards on DONTNEED/FREE
seems wrong to me, so it's between error (and potentially catching some
misuse) and ignore (potentially more performant in case somebody wants to
DOTNEED/FREE an area that contains scattered guards).
And the impossibility to meaningfully unwind on errors in the middle of the
operation (unless we pre-scan for guards) isn't exactly nice, so maybe just
ignore, i.e. the current approach?
> In any case we explicitly add code here to prevent guard pages from going. I
> will correct everything where I wrongly say otherwise, doh!
>
>>
>> > mean the MADV flags are a confusing mess generally, as per Vlasta's comments
>> > which to begin with I strongly disagreed with then, discussing further, realsed
>> > that no this is just a bit insane and had driven _me_ insane.
>> >
>> > >
>> > > UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap uffd-wp
>> > > entries.
>> > >
>> > > >
>> > > > Also semantically you are achieving what the calls expect you are freeing the
>> > > > ranges since the guard page regions are unbacked so are already freed... so yeah
>> > > > I don't think an error really makes sense here.
>> > >
>> > > I you compare it to a VMA hole, it make sense to fail. If we treat it like
>> > > PROT_NONE, it make sense to skip them.
>> > >
>> > > >
>> > > > We might also be limiting use cases by assuming they might _only_ be used for
>> > > > allocators and such.
>> > >
>> > > I don't buy that as an argument, sorry :)
>> > >
>> > > "Let's map the kernel writable into all user space because otherwise we
>> > > might be limiting use cases"
>> >
>> > That's a great idea! Patch series incoming, 1st April 2025... :>)
>>
>> :) Just flip the bit on x86 and we're done!
>
> ;)
>
>>
>> > >
>> > >
>> > > :P
>> > >
>> > > --
>> > > Cheers,
>> > >
>> > > David / dhildenb
>> > >
>> >
>> > Overall I think just always leaving in place except on remedy err sorry sorry
>> > unpoison and munmap and not returning an error if encountered elsewhere (other
>> > than, of course, GUP) is the right way forward and most in line with user
>> > expectation and practical usage.
>>
>>
>> Fine with me, make sure to document that is behaves like a PROT_NONE VMA,
>> not like a memory hole, except when something would trigger a fault (GUP
>> etc).
>
> Ack will make sure to document.
>
>>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 17:23 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <b2bca752-77f3-4b63-abe9-348a5fc2a5cc@lucifer.local>
On 21.10.24 19:15, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 07:05:27PM +0200, David Hildenbrand wrote:
>> On 20.10.24 18:20, Lorenzo Stoakes wrote:
>>> Implement a new lightweight guard page feature, that is regions of userland
>>> virtual memory that, when accessed, cause a fatal signal to arise.
>>>
>>> Currently users must establish PROT_NONE ranges to achieve this.
>>>
>>> However this is very costly memory-wise - we need a VMA for each and every
>>> one of these regions AND they become unmergeable with surrounding VMAs.
>>>
>>> In addition repeated mmap() calls require repeated kernel context switches
>>> and contention of the mmap lock to install these ranges, potentially also
>>> having to unmap memory if installed over existing ranges.
>>>
>>> The lightweight guard approach eliminates the VMA cost altogether - rather
>>> than establishing a PROT_NONE VMA, it operates at the level of page table
>>> entries - poisoning PTEs such that accesses to them cause a fault followed
>>> by a SIGSGEV signal being raised.
>>>
>>> This is achieved through the PTE marker mechanism, which a previous commit
>>> in this series extended to permit this to be done, installed via the
>>> generic page walking logic, also extended by a prior commit for this
>>> purpose.
>>>
>>> These poison ranges are established with MADV_GUARD_POISON, and if the
>>> range in which they are installed contain any existing mappings, they will
>>> be zapped, i.e. free the range and unmap memory (thus mimicking the
>>> behaviour of MADV_DONTNEED in this respect).
>>>
>>> Any existing poison entries will be left untouched. There is no nesting of
>>> poisoned pages.
>>>
>>> Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
>>> unexpected behaviour, but are cleared on process teardown or unmapping of
>>> memory ranges.
>>>
>>> Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
>>> 'remedying' the poisoning. The ranges over which this is applied, should
>>> they contain non-poison entries, will be untouched, only poison entries
>>> will be cleared.
>>>
>>> We permit this operation on anonymous memory only, and only VMAs which are
>>> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
>>> drop locked pages which would be rather counterintuitive).
>>>
>>> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
>>> Suggested-by: Jann Horn <jannh@google.com>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>> 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/uapi/asm-generic/mman-common.h | 3 +
>>> mm/madvise.c | 168 +++++++++++++++++++++++++
>>> mm/mprotect.c | 3 +-
>>> mm/mseal.c | 1 +
>>> 8 files changed, 186 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
>>> index 763929e814e9..71e13f27742d 100644
>>> --- a/arch/alpha/include/uapi/asm/mman.h
>>> +++ b/arch/alpha/include/uapi/asm/mman.h
>>> @@ -78,6 +78,9 @@
>>> #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
>>> +#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
>>> +#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
>>
>> Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth. like
>> that would have been even clearer, at least to me.
>
> :)
>
> It still feels like poisoning to me because we're explicitly putting
> something in the page tables to make a range have different fault behaviour
> like a HW poisoning, and 'installing' suggests backing or something like
> this, I think that's more confusing.
I connect "poison" to "SIGBUS" and "corrupt memory state", not to "there
is nothing and there must not be anything". Thus my thinking. But again,
not the end of the world, just wanted to raise it ...
>
>>
>> But no strong opinion, just if somebody else reading along was wondering
>> about the same.
>>
>>
>> I'm hoping to find more time to have a closer look at this this week, but in
>> general, the concept sounds reasonable to me.
>
> Thanks!
Thank you for implementing this and up-streaming it!
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: David Hildenbrand @ 2024-10-21 17:21 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <acf358a4-c503-4347-8156-9269c43bf796@lucifer.local>
>>
>>>
>>>>
>>>>>
>>>>> Also the existing logic is that existing markers (HW poison, uffd-simulated HW
>>>>> poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
>>>>> no error on MADV_FREE either, so it'd be consistent with existing behaviour.
>>>>
>>>>
>>>> HW poison / uffd-simulated HW poison are expected to be zapped: it's just
>>>> like a mapped page with HWPOISON. So that is correct.
>>>
>>> Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
>>
>> Huh?
>>
>> madvise_dontneed_single_vma()->zap_page_range_single(details=NULL)->unmap_single_vma(details=NULL)
>> ... zap_pte_range()
>>
>> } else if (is_hwpoison_entry(entry) ||
>> is_poisoned_swp_entry(entry)) {
>> if (!should_zap_cows(details))
>> continue;
>> ...
>>
>> Should just zap them.
>>
>> What am I missing?
>
> Yeah ok it's me who's missing something here, I hadn't noticed details == NULL
> so should_zap_cows() is true, my mistake!
It's confusing code ... I once had a patch to call it
"should_zap_anon_folios" etc, but it would only slightly make it clearer
what is actually happening.
>
> In any case we explicitly add code here to prevent guard pages from going. I
> will correct everything where I wrongly say otherwise, doh!
Right, that's also one of the reasons I don't think "poison" terminology
is the best fit, because there are som subtle differences. At least the
marker does not mention "poison" but PTE_MARKER_GUARD, which I think is
a very good naming :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 17:15 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <b13a83f4-c31c-441d-b18e-d63d78c4b2fb@redhat.com>
On Mon, Oct 21, 2024 at 07:05:27PM +0200, David Hildenbrand wrote:
> On 20.10.24 18:20, Lorenzo Stoakes wrote:
> > Implement a new lightweight guard page feature, that is regions of userland
> > virtual memory that, when accessed, cause a fatal signal to arise.
> >
> > Currently users must establish PROT_NONE ranges to achieve this.
> >
> > However this is very costly memory-wise - we need a VMA for each and every
> > one of these regions AND they become unmergeable with surrounding VMAs.
> >
> > In addition repeated mmap() calls require repeated kernel context switches
> > and contention of the mmap lock to install these ranges, potentially also
> > having to unmap memory if installed over existing ranges.
> >
> > The lightweight guard approach eliminates the VMA cost altogether - rather
> > than establishing a PROT_NONE VMA, it operates at the level of page table
> > entries - poisoning PTEs such that accesses to them cause a fault followed
> > by a SIGSGEV signal being raised.
> >
> > This is achieved through the PTE marker mechanism, which a previous commit
> > in this series extended to permit this to be done, installed via the
> > generic page walking logic, also extended by a prior commit for this
> > purpose.
> >
> > These poison ranges are established with MADV_GUARD_POISON, and if the
> > range in which they are installed contain any existing mappings, they will
> > be zapped, i.e. free the range and unmap memory (thus mimicking the
> > behaviour of MADV_DONTNEED in this respect).
> >
> > Any existing poison entries will be left untouched. There is no nesting of
> > poisoned pages.
> >
> > Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
> > unexpected behaviour, but are cleared on process teardown or unmapping of
> > memory ranges.
> >
> > Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
> > 'remedying' the poisoning. The ranges over which this is applied, should
> > they contain non-poison entries, will be untouched, only poison entries
> > will be cleared.
> >
> > We permit this operation on anonymous memory only, and only VMAs which are
> > non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> > drop locked pages which would be rather counterintuitive).
> >
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Suggested-by: Jann Horn <jannh@google.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > 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/uapi/asm-generic/mman-common.h | 3 +
> > mm/madvise.c | 168 +++++++++++++++++++++++++
> > mm/mprotect.c | 3 +-
> > mm/mseal.c | 1 +
> > 8 files changed, 186 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> > index 763929e814e9..71e13f27742d 100644
> > --- a/arch/alpha/include/uapi/asm/mman.h
> > +++ b/arch/alpha/include/uapi/asm/mman.h
> > @@ -78,6 +78,9 @@
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
> > +#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
>
> Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth. like
> that would have been even clearer, at least to me.
:)
It still feels like poisoning to me because we're explicitly putting
something in the page tables to make a range have different fault behaviour
like a HW poisoning, and 'installing' suggests backing or something like
this, I think that's more confusing.
>
> But no strong opinion, just if somebody else reading along was wondering
> about the same.
>
>
> I'm hoping to find more time to have a closer look at this this week, but in
> general, the concept sounds reasonable to me.
Thanks!
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 17:14 UTC (permalink / raw)
To: David Hildenbrand
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <ef0e11c5-13cf-4d47-a277-41da317be165@redhat.com>
On Mon, Oct 21, 2024 at 07:00:53PM +0200, David Hildenbrand wrote:
> On 21.10.24 18:51, Lorenzo Stoakes wrote:
> > On Mon, Oct 21, 2024 at 06:44:04PM +0200, David Hildenbrand wrote:
> > > On 21.10.24 18:23, Lorenzo Stoakes wrote:
> > > > On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
> > > > [snip]
> > > > > >
> > > > > > To summarise for on-list:
> > > > > >
> > > > > > * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
> > > > > > ability to be 'cancelled' if you write to the memory. Also, after the
> > > > > > freeing is complete, you can write to the memory to reuse it, the mapping
> > > > > > is still there.
> > > > > >
> > > > > > * For hardware poison markers it makes sense to drop them as you're
> > > > > > effectively saying 'I am done with this range that is now unbacked and
> > > > > > expect to get an empty page should I use it now'. UFFD WP I am not sure
> > > > > > about but presumably also fine.
> > > > > >
> > > > > > * However, guard pages are different - if you 'cancel' and you are left
> > > > > > with a block of memory allocated to you by a pthread or userland
> > > > > > allocator implementation, you don't want to then no longer be protected
> > > > > > from overrunning into other thread memory.
> > > > >
> > > > > Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
> > > > > error? It sounds like a usage "error" to me (in contrast to munmap()).
> > > >
> > > > It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
> > > > place, from v3 we will do the same for MADV_FREE.
> > > >
> > > > I'm not sure I'd say it's an error per se, as somebody might have a use case
> > > > where they want to zap over a range but keep guard pages, perhaps an allocator
> > > > or something?
> > >
> > > Hm, not sure I see use for that.
> > >
> > > Staring at madvise_walk_vmas(), we return ENOMEM on VMA holes, but would
> > > process PROT_NONE. So current behavior is at least consistent with PROT_NONE
> > > handling (where something could be mapped, though).
> >
> > Err, the handling of holes is terrible, yes we return ENOMEM, but we _carry out
> > the whole procedure_ then return an error, an error _indistinguishable from an
> > error arising from any of the individual parts_.
> >
> > Which is just, awful.
>
> Yes, absolutely. I don't know why we decided to continue. And why we return
> ENOMEM ...
Anyway UAPI so no turning back now :)
>
> >
> > >
> > > No strong opinion.
> >
> > Well you used up your strong opinion on the naming ;)
>
> He, and I am out of energy for this year ;)
Haha understandable...
>
> In retrospective, "install or remove a guard PTE" is just much better than
> anything else ...
>
> So I should never have been mislead to suggest poison/unpoison as a
> replacement for poison/remedy :P
You know the reason ;)
>
> >
> > >
> > > >
> > > > Also the existing logic is that existing markers (HW poison, uffd-simulated HW
> > > > poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
> > > > no error on MADV_FREE either, so it'd be consistent with existing behaviour.
> > >
> > >
> > > HW poison / uffd-simulated HW poison are expected to be zapped: it's just
> > > like a mapped page with HWPOISON. So that is correct.
> >
> > Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
>
> Huh?
>
> madvise_dontneed_single_vma()->zap_page_range_single(details=NULL)->unmap_single_vma(details=NULL)
> ... zap_pte_range()
>
> } else if (is_hwpoison_entry(entry) ||
> is_poisoned_swp_entry(entry)) {
> if (!should_zap_cows(details))
> continue;
> ...
>
> Should just zap them.
>
> What am I missing?
Yeah ok it's me who's missing something here, I hadn't noticed details == NULL
so should_zap_cows() is true, my mistake!
In any case we explicitly add code here to prevent guard pages from going. I
will correct everything where I wrongly say otherwise, doh!
>
> > mean the MADV flags are a confusing mess generally, as per Vlasta's comments
> > which to begin with I strongly disagreed with then, discussing further, realsed
> > that no this is just a bit insane and had driven _me_ insane.
> >
> > >
> > > UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap uffd-wp
> > > entries.
> > >
> > > >
> > > > Also semantically you are achieving what the calls expect you are freeing the
> > > > ranges since the guard page regions are unbacked so are already freed... so yeah
> > > > I don't think an error really makes sense here.
> > >
> > > I you compare it to a VMA hole, it make sense to fail. If we treat it like
> > > PROT_NONE, it make sense to skip them.
> > >
> > > >
> > > > We might also be limiting use cases by assuming they might _only_ be used for
> > > > allocators and such.
> > >
> > > I don't buy that as an argument, sorry :)
> > >
> > > "Let's map the kernel writable into all user space because otherwise we
> > > might be limiting use cases"
> >
> > That's a great idea! Patch series incoming, 1st April 2025... :>)
>
> :) Just flip the bit on x86 and we're done!
;)
>
> > >
> > >
> > > :P
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb
> > >
> >
> > Overall I think just always leaving in place except on remedy err sorry sorry
> > unpoison and munmap and not returning an error if encountered elsewhere (other
> > than, of course, GUP) is the right way forward and most in line with user
> > expectation and practical usage.
>
>
> Fine with me, make sure to document that is behaves like a PROT_NONE VMA,
> not like a memory hole, except when something would trigger a fault (GUP
> etc).
Ack will make sure to document.
>
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 17:05 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <fce49bbbfe41b82161a37b022c8eb1e6c20e1d85.1729440856.git.lorenzo.stoakes@oracle.com>
On 20.10.24 18:20, Lorenzo Stoakes wrote:
> Implement a new lightweight guard page feature, that is regions of userland
> virtual memory that, when accessed, cause a fatal signal to arise.
>
> Currently users must establish PROT_NONE ranges to achieve this.
>
> However this is very costly memory-wise - we need a VMA for each and every
> one of these regions AND they become unmergeable with surrounding VMAs.
>
> In addition repeated mmap() calls require repeated kernel context switches
> and contention of the mmap lock to install these ranges, potentially also
> having to unmap memory if installed over existing ranges.
>
> The lightweight guard approach eliminates the VMA cost altogether - rather
> than establishing a PROT_NONE VMA, it operates at the level of page table
> entries - poisoning PTEs such that accesses to them cause a fault followed
> by a SIGSGEV signal being raised.
>
> This is achieved through the PTE marker mechanism, which a previous commit
> in this series extended to permit this to be done, installed via the
> generic page walking logic, also extended by a prior commit for this
> purpose.
>
> These poison ranges are established with MADV_GUARD_POISON, and if the
> range in which they are installed contain any existing mappings, they will
> be zapped, i.e. free the range and unmap memory (thus mimicking the
> behaviour of MADV_DONTNEED in this respect).
>
> Any existing poison entries will be left untouched. There is no nesting of
> poisoned pages.
>
> Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
> unexpected behaviour, but are cleared on process teardown or unmapping of
> memory ranges.
>
> Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
> 'remedying' the poisoning. The ranges over which this is applied, should
> they contain non-poison entries, will be untouched, only poison entries
> will be cleared.
>
> We permit this operation on anonymous memory only, and only VMAs which are
> non-special, non-huge and not mlock()'d (if we permitted this we'd have to
> drop locked pages which would be rather counterintuitive).
>
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Suggested-by: Jann Horn <jannh@google.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> 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/uapi/asm-generic/mman-common.h | 3 +
> mm/madvise.c | 168 +++++++++++++++++++++++++
> mm/mprotect.c | 3 +-
> mm/mseal.c | 1 +
> 8 files changed, 186 insertions(+), 1 deletion(-)
>
> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> index 763929e814e9..71e13f27742d 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -78,6 +78,9 @@
>
> #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
>
> +#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
> +#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
Just to raise it here: MADV_GUARD_INSTALL / MADV_GUARD_REMOVE or sth.
like that would have been even clearer, at least to me.
But no strong opinion, just if somebody else reading along was wondering
about the same.
I'm hoping to find more time to have a closer look at this this week,
but in general, the concept sounds reasonable to me.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: David Hildenbrand @ 2024-10-21 17:00 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cbf17dc3-01eb-4416-8ec5-cac05e50d663@lucifer.local>
On 21.10.24 18:51, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 06:44:04PM +0200, David Hildenbrand wrote:
>> On 21.10.24 18:23, Lorenzo Stoakes wrote:
>>> On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
>>> [snip]
>>>>>
>>>>> To summarise for on-list:
>>>>>
>>>>> * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
>>>>> ability to be 'cancelled' if you write to the memory. Also, after the
>>>>> freeing is complete, you can write to the memory to reuse it, the mapping
>>>>> is still there.
>>>>>
>>>>> * For hardware poison markers it makes sense to drop them as you're
>>>>> effectively saying 'I am done with this range that is now unbacked and
>>>>> expect to get an empty page should I use it now'. UFFD WP I am not sure
>>>>> about but presumably also fine.
>>>>>
>>>>> * However, guard pages are different - if you 'cancel' and you are left
>>>>> with a block of memory allocated to you by a pthread or userland
>>>>> allocator implementation, you don't want to then no longer be protected
>>>>> from overrunning into other thread memory.
>>>>
>>>> Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
>>>> error? It sounds like a usage "error" to me (in contrast to munmap()).
>>>
>>> It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
>>> place, from v3 we will do the same for MADV_FREE.
>>>
>>> I'm not sure I'd say it's an error per se, as somebody might have a use case
>>> where they want to zap over a range but keep guard pages, perhaps an allocator
>>> or something?
>>
>> Hm, not sure I see use for that.
>>
>> Staring at madvise_walk_vmas(), we return ENOMEM on VMA holes, but would
>> process PROT_NONE. So current behavior is at least consistent with PROT_NONE
>> handling (where something could be mapped, though).
>
> Err, the handling of holes is terrible, yes we return ENOMEM, but we _carry out
> the whole procedure_ then return an error, an error _indistinguishable from an
> error arising from any of the individual parts_.
>
> Which is just, awful.
Yes, absolutely. I don't know why we decided to continue. And why we
return ENOMEM ...
>
>>
>> No strong opinion.
>
> Well you used up your strong opinion on the naming ;)
He, and I am out of energy for this year ;)
In retrospective, "install or remove a guard PTE" is just much better
than anything else ...
So I should never have been mislead to suggest poison/unpoison as a
replacement for poison/remedy :P
>
>>
>>>
>>> Also the existing logic is that existing markers (HW poison, uffd-simulated HW
>>> poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
>>> no error on MADV_FREE either, so it'd be consistent with existing behaviour.
>>
>>
>> HW poison / uffd-simulated HW poison are expected to be zapped: it's just
>> like a mapped page with HWPOISON. So that is correct.
>
> Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
Huh?
madvise_dontneed_single_vma()->zap_page_range_single(details=NULL)->unmap_single_vma(details=NULL)
... zap_pte_range()
} else if (is_hwpoison_entry(entry) ||
is_poisoned_swp_entry(entry)) {
if (!should_zap_cows(details))
continue;
...
Should just zap them.
What am I missing?
> mean the MADV flags are a confusing mess generally, as per Vlasta's comments
> which to begin with I strongly disagreed with then, discussing further, realsed
> that no this is just a bit insane and had driven _me_ insane.
>
>>
>> UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap uffd-wp
>> entries.
>>
>>>
>>> Also semantically you are achieving what the calls expect you are freeing the
>>> ranges since the guard page regions are unbacked so are already freed... so yeah
>>> I don't think an error really makes sense here.
>>
>> I you compare it to a VMA hole, it make sense to fail. If we treat it like
>> PROT_NONE, it make sense to skip them.
>>
>>>
>>> We might also be limiting use cases by assuming they might _only_ be used for
>>> allocators and such.
>>
>> I don't buy that as an argument, sorry :)
>>
>> "Let's map the kernel writable into all user space because otherwise we
>> might be limiting use cases"
>
> That's a great idea! Patch series incoming, 1st April 2025... :>)
:) Just flip the bit on x86 and we're done!
>>
>>
>> :P
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
>
> Overall I think just always leaving in place except on remedy err sorry sorry
> unpoison and munmap and not returning an error if encountered elsewhere (other
> than, of course, GUP) is the right way forward and most in line with user
> expectation and practical usage.
Fine with me, make sure to document that is behaves like a PROT_NONE
VMA, not like a memory hole, except when something would trigger a fault
(GUP etc).
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 16:51 UTC (permalink / raw)
To: David Hildenbrand
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <49afa956-21e1-4b3d-9dde-82a6891f2902@redhat.com>
On Mon, Oct 21, 2024 at 06:44:04PM +0200, David Hildenbrand wrote:
> On 21.10.24 18:23, Lorenzo Stoakes wrote:
> > On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
> > [snip]
> > > >
> > > > To summarise for on-list:
> > > >
> > > > * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
> > > > ability to be 'cancelled' if you write to the memory. Also, after the
> > > > freeing is complete, you can write to the memory to reuse it, the mapping
> > > > is still there.
> > > >
> > > > * For hardware poison markers it makes sense to drop them as you're
> > > > effectively saying 'I am done with this range that is now unbacked and
> > > > expect to get an empty page should I use it now'. UFFD WP I am not sure
> > > > about but presumably also fine.
> > > >
> > > > * However, guard pages are different - if you 'cancel' and you are left
> > > > with a block of memory allocated to you by a pthread or userland
> > > > allocator implementation, you don't want to then no longer be protected
> > > > from overrunning into other thread memory.
> > >
> > > Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
> > > error? It sounds like a usage "error" to me (in contrast to munmap()).
> >
> > It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
> > place, from v3 we will do the same for MADV_FREE.
> >
> > I'm not sure I'd say it's an error per se, as somebody might have a use case
> > where they want to zap over a range but keep guard pages, perhaps an allocator
> > or something?
>
> Hm, not sure I see use for that.
>
> Staring at madvise_walk_vmas(), we return ENOMEM on VMA holes, but would
> process PROT_NONE. So current behavior is at least consistent with PROT_NONE
> handling (where something could be mapped, though).
Err, the handling of holes is terrible, yes we return ENOMEM, but we _carry out
the whole procedure_ then return an error, an error _indistinguishable from an
error arising from any of the individual parts_.
Which is just, awful.
>
> No strong opinion.
Well you used up your strong opinion on the naming ;)
>
> >
> > Also the existing logic is that existing markers (HW poison, uffd-simulated HW
> > poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
> > no error on MADV_FREE either, so it'd be consistent with existing behaviour.
>
>
> HW poison / uffd-simulated HW poison are expected to be zapped: it's just
> like a mapped page with HWPOISON. So that is correct.
Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
mean the MADV flags are a confusing mess generally, as per Vlasta's comments
which to begin with I strongly disagreed with then, discussing further, realsed
that no this is just a bit insane and had driven _me_ insane.
>
> UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap uffd-wp
> entries.
>
> >
> > Also semantically you are achieving what the calls expect you are freeing the
> > ranges since the guard page regions are unbacked so are already freed... so yeah
> > I don't think an error really makes sense here.
>
> I you compare it to a VMA hole, it make sense to fail. If we treat it like
> PROT_NONE, it make sense to skip them.
>
> >
> > We might also be limiting use cases by assuming they might _only_ be used for
> > allocators and such.
>
> I don't buy that as an argument, sorry :)
>
> "Let's map the kernel writable into all user space because otherwise we
> might be limiting use cases"
That's a great idea! Patch series incoming, 1st April 2025... :>)
>
>
> :P
>
> --
> Cheers,
>
> David / dhildenb
>
Overall I think just always leaving in place except on remedy err sorry sorry
unpoison and munmap and not returning an error if encountered elsewhere (other
than, of course, GUP) is the right way forward and most in line with user
expectation and practical usage.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: David Hildenbrand @ 2024-10-21 16:44 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <ea771edf-0e38-440f-b264-3cbe285a628b@lucifer.local>
On 21.10.24 18:23, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
> [snip]
>>>
>>> To summarise for on-list:
>>>
>>> * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
>>> ability to be 'cancelled' if you write to the memory. Also, after the
>>> freeing is complete, you can write to the memory to reuse it, the mapping
>>> is still there.
>>>
>>> * For hardware poison markers it makes sense to drop them as you're
>>> effectively saying 'I am done with this range that is now unbacked and
>>> expect to get an empty page should I use it now'. UFFD WP I am not sure
>>> about but presumably also fine.
>>>
>>> * However, guard pages are different - if you 'cancel' and you are left
>>> with a block of memory allocated to you by a pthread or userland
>>> allocator implementation, you don't want to then no longer be protected
>>> from overrunning into other thread memory.
>>
>> Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
>> error? It sounds like a usage "error" to me (in contrast to munmap()).
>
> It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
> place, from v3 we will do the same for MADV_FREE.
>
> I'm not sure I'd say it's an error per se, as somebody might have a use case
> where they want to zap over a range but keep guard pages, perhaps an allocator
> or something?
Hm, not sure I see use for that.
Staring at madvise_walk_vmas(), we return ENOMEM on VMA holes, but would
process PROT_NONE. So current behavior is at least consistent with
PROT_NONE handling (where something could be mapped, though).
No strong opinion.
>
> Also the existing logic is that existing markers (HW poison, uffd-simulated HW
> poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
> no error on MADV_FREE either, so it'd be consistent with existing behaviour.
HW poison / uffd-simulated HW poison are expected to be zapped: it's
just like a mapped page with HWPOISON. So that is correct.
UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap
uffd-wp entries.
>
> Also semantically you are achieving what the calls expect you are freeing the
> ranges since the guard page regions are unbacked so are already freed... so yeah
> I don't think an error really makes sense here.
I you compare it to a VMA hole, it make sense to fail. If we treat it
like PROT_NONE, it make sense to skip them.
>
> We might also be limiting use cases by assuming they might _only_ be used for
> allocators and such.
I don't buy that as an argument, sorry :)
"Let's map the kernel writable into all user space because otherwise we
might be limiting use cases"
:P
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 16:23 UTC (permalink / raw)
To: David Hildenbrand
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cb0e49be-7b4e-4760-884c-8f4bf74ec1e1@redhat.com>
On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
[snip]
> >
> > To summarise for on-list:
> >
> > * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
> > ability to be 'cancelled' if you write to the memory. Also, after the
> > freeing is complete, you can write to the memory to reuse it, the mapping
> > is still there.
> >
> > * For hardware poison markers it makes sense to drop them as you're
> > effectively saying 'I am done with this range that is now unbacked and
> > expect to get an empty page should I use it now'. UFFD WP I am not sure
> > about but presumably also fine.
> >
> > * However, guard pages are different - if you 'cancel' and you are left
> > with a block of memory allocated to you by a pthread or userland
> > allocator implementation, you don't want to then no longer be protected
> > from overrunning into other thread memory.
>
> Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
> error? It sounds like a usage "error" to me (in contrast to munmap()).
It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
place, from v3 we will do the same for MADV_FREE.
I'm not sure I'd say it's an error per se, as somebody might have a use case
where they want to zap over a range but keep guard pages, perhaps an allocator
or something?
Also the existing logic is that existing markers (HW poison, uffd-simulated HW
poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
no error on MADV_FREE either, so it'd be consistent with existing behaviour.
Also semantically you are achieving what the calls expect you are freeing the
ranges since the guard page regions are unbacked so are already freed... so yeah
I don't think an error really makes sense here.
We might also be limiting use cases by assuming they might _only_ be used for
allocators and such.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: David Hildenbrand @ 2024-10-21 16:00 UTC (permalink / raw)
To: Lorenzo Stoakes, Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <4f4e41f1-531c-4686-b44d-dacdf034c241@lucifer.local>
On 21.10.24 17:33, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 04:54:06PM +0200, Vlastimil Babka wrote:
>> On 10/21/24 16:33, Lorenzo Stoakes wrote:
>>> On Mon, Oct 21, 2024 at 04:13:34PM +0200, Vlastimil Babka wrote:
>>>> On 10/20/24 18:20, Lorenzo Stoakes wrote:
>>>>> Add a new PTE marker that results in any access causing the accessing
>>>>> process to segfault.
>>>>>
>>>>> This is preferable to PTE_MARKER_POISONED, which results in the same
>>>>> handling as hardware poisoned memory, and is thus undesirable for cases
>>>>> where we simply wish to 'soft' poison a range.
>>>>>
>>>>> This is in preparation for implementing the ability to specify guard pages
>>>>> at the page table level, i.e. ranges that, when accessed, should cause
>>>>> process termination.
>>>>>
>>>>> Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
>>>>> function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
>>>>> purpose was simply incorrect.
>>>>>
>>>>> We then reuse the same logic to determine whether a zap should clear a
>>>>> guard entry - this should only be performed on teardown and never on
>>>>> MADV_DONTNEED or the like.
>>>>
>>>> Since I would have personally put MADV_FREE among "or the like" here, it's
>>>> surprising to me that it in fact it's tearing down the guard entries now. Is
>>>> that intentional? It should be at least mentioned very explicitly. But I'd
>>>> really argue against it, as MADV_FREE is to me a weaker form of
>>>> MADV_DONTNEED - the existing pages are not zapped immediately but
>>>> prioritized for reclaim. If MADV_DONTNEED leaves guard PTEs in place, why
>>>> shouldn't MADV_FREE too?
>>>
>>> That is not, as I understand it, what MADV_FREE is, semantically. From the
>>> man pages:
>>>
>>> MADV_FREE (since Linux 4.5)
>>>
>>> The application no longer requires the pages in the range
>>> specified by addr and len. The kernel can thus free these
>>> pages, but the freeing could be delayed until memory pressure
>>> occurs.
>>>
>>> MADV_DONTNEED
>>>
>>> Do not expect access in the near future. (For the time
>>> being, the application is finished with the given range, so
>>> the kernel can free resources associated with it.)
>>>
>>> MADV_FREE is 'we are completely done with this range'. MADV_DONTNEED is 'we
>>> don't expect to use it in the near future'.
>>
>> I think the description gives a wrong impression. What I think matters it
>> what happens (limited to anon private case as MADV_FREE doesn't support any
>> other)
>>
>> MADV_DONTNEED - pages discarded immediately, further access gives new
>> zero-filled pages
>>
>> MADV_FREE - pages prioritized for discarding, if that happens before next
>> write, it gets zero-filled page on next access, but a write done soon enough
>> can cancel the upcoming discard.
>>
>> In that sense, MADV_FREE is a weaker form of DONTNEED, no?
>>
>>>>
>>>> Seems to me rather currently an artifact of MADV_FREE implementation - if it
>>>> encounters hwpoison entries it will tear them down because why not, we have
>>>> detected a hw memory error and are lucky the program wants to discard the
>>>> pages and not access them, so best use the opportunity and get rid of the
>>>> PTE entries immediately (if MADV_DONTNEED doesn't do that too, it certainly
>>>> could).
>>>
>>> Right, but we explicitly do not tear them down in the case of MADV_DONTNEED
>>> which matches the description in the manpages that the user _might_ come
>>> back to the range, whereas MADV_FREE means they are truly done but just
>>> don't want the overhead of actually unmapping at this point.
>>
>> But it's also defined what happens if user comes back to the range after a
>> MADV_FREE. I think the overhead saved happens in the case of actually coming
>> back soon enough to prevent the discard. With MADV_DONTNEED its immediate
>> and unconditional.
>>
>>> Seems to be this is moreso that MADV_FREE is a not-really-as-efficient
>>> version of what Rik wants to do with his MADV_LAZYFREE thing.
>>
>> I think that further optimizes MADV_FREE, which is already more optimized
>> than MADV_DONTNEED.
>>
>>>>
>>>> But to extend this to guard PTEs which are result of an explicit userspace
>>>> action feels wrong, unless the semantics is the same for MADV_DONTEED. The
>>>> semantics chosen for MADV_DONTNEED makes sense, so MADV_FREE should behave
>>>> the same?
>>>
>>> My understanding from the above is that MADV_FREE is a softer version of
>>> munmap(), i.e. 'totally done with this range', whereas MADV_DONTNEED is a
>>> 'revert state to when I first mapped this stuff because I'm done with it
>>> for now but might use it later'.
>>
>> From the implementation I get the opposite understanding. Neither tears down
>> the vma like a proper unmap(). MADV_DONTNEED zaps page tables immediately,
>> MADV_FREE effectively too but with a delay depending on memory pressure.
>>
>
> OK so based on IRC chat I think the conclusion here is TL;DR yes we have to
> change this, you're right :)
>
> To summarise for on-list:
>
> * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
> ability to be 'cancelled' if you write to the memory. Also, after the
> freeing is complete, you can write to the memory to reuse it, the mapping
> is still there.
>
> * For hardware poison markers it makes sense to drop them as you're
> effectively saying 'I am done with this range that is now unbacked and
> expect to get an empty page should I use it now'. UFFD WP I am not sure
> about but presumably also fine.
>
> * However, guard pages are different - if you 'cancel' and you are left
> with a block of memory allocated to you by a pthread or userland
> allocator implementation, you don't want to then no longer be protected
> from overrunning into other thread memory.
Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored
or error? It sounds like a usage "error" to me (in contrast to munmap()).
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 15:41 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <4f4e41f1-531c-4686-b44d-dacdf034c241@lucifer.local>
On Mon, Oct 21, 2024 at 04:33:19PM +0100, Lorenzo Stoakes wrote:
[snip]
> * For hardware poison markers it makes sense to drop them as you're
> effectively saying 'I am done with this range that is now unbacked and
> expect to get an empty page should I use it now'. UFFD WP I am not sure
> about but presumably also fine.
[snip]
CORRECTION: (sorry multitasking atm now I have a non-melted CPU) UFFD WP is
safe from MADV_FREE as it checks is_poisoned_swp_entry() which
this patch updates to include guard pages, a change I will undo
in v3. So disregard comment on UFFD WP here.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 15:33 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <caf95a99-e975-4f3d-a94b-298a5fc88b5a@suse.cz>
On Mon, Oct 21, 2024 at 04:54:06PM +0200, Vlastimil Babka wrote:
> On 10/21/24 16:33, Lorenzo Stoakes wrote:
> > On Mon, Oct 21, 2024 at 04:13:34PM +0200, Vlastimil Babka wrote:
> >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> >> > Add a new PTE marker that results in any access causing the accessing
> >> > process to segfault.
> >> >
> >> > This is preferable to PTE_MARKER_POISONED, which results in the same
> >> > handling as hardware poisoned memory, and is thus undesirable for cases
> >> > where we simply wish to 'soft' poison a range.
> >> >
> >> > This is in preparation for implementing the ability to specify guard pages
> >> > at the page table level, i.e. ranges that, when accessed, should cause
> >> > process termination.
> >> >
> >> > Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
> >> > function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
> >> > purpose was simply incorrect.
> >> >
> >> > We then reuse the same logic to determine whether a zap should clear a
> >> > guard entry - this should only be performed on teardown and never on
> >> > MADV_DONTNEED or the like.
> >>
> >> Since I would have personally put MADV_FREE among "or the like" here, it's
> >> surprising to me that it in fact it's tearing down the guard entries now. Is
> >> that intentional? It should be at least mentioned very explicitly. But I'd
> >> really argue against it, as MADV_FREE is to me a weaker form of
> >> MADV_DONTNEED - the existing pages are not zapped immediately but
> >> prioritized for reclaim. If MADV_DONTNEED leaves guard PTEs in place, why
> >> shouldn't MADV_FREE too?
> >
> > That is not, as I understand it, what MADV_FREE is, semantically. From the
> > man pages:
> >
> > MADV_FREE (since Linux 4.5)
> >
> > The application no longer requires the pages in the range
> > specified by addr and len. The kernel can thus free these
> > pages, but the freeing could be delayed until memory pressure
> > occurs.
> >
> > MADV_DONTNEED
> >
> > Do not expect access in the near future. (For the time
> > being, the application is finished with the given range, so
> > the kernel can free resources associated with it.)
> >
> > MADV_FREE is 'we are completely done with this range'. MADV_DONTNEED is 'we
> > don't expect to use it in the near future'.
>
> I think the description gives a wrong impression. What I think matters it
> what happens (limited to anon private case as MADV_FREE doesn't support any
> other)
>
> MADV_DONTNEED - pages discarded immediately, further access gives new
> zero-filled pages
>
> MADV_FREE - pages prioritized for discarding, if that happens before next
> write, it gets zero-filled page on next access, but a write done soon enough
> can cancel the upcoming discard.
>
> In that sense, MADV_FREE is a weaker form of DONTNEED, no?
>
> >>
> >> Seems to me rather currently an artifact of MADV_FREE implementation - if it
> >> encounters hwpoison entries it will tear them down because why not, we have
> >> detected a hw memory error and are lucky the program wants to discard the
> >> pages and not access them, so best use the opportunity and get rid of the
> >> PTE entries immediately (if MADV_DONTNEED doesn't do that too, it certainly
> >> could).
> >
> > Right, but we explicitly do not tear them down in the case of MADV_DONTNEED
> > which matches the description in the manpages that the user _might_ come
> > back to the range, whereas MADV_FREE means they are truly done but just
> > don't want the overhead of actually unmapping at this point.
>
> But it's also defined what happens if user comes back to the range after a
> MADV_FREE. I think the overhead saved happens in the case of actually coming
> back soon enough to prevent the discard. With MADV_DONTNEED its immediate
> and unconditional.
>
> > Seems to be this is moreso that MADV_FREE is a not-really-as-efficient
> > version of what Rik wants to do with his MADV_LAZYFREE thing.
>
> I think that further optimizes MADV_FREE, which is already more optimized
> than MADV_DONTNEED.
>
> >>
> >> But to extend this to guard PTEs which are result of an explicit userspace
> >> action feels wrong, unless the semantics is the same for MADV_DONTEED. The
> >> semantics chosen for MADV_DONTNEED makes sense, so MADV_FREE should behave
> >> the same?
> >
> > My understanding from the above is that MADV_FREE is a softer version of
> > munmap(), i.e. 'totally done with this range', whereas MADV_DONTNEED is a
> > 'revert state to when I first mapped this stuff because I'm done with it
> > for now but might use it later'.
>
> From the implementation I get the opposite understanding. Neither tears down
> the vma like a proper unmap(). MADV_DONTNEED zaps page tables immediately,
> MADV_FREE effectively too but with a delay depending on memory pressure.
>
OK so based on IRC chat I think the conclusion here is TL;DR yes we have to
change this, you're right :)
To summarise for on-list:
* MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
ability to be 'cancelled' if you write to the memory. Also, after the
freeing is complete, you can write to the memory to reuse it, the mapping
is still there.
* For hardware poison markers it makes sense to drop them as you're
effectively saying 'I am done with this range that is now unbacked and
expect to get an empty page should I use it now'. UFFD WP I am not sure
about but presumably also fine.
* However, guard pages are different - if you 'cancel' and you are left
with a block of memory allocated to you by a pthread or userland
allocator implementation, you don't want to then no longer be protected
from overrunning into other thread memory.
So, while I implemented this based on a. the semantics as apparently
expressed in the man page and b. existing PTE marker behaviour, it seems
that it would actually be problematic to do so.
So yeah, I'll change that in v3!
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Vlastimil Babka @ 2024-10-21 14:54 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <434a440a-d6a4-4144-b4fb-8e0d8535f03f@lucifer.local>
On 10/21/24 16:33, Lorenzo Stoakes wrote:
> On Mon, Oct 21, 2024 at 04:13:34PM +0200, Vlastimil Babka wrote:
>> On 10/20/24 18:20, Lorenzo Stoakes wrote:
>> > Add a new PTE marker that results in any access causing the accessing
>> > process to segfault.
>> >
>> > This is preferable to PTE_MARKER_POISONED, which results in the same
>> > handling as hardware poisoned memory, and is thus undesirable for cases
>> > where we simply wish to 'soft' poison a range.
>> >
>> > This is in preparation for implementing the ability to specify guard pages
>> > at the page table level, i.e. ranges that, when accessed, should cause
>> > process termination.
>> >
>> > Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
>> > function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
>> > purpose was simply incorrect.
>> >
>> > We then reuse the same logic to determine whether a zap should clear a
>> > guard entry - this should only be performed on teardown and never on
>> > MADV_DONTNEED or the like.
>>
>> Since I would have personally put MADV_FREE among "or the like" here, it's
>> surprising to me that it in fact it's tearing down the guard entries now. Is
>> that intentional? It should be at least mentioned very explicitly. But I'd
>> really argue against it, as MADV_FREE is to me a weaker form of
>> MADV_DONTNEED - the existing pages are not zapped immediately but
>> prioritized for reclaim. If MADV_DONTNEED leaves guard PTEs in place, why
>> shouldn't MADV_FREE too?
>
> That is not, as I understand it, what MADV_FREE is, semantically. From the
> man pages:
>
> MADV_FREE (since Linux 4.5)
>
> The application no longer requires the pages in the range
> specified by addr and len. The kernel can thus free these
> pages, but the freeing could be delayed until memory pressure
> occurs.
>
> MADV_DONTNEED
>
> Do not expect access in the near future. (For the time
> being, the application is finished with the given range, so
> the kernel can free resources associated with it.)
>
> MADV_FREE is 'we are completely done with this range'. MADV_DONTNEED is 'we
> don't expect to use it in the near future'.
I think the description gives a wrong impression. What I think matters it
what happens (limited to anon private case as MADV_FREE doesn't support any
other)
MADV_DONTNEED - pages discarded immediately, further access gives new
zero-filled pages
MADV_FREE - pages prioritized for discarding, if that happens before next
write, it gets zero-filled page on next access, but a write done soon enough
can cancel the upcoming discard.
In that sense, MADV_FREE is a weaker form of DONTNEED, no?
>>
>> Seems to me rather currently an artifact of MADV_FREE implementation - if it
>> encounters hwpoison entries it will tear them down because why not, we have
>> detected a hw memory error and are lucky the program wants to discard the
>> pages and not access them, so best use the opportunity and get rid of the
>> PTE entries immediately (if MADV_DONTNEED doesn't do that too, it certainly
>> could).
>
> Right, but we explicitly do not tear them down in the case of MADV_DONTNEED
> which matches the description in the manpages that the user _might_ come
> back to the range, whereas MADV_FREE means they are truly done but just
> don't want the overhead of actually unmapping at this point.
But it's also defined what happens if user comes back to the range after a
MADV_FREE. I think the overhead saved happens in the case of actually coming
back soon enough to prevent the discard. With MADV_DONTNEED its immediate
and unconditional.
> Seems to be this is moreso that MADV_FREE is a not-really-as-efficient
> version of what Rik wants to do with his MADV_LAZYFREE thing.
I think that further optimizes MADV_FREE, which is already more optimized
than MADV_DONTNEED.
>>
>> But to extend this to guard PTEs which are result of an explicit userspace
>> action feels wrong, unless the semantics is the same for MADV_DONTEED. The
>> semantics chosen for MADV_DONTNEED makes sense, so MADV_FREE should behave
>> the same?
>
> My understanding from the above is that MADV_FREE is a softer version of
> munmap(), i.e. 'totally done with this range', whereas MADV_DONTNEED is a
> 'revert state to when I first mapped this stuff because I'm done with it
> for now but might use it later'.
From the implementation I get the opposite understanding. Neither tears down
the vma like a proper unmap(). MADV_DONTNEED zaps page tables immediately,
MADV_FREE effectively too but with a delay depending on memory pressure.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox