* 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: 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: 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: 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 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: Vlastimil Babka @ 2024-10-21 20:45 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: <393b0932-1c52-4d59-9466-e5e6184a7daf@lucifer.local>
On 10/21/24 22:27, Lorenzo Stoakes wrote:
> 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...
Whoops, you're right, I did read v1 but forgot about the RFC.
But we can assume people who'll only see the code after it's merged will not
have read it either, so since a potentially endless loop could be
suspicious, expanding the comment to explain how it's fine wouldn't hurt?
> 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.
OK.
>>
>> > + 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.
If Jann is fine with this then it must be secure enough.
>>
>> 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 :)
Job security :)
j/k
> 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: Lorenzo Stoakes @ 2024-10-21 20:49 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: <9727ada4-0048-499b-a43f-ac0a625bae5d@redhat.com>
On Mon, Oct 21, 2024 at 10:37:39PM +0200, David Hildenbrand wrote:
> 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.
The looping mechanism is fine for dealing with concurrent faults. There's
no actual _race_ due to PTL, it's just that a user could repeatedly
populate stuff stupidly in a range that is meant to have poison markers put
in.
It's not likely and would be kind of an abusive of the interface, and it'd
really be a process just hurting itself.
In nearly all cases you won't zap at all. The whole point is it's
optimistic. In 99.99% of others you zap once...
>
> Have to do some more reading on this.
May I suggest a book on the history of the prodigy?
>
> >
> > 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.
Yeah I mean originally I had a -EAGAIN which was sort of equivalent of this
but Jann pointed out you're just shifting work to userland who would loop
and repeat.
I just don't see why we'd do this.
In fact I was looking at the series and thinking 'wow it's actually a
really small delta' and being proud but... still not KIS enough apparently
;)
>
> 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.
I don't think there's really any value in that. There's just no sensible
situation in which a user would care about this I don't think.
And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
then why not just do that in the kernel?
Trying to explain to a user 'hey this is for installing guard pages but if
there's a facing fault it'll fail and that could keep happening and then
you'll have to zap and maybe in a loop' just... seems like a bloody awful
interface?
I prefer 'here's an interface for installing and removing guard pages,
enjoy!' :)
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-21 21:13 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, Dave Hansen
In-Reply-To: <69d1e02b-aa15-4712-90f1-6166b551b992@lucifer.local>
+cc Dave Hansen
On Mon, Oct 21, 2024 at 09:42:53PM +0100, Lorenzo Stoakes wrote:
> 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!
To put it on list - Dave Hansen commented on IRC that it would be safer to
avoid this for now due to this being an ABI change, and reasonable to
perhaps add it later if required, so that seems a sensible way forward.
Thanks!
[snip]
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-21 21:20 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: <73134e10-19eb-4e52-b87f-5fbfd322b575@lucifer.local>
>> 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.
>
> The looping mechanism is fine for dealing with concurrent faults. There's
> no actual _race_ due to PTL, it's just that a user could repeatedly
> populate stuff stupidly in a range that is meant to have poison markers put
> in.
>
> It's not likely and would be kind of an abusive of the interface, and it'd
> really be a process just hurting itself.
>
> In nearly all cases you won't zap at all. The whole point is it's
> optimistic. In 99.99% of others you zap once...
Exactly! And that's why I am questioning whether the kernel should care
about that. See below.
>
>>
>> Have to do some more reading on this.
>
> May I suggest a book on the history of the prodigy?
:D
>
>>
>>>
>>> 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.
>
> Yeah I mean originally I had a -EAGAIN which was sort of equivalent of this
> but Jann pointed out you're just shifting work to userland who would loop
> and repeat.
>
> I just don't see why we'd do this.
>
> In fact I was looking at the series and thinking 'wow it's actually a
> really small delta' and being proud but... still not KIS enough apparently
> ;)
You know, I read a lot of kernel code ... and mfill_atomic_install_pte()
is what popped in my head: if there is already something, let user space
handle it, because it is unexpected.
The uffd interface is slightly better, as it gives you the number of
processed PTEs back, which madvise() is not designed for.
But maybe this (returning how many we already processed) is not required
due to the nature of guard pages (below).
>
>>
>> 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.
>
> I don't think there's really any value in that. There's just no sensible
> situation in which a user would care about this I don't think.
Making sure nobody touches an area, and wile doing that somebody already
touched that area? I guess it could be worked around by
mprotect(PROT_NONE),madvise(GUARD),mprotect(PROT_READ|PROT_WRITE) ...
which is not particularly nice :)
>
> And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
> then why not just do that in the kernel?
Heh, no!
If user space doesn't expect there to be something, it should *fail*.
That's likely going to be the majority of use cases for guard pages
(happy to be told otherwise). No retry.
And if user space expects there to be something it should zap ahead of
time (which some allocators maybe already do to free up memory after
free()) to then install the guard. No retry.
There is this case where user space might be unsure. There, it might
make sense to retry exactly once.
>
> Trying to explain to a user 'hey this is for installing guard pages but if
> there's a facing fault it'll fail and that could keep happening and then
> you'll have to zap and maybe in a loop' just... seems like a bloody awful
> interface?
Hope my example above made it clearer. This "retry forever until it
works" use case doesn't quite make sense to me, but I might just be
missing something important.
But again, I have to do more reading on the history of the current
approach ... and it's fairly late here.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Dave Hansen @ 2024-10-21 21:20 UTC (permalink / raw)
To: Lorenzo Stoakes, 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, Dave Hansen
In-Reply-To: <5ee11594-c40a-4ef1-a6c2-f1e38da0e9a3@lucifer.local>
On 10/21/24 14:13, Lorenzo Stoakes wrote:
>> 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!
> To put it on list - Dave Hansen commented on IRC that it would be safer to
> avoid this for now due to this being an ABI change, and reasonable to
> perhaps add it later if required, so that seems a sensible way forward.
We added SEGV_PKUERR because we really did expect signal handlers to
want to do something new and special, including consuming si_pkey. Old
signal handlers would probably be pretty confused.
So, yeah, if it's not crystal clear that new signal handler code is
needed, than I'd punt on adding a new SEGV_* code for now.
BTW, SEGV_* codes are sequentially assigned. It isn't a bitfield and
there are no space constraints. We've only used a dozen or so of them
and ->si_code is an int.
^ permalink raw reply
* Re: [PATCH v2 5/5] selftests/mm: add self tests for guard page feature
From: Shuah Khan @ 2024-10-21 21:31 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, 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, Shuah Khan
In-Reply-To: <090ed13db09bc25863a78eba902d8bf2c6534ced.1729440856.git.lorenzo.stoakes@oracle.com>
On 10/20/24 10:20, Lorenzo Stoakes wrote:
> Utilise the kselftest harmness to implement tests for the guard page
> implementation.
>
> We start by implement basic tests asserting that guard pages can be
> established (poisoned), cleared (remedied) and that touching poisoned pages
> result in SIGSEGV. We also assert that, in remedying a range, non-poison
> pages remain intact.
>
> We then examine different operations on regions containing poison markers
> behave to ensure correct behaviour:
>
> * Operations over multiple VMAs operate as expected.
> * Invoking MADV_GUARD_POISION / MADV_GUARD_REMEDY via process_madvise() in
> batches works correctly.
> * Ensuring that munmap() correctly tears down poison markers.
> * Using mprotect() to adjust protection bits does not in any way override
> or cause issues with poison markers.
> * Ensuring that splitting and merging VMAs around poison markers causes no
> issue - i.e. that a marker which 'belongs' to one VMA can function just
> as well 'belonging' to another.
> * Ensuring that madvise(..., MADV_DONTNEED) does not remove poison markers.
> * Ensuring that mlock()'ing a range containing poison markers does not
> cause issues.
> * Ensuring that mremap() can move a poisoned range and retain poison
> markers.
> * Ensuring that mremap() can expand a poisoned range and retain poison
> markers (perhaps moving the range).
> * Ensuring that mremap() can shrink a poisoned range and retain poison
> markers.
> * Ensuring that forking a process correctly retains poison markers.
> * Ensuring that forking a VMA with VM_WIPEONFORK set behaves sanely.
> * Ensuring that lazyfree simply clears poison markers.
> * Ensuring that userfaultfd can co-exist with guard pages.
> * Ensuring that madvise(..., MADV_POPULATE_READ) and
> madvise(..., MADV_POPULATE_WRITE) error out when encountering
> poison markers.
> * Ensuring that madvise(..., MADV_COLD) and madvise(..., MADV_PAGEOUT) do
> not remove poison markers.
>
> If any test is unable to be run due to lack of permissions, that test is
> skipped.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> tools/testing/selftests/mm/.gitignore | 1 +
> tools/testing/selftests/mm/Makefile | 1 +
> tools/testing/selftests/mm/guard-pages.c | 1228 ++++++++++++++++++++++
> 3 files changed, 1230 insertions(+)
> create mode 100644 tools/testing/selftests/mm/guard-pages.c
>
> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> index 689bbd520296..8f01f4da1c0d 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -54,3 +54,4 @@ droppable
> hugetlb_dio
> pkey_sighandler_tests_32
> pkey_sighandler_tests_64
> +guard-pages
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index 02e1204971b0..15c734d6cfec 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
> TEST_GEN_FILES += hugetlb_madv_vs_map
> TEST_GEN_FILES += hugetlb_dio
> TEST_GEN_FILES += droppable
> +TEST_GEN_FILES += guard-pages
>
> ifneq ($(ARCH),arm64)
> TEST_GEN_FILES += soft-dirty
> diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-pages.c
> new file mode 100644
> index 000000000000..f67d2700d44a
> --- /dev/null
> +++ b/tools/testing/selftests/mm/guard-pages.c
> @@ -0,0 +1,1228 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#define _GNU_SOURCE
> +#include "../kselftest_harness.h"
> +#include <asm-generic/mman.h> /* Force the import of the tools version. */
> +#include <assert.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/userfaultfd.h>
> +#include <setjmp.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <sys/uio.h>
> +#include <unistd.h>
> +
> +/*
> + * Ignore the checkpatch warning, as per the C99 standard, section 7.14.1.1:
> + *
> + * "If the signal occurs other than as the result of calling the abort or raise
> + * function, the behavior is undefined if the signal handler refers to any
> + * object with static storage duration other than by assigning a value to an
> + * object declared as volatile sig_atomic_t"
> + */
> +static volatile sig_atomic_t signal_jump_set;
> +static sigjmp_buf signal_jmp_buf;
> +
> +/*
> + * Ignore the checkpatch warning, we must read from x but don't want to do
> + * anything with it in order to trigger a read page fault. We therefore must use
> + * volatile to stop the compiler from optimising this away.
> + */
> +#define FORCE_READ(x) (*(volatile typeof(x) *)x)
> +
Thank you.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 21:33 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: <0f7a6b69-5706-4010-ba7a-68a071922c80@redhat.com>
On Mon, Oct 21, 2024 at 11:20:03PM +0200, David Hildenbrand wrote:
> > > 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.
> >
> > The looping mechanism is fine for dealing with concurrent faults. There's
> > no actual _race_ due to PTL, it's just that a user could repeatedly
> > populate stuff stupidly in a range that is meant to have poison markers put
> > in.
> >
> > It's not likely and would be kind of an abusive of the interface, and it'd
> > really be a process just hurting itself.
> >
> > In nearly all cases you won't zap at all. The whole point is it's
> > optimistic. In 99.99% of others you zap once...
>
> Exactly! And that's why I am questioning whether the kernel should care
> about that. See below.
>
> >
> > >
> > > Have to do some more reading on this.
> >
> > May I suggest a book on the history of the prodigy?
>
> :D
>
> >
> > >
> > > >
> > > > 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.
> >
> > Yeah I mean originally I had a -EAGAIN which was sort of equivalent of this
> > but Jann pointed out you're just shifting work to userland who would loop
> > and repeat.
> >
> > I just don't see why we'd do this.
> >
> > In fact I was looking at the series and thinking 'wow it's actually a
> > really small delta' and being proud but... still not KIS enough apparently
> > ;)
>
> You know, I read a lot of kernel code ... and mfill_atomic_install_pte() is
> what popped in my head: if there is already something, let user space handle
> it, because it is unexpected.
>
> The uffd interface is slightly better, as it gives you the number of
> processed PTEs back, which madvise() is not designed for.
>
> But maybe this (returning how many we already processed) is not required due
> to the nature of guard pages (below).
>
> >
> > >
> > > 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.
> >
> > I don't think there's really any value in that. There's just no sensible
> > situation in which a user would care about this I don't think.
>
> Making sure nobody touches an area, and wile doing that somebody already
> touched that area? I guess it could be worked around by
> mprotect(PROT_NONE),madvise(GUARD),mprotect(PROT_READ|PROT_WRITE) ... which
> is not particularly nice :)
>
> >
> > And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
> > then why not just do that in the kernel?
>
> Heh, no!
>
> If user space doesn't expect there to be something, it should *fail*. That's
> likely going to be the majority of use cases for guard pages (happy to be
> told otherwise). No retry.
>
> And if user space expects there to be something it should zap ahead of time
> (which some allocators maybe already do to free up memory after free()) to
> then install the guard. No retry.
>
> There is this case where user space might be unsure. There, it might make
> sense to retry exactly once.
>
> >
> > Trying to explain to a user 'hey this is for installing guard pages but if
> > there's a facing fault it'll fail and that could keep happening and then
> > you'll have to zap and maybe in a loop' just... seems like a bloody awful
> > interface?
>
> Hope my example above made it clearer. This "retry forever until it works"
> use case doesn't quite make sense to me, but I might just be missing
> something important.
It won't be forever in any case other than a process that is abusing itself.
Then it's a security question and... see below!
>
> But again, I have to do more reading on the history of the current approach
> ... and it's fairly late here.
Yes late for me too and I've been working since 8am pretty solid so... :)
>
>
> --
> Cheers,
>
> David / dhildenb
>
So we all agree it's very unlikely, we all agree that even if it happens it'll
happen only once, so what is the problem with the loop exactly? Other than
philosophical?
We do have other loops in the kernel like this... it'd really be the same as the
process throttling itself and whether it loops in userland or kernel land is
kind of immaterial really.
So to me this comes down fundamentally - is this a genuine security/abuse
concern?
Since Jann suggested this, I find it unlikely. But I have pinged him for his
input.
Otherwise I don't know what we're arguing about... and I think we're fine!
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Vlastimil Babka @ 2024-10-21 21:35 UTC (permalink / raw)
To: David Hildenbrand, Lorenzo Stoakes
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: <0f7a6b69-5706-4010-ba7a-68a071922c80@redhat.com>
On 10/21/24 23:20, David Hildenbrand wrote:
>> I don't think there's really any value in that. There's just no sensible
>> situation in which a user would care about this I don't think.
>
> Making sure nobody touches an area, and wile doing that somebody already
> touched that area? I guess it could be worked around by
> mprotect(PROT_NONE),madvise(GUARD),mprotect(PROT_READ|PROT_WRITE) ...
> which is not particularly nice :)
>
>>
>> And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
>> then why not just do that in the kernel?
>
> Heh, no!
>
> If user space doesn't expect there to be something, it should *fail*.
> That's likely going to be the majority of use cases for guard pages
> (happy to be told otherwise). No retry.
>
> And if user space expects there to be something it should zap ahead of
> time (which some allocators maybe already do to free up memory after
> free()) to then install the guard. No retry.
>
> There is this case where user space might be unsure. There, it might
> make sense to retry exactly once.
I've thought so too and the RFC was implemented like this, but Jann came up
with a scenario where a THP can cause the range including our
to-be-installed guard pte to be populated even if the userspace is not
trying to access that exact address, see here:
https://lore.kernel.org/all/CAG48ez3vqbqyWb4bLdpqSUnhwqGo2OQetecNhEGPdCGDr94nbQ@mail.gmail.com/
So unless we can't *reliably* detect that userspace is really shooting
itself in the foot and return a failure to install guard pte *only* in that
case (which would be useful), and not horribly complicate everything to
ensure that reliability and to avoid false positives due to races with
THP's, then it's probably better to just retry as this version does.
>>
>> Trying to explain to a user 'hey this is for installing guard pages but if
>> there's a facing fault it'll fail and that could keep happening and then
>> you'll have to zap and maybe in a loop' just... seems like a bloody awful
>> interface?
>
> Hope my example above made it clearer. This "retry forever until it
> works" use case doesn't quite make sense to me, but I might just be
> missing something important.
>
> But again, I have to do more reading on the history of the current
> approach ... and it's fairly late here.
Yeah see the RFC thread linked above.
^ permalink raw reply
* Re: (subset) [PATCH RFC v3 00/10] extensible syscalls: CHECK_FIELDS to allow for easier feature detection
From: Aleksa Sarai @ 2024-10-21 21:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, stable, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Alexander Viro, Jan Kara,
Shuah Khan
In-Reply-To: <20241021-kraut-fundgrube-cf1648e59df4@brauner>
[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]
On 2024-10-21, Christian Brauner <brauner@kernel.org> wrote:
> On Thu, 10 Oct 2024 07:40:33 +1100, Aleksa Sarai wrote:
> > This is something that I've been thinking about for a while. We had a
> > discussion at LPC 2020 about this[1] but the proposals suggested there
> > never materialised.
> >
> > In short, it is quite difficult for userspace to detect the feature
> > capability of syscalls at runtime. This is something a lot of programs
> > want to do, but they are forced to create elaborate scenarios to try to
> > figure out if a feature is supported without causing damage to the
> > system. For the vast majority of cases, each individual feature also
> > needs to be tested individually (because syscall results are
> > all-or-nothing), so testing even a single syscall's feature set can
> > easily inflate the startup time of programs.
> >
> > [...]
>
> I think the copy_struct_to_user() is useful especially now that we'll gain
> another user with pidfd_info.
Once we start extending pidfd_info, it might be necessary to add some
more helpers to make it easier to figure out what bits to set in the
returned request mask.
> ---
>
> Applied to the vfs.usercopy branch of the vfs/vfs.git tree.
> Patches in the vfs.usercopy branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.usercopy
>
> [01/10] uaccess: add copy_struct_to_user helper
> https://git.kernel.org/vfs/vfs/c/424a55a4a908
> [02/10] sched_getattr: port to copy_struct_to_user
> https://git.kernel.org/vfs/vfs/c/112cca098a70
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-21 21:46 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: <b92c58da-ec94-409b-8cdf-46eb3d2c7870@suse.cz>
On Mon, Oct 21, 2024 at 11:35:24PM +0200, Vlastimil Babka wrote:
> On 10/21/24 23:20, David Hildenbrand wrote:
> >> I don't think there's really any value in that. There's just no sensible
> >> situation in which a user would care about this I don't think.
> >
> > Making sure nobody touches an area, and wile doing that somebody already
> > touched that area? I guess it could be worked around by
> > mprotect(PROT_NONE),madvise(GUARD),mprotect(PROT_READ|PROT_WRITE) ...
> > which is not particularly nice :)
> >
> >>
> >> And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
> >> then why not just do that in the kernel?
> >
> > Heh, no!
> >
> > If user space doesn't expect there to be something, it should *fail*.
> > That's likely going to be the majority of use cases for guard pages
> > (happy to be told otherwise). No retry.
> >
> > And if user space expects there to be something it should zap ahead of
> > time (which some allocators maybe already do to free up memory after
> > free()) to then install the guard. No retry.
> >
> > There is this case where user space might be unsure. There, it might
> > make sense to retry exactly once.
>
> I've thought so too and the RFC was implemented like this, but Jann came up
> with a scenario where a THP can cause the range including our
> to-be-installed guard pte to be populated even if the userspace is not
> trying to access that exact address, see here:
>
> https://lore.kernel.org/all/CAG48ez3vqbqyWb4bLdpqSUnhwqGo2OQetecNhEGPdCGDr94nbQ@mail.gmail.com/
>
> So unless we can't *reliably* detect that userspace is really shooting
> itself in the foot and return a failure to install guard pte *only* in that
> case (which would be useful), and not horribly complicate everything to
> ensure that reliability and to avoid false positives due to races with
> THP's, then it's probably better to just retry as this version does.
It would be complicated, and I'd reallly like to avoid trying to detect
this. It feels a bit whack-a-mole because maybe there's other scenarios
we've not thought about that could be equally problematic?
>
> >>
> >> Trying to explain to a user 'hey this is for installing guard pages but if
> >> there's a facing fault it'll fail and that could keep happening and then
> >> you'll have to zap and maybe in a loop' just... seems like a bloody awful
> >> interface?
> >
> > Hope my example above made it clearer. This "retry forever until it
> > works" use case doesn't quite make sense to me, but I might just be
> > missing something important.
> >
> > But again, I have to do more reading on the history of the current
> > approach ... and it's fairly late here.
>
> Yeah see the RFC thread linked above.
>
Right, but I don't think this is the only scenario that can happen, and I
think, FWIW, yet again the fundamental point comes down to 'is it a
problem?'
Because if looping like this isn't, then problem solved we can all high 5
and go home listening to the prodigy and full happiness.
If not then we can revisit.
And how could it be a problem? Surely only security or DoS
potential. Hopefully Jann can give some positive feedback on that.
We could also, and I hate to say it, but... try to find some means of
checking on reasonable forward progress in the loop if we had to or some
other 'reasonable attempt'.
But let's see...
^ permalink raw reply
* Re: [PATCH v2 5/5] selftests/mm: add self tests for guard page feature
From: Lorenzo Stoakes @ 2024-10-22 10:25 UTC (permalink / raw)
To: Shuah Khan
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, 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: <360241aa-49ec-42b1-99c9-759a9a0873a0@linuxfoundation.org>
On Mon, Oct 21, 2024 at 03:31:59PM -0600, Shuah Khan wrote:
> On 10/20/24 10:20, Lorenzo Stoakes wrote:
> > Utilise the kselftest harmness to implement tests for the guard page
> > implementation.
> >
> > We start by implement basic tests asserting that guard pages can be
> > established (poisoned), cleared (remedied) and that touching poisoned pages
> > result in SIGSEGV. We also assert that, in remedying a range, non-poison
> > pages remain intact.
> >
> > We then examine different operations on regions containing poison markers
> > behave to ensure correct behaviour:
> >
> > * Operations over multiple VMAs operate as expected.
> > * Invoking MADV_GUARD_POISION / MADV_GUARD_REMEDY via process_madvise() in
> > batches works correctly.
> > * Ensuring that munmap() correctly tears down poison markers.
> > * Using mprotect() to adjust protection bits does not in any way override
> > or cause issues with poison markers.
> > * Ensuring that splitting and merging VMAs around poison markers causes no
> > issue - i.e. that a marker which 'belongs' to one VMA can function just
> > as well 'belonging' to another.
> > * Ensuring that madvise(..., MADV_DONTNEED) does not remove poison markers.
> > * Ensuring that mlock()'ing a range containing poison markers does not
> > cause issues.
> > * Ensuring that mremap() can move a poisoned range and retain poison
> > markers.
> > * Ensuring that mremap() can expand a poisoned range and retain poison
> > markers (perhaps moving the range).
> > * Ensuring that mremap() can shrink a poisoned range and retain poison
> > markers.
> > * Ensuring that forking a process correctly retains poison markers.
> > * Ensuring that forking a VMA with VM_WIPEONFORK set behaves sanely.
> > * Ensuring that lazyfree simply clears poison markers.
> > * Ensuring that userfaultfd can co-exist with guard pages.
> > * Ensuring that madvise(..., MADV_POPULATE_READ) and
> > madvise(..., MADV_POPULATE_WRITE) error out when encountering
> > poison markers.
> > * Ensuring that madvise(..., MADV_COLD) and madvise(..., MADV_PAGEOUT) do
> > not remove poison markers.
> >
> > If any test is unable to be run due to lack of permissions, that test is
> > skipped.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > tools/testing/selftests/mm/.gitignore | 1 +
> > tools/testing/selftests/mm/Makefile | 1 +
> > tools/testing/selftests/mm/guard-pages.c | 1228 ++++++++++++++++++++++
> > 3 files changed, 1230 insertions(+)
> > create mode 100644 tools/testing/selftests/mm/guard-pages.c
> >
> > diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> > index 689bbd520296..8f01f4da1c0d 100644
> > --- a/tools/testing/selftests/mm/.gitignore
> > +++ b/tools/testing/selftests/mm/.gitignore
> > @@ -54,3 +54,4 @@ droppable
> > hugetlb_dio
> > pkey_sighandler_tests_32
> > pkey_sighandler_tests_64
> > +guard-pages
> > diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> > index 02e1204971b0..15c734d6cfec 100644
> > --- a/tools/testing/selftests/mm/Makefile
> > +++ b/tools/testing/selftests/mm/Makefile
> > @@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
> > TEST_GEN_FILES += hugetlb_madv_vs_map
> > TEST_GEN_FILES += hugetlb_dio
> > TEST_GEN_FILES += droppable
> > +TEST_GEN_FILES += guard-pages
> > ifneq ($(ARCH),arm64)
> > TEST_GEN_FILES += soft-dirty
> > diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-pages.c
> > new file mode 100644
> > index 000000000000..f67d2700d44a
> > --- /dev/null
> > +++ b/tools/testing/selftests/mm/guard-pages.c
> > @@ -0,0 +1,1228 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#define _GNU_SOURCE
> > +#include "../kselftest_harness.h"
> > +#include <asm-generic/mman.h> /* Force the import of the tools version. */
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/userfaultfd.h>
> > +#include <setjmp.h>
> > +#include <signal.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/mman.h>
> > +#include <sys/syscall.h>
> > +#include <sys/uio.h>
> > +#include <unistd.h>
> > +
> > +/*
> > + * Ignore the checkpatch warning, as per the C99 standard, section 7.14.1.1:
> > + *
> > + * "If the signal occurs other than as the result of calling the abort or raise
> > + * function, the behavior is undefined if the signal handler refers to any
> > + * object with static storage duration other than by assigning a value to an
> > + * object declared as volatile sig_atomic_t"
> > + */
> > +static volatile sig_atomic_t signal_jump_set;
> > +static sigjmp_buf signal_jmp_buf;
> > +
> > +/*
> > + * Ignore the checkpatch warning, we must read from x but don't want to do
> > + * anything with it in order to trigger a read page fault. We therefore must use
> > + * volatile to stop the compiler from optimising this away.
> > + */
> > +#define FORCE_READ(x) (*(volatile typeof(x) *)x)
> > +
>
> Thank you.
>
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Thanks! :)
>
> thanks,
> -- Shuah
>
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Jann Horn @ 2024-10-22 19:08 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Lorenzo Stoakes, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney,
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: <f2448c59-0456-49e8-9676-609629227808@suse.cz>
On Mon, Oct 21, 2024 at 10:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> On 10/21/24 22:27, Lorenzo Stoakes wrote:
> > On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> >> > + 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.
>
> If Jann is fine with this then it must be secure enough.
My thinking there was:
We can legitimately race with adjacent faults populating the area
we're operating on with THP pages; as long as the zapping and
poison-marker-setting are separate, *someone* will have to do the
retry. Either we do it in the kernel, or we tell userspace to handle
it, but having the kernel take care of it is preferable because it
makes the stable UAPI less messy.
One easy way to do it in the kernel would be to return -ERESTARTNOINTR
after the zap_page_range_single() instead of jumping back up, which in
terms of locking and signal handling and such would be equivalent to
looping in userspace (because really that's what -ERESTARTNOINTR does
- it returns out to userspace and moves the instruction pointer back
to restart the syscall). Though if we do that immediately, it might
make MADV_POISON unnecessarily slow, so we should probably retry once
before doing that. The other easy way is to just loop here.
The cond_resched() and pending fatal signal check mean that (except on
CONFIG_PREEMPT_NONE) the only differences between the current
implementation and looping in userspace are that we don't handle
non-fatal signals in between iterations and that we keep hogging the
mmap_lock in read mode. We do already have a bunch of codepaths that
retry on concurrent page table changes, like when zap_pte_range()
encounters a pte_offset_map_lock() failure; though I guess the
difference is that the retry on those is just a couple instructions,
which would be harder to race consistently, while here we redo walks
across the entire range, which should be fairly easy to race
repeatedly.
So I guess you have a point that this might be the easiest way to
stall other tasks that are trying to take mmap_lock for an extended
amount of time, I did not fully consider that... and then I guess you
could use that to slow down usercopy fault handling (once the lock
switches to handoff mode because of a stalled writer?) or slow down
other processes trying to read /proc/$pid/cmdline?
You can already indefinitely hog the mmap_lock with FUSE, though that
requires that you can mount a FUSE filesystem (which you wouldn't be
able in reasonably sandboxed code) and that you can find something
like a pin_user_pages() call that can't drop the mmap lock in between,
and there aren't actually that many of those...
So I guess you have a point and the -ERESTARTNOINTR approach would be
a little bit nicer, as long as it's easy to implement.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: David Hildenbrand @ 2024-10-22 19:13 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes
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: <8329667f-73b6-48fe-8f3c-07c741462fee@suse.cz>
On 21.10.24 19:26, Vlastimil Babka wrote:
> 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.
Note that in VM context "uffd poison" really just is "this was hwpoison
on the source VM, so we mimic that on the destination VM, because the
data *is* lost" -- so you want the exact same behavior.
For example, when a VM reboots you might just want to ZAP these hwpoison
entries, and get fresh pages on next access.
So to me it makes sense that they are treated equally.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: David Hildenbrand @ 2024-10-22 19:18 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes
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: <b92c58da-ec94-409b-8cdf-46eb3d2c7870@suse.cz>
On 21.10.24 23:35, Vlastimil Babka wrote:
> On 10/21/24 23:20, David Hildenbrand wrote:
>>> I don't think there's really any value in that. There's just no sensible
>>> situation in which a user would care about this I don't think.
>>
>> Making sure nobody touches an area, and wile doing that somebody already
>> touched that area? I guess it could be worked around by
>> mprotect(PROT_NONE),madvise(GUARD),mprotect(PROT_READ|PROT_WRITE) ...
>> which is not particularly nice :)
>>
>>>
>>> And if you're saying 'hey do MADV_DONTNEED if this fails and keep trying!'
>>> then why not just do that in the kernel?
>>
>> Heh, no!
>>
>> If user space doesn't expect there to be something, it should *fail*.
>> That's likely going to be the majority of use cases for guard pages
>> (happy to be told otherwise). No retry.
>>
>> And if user space expects there to be something it should zap ahead of
>> time (which some allocators maybe already do to free up memory after
>> free()) to then install the guard. No retry.
>>
>> There is this case where user space might be unsure. There, it might
>> make sense to retry exactly once.
>
> I've thought so too and the RFC was implemented like this, but Jann came up
> with a scenario where a THP can cause the range including our
> to-be-installed guard pte to be populated even if the userspace is not
> trying to access that exact address, see here:
>
> https://lore.kernel.org/all/CAG48ez3vqbqyWb4bLdpqSUnhwqGo2OQetecNhEGPdCGDr94nbQ@mail.gmail.com/
Ah, THP, I should have realized that myself. Yes indeed, in some cases
we'll have to zap because something was already populated. Not sure how
often it will happen in practice, will depend on the use case.
For use cases like one "one static guard page every 2MiB", I would
assume we install the guard pages early, before expecting any page
faults in that area. Likely there are other ones where it might happen
more frequently.
For uffd that does not apply, because khugepaged backs off with uffd
enabled and the only way to resolve a fault is using uffd -- which
places exactly what was requested by user space. So, no populated PTEs
without actual page faults on the corresponding virtual addresses.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-22 19:35 UTC (permalink / raw)
To: Jann Horn
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney,
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: <CAG48ez3WS3EH9DuhE1b+7AX3+1=dVtd1M7y_5Ev4Shp2YxiYWg@mail.gmail.com>
On Tue, Oct 22, 2024 at 09:08:53PM +0200, Jann Horn wrote:
> On Mon, Oct 21, 2024 at 10:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > On 10/21/24 22:27, Lorenzo Stoakes wrote:
> > > On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> > >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > >> > + 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.
> >
> > If Jann is fine with this then it must be secure enough.
>
> My thinking there was:
>
> We can legitimately race with adjacent faults populating the area
> we're operating on with THP pages; as long as the zapping and
> poison-marker-setting are separate, *someone* will have to do the
> retry. Either we do it in the kernel, or we tell userspace to handle
> it, but having the kernel take care of it is preferable because it
> makes the stable UAPI less messy.
>
> One easy way to do it in the kernel would be to return -ERESTARTNOINTR
> after the zap_page_range_single() instead of jumping back up, which in
> terms of locking and signal handling and such would be equivalent to
> looping in userspace (because really that's what -ERESTARTNOINTR does
> - it returns out to userspace and moves the instruction pointer back
> to restart the syscall). Though if we do that immediately, it might
> make MADV_POISON unnecessarily slow, so we should probably retry once
> before doing that. The other easy way is to just loop here.
Yes we should definitely retry probably a few times to cover the rare
situation of a THP race as you describe under non-abusive circumstances.
>
> The cond_resched() and pending fatal signal check mean that (except on
> CONFIG_PREEMPT_NONE) the only differences between the current
> implementation and looping in userspace are that we don't handle
> non-fatal signals in between iterations and that we keep hogging the
> mmap_lock in read mode. We do already have a bunch of codepaths that
> retry on concurrent page table changes, like when zap_pte_range()
> encounters a pte_offset_map_lock() failure; though I guess the
> difference is that the retry on those is just a couple instructions,
> which would be harder to race consistently, while here we redo walks
> across the entire range, which should be fairly easy to race
> repeatedly.
>
> So I guess you have a point that this might be the easiest way to
> stall other tasks that are trying to take mmap_lock for an extended
> amount of time, I did not fully consider that... and then I guess you
> could use that to slow down usercopy fault handling (once the lock
> switches to handoff mode because of a stalled writer?) or slow down
> other processes trying to read /proc/$pid/cmdline?
Hm does that need a write lock?
>
> You can already indefinitely hog the mmap_lock with FUSE, though that
> requires that you can mount a FUSE filesystem (which you wouldn't be
> able in reasonably sandboxed code) and that you can find something
> like a pin_user_pages() call that can't drop the mmap lock in between,
> and there aren't actually that many of those...
>
> So I guess you have a point and the -ERESTARTNOINTR approach would be
> a little bit nicer, as long as it's easy to implement.
I can go ahead and do it that way if nobody objects, with a few loops
before we do it... which hopefully covers off all the concerns?
Thanks
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Jann Horn @ 2024-10-22 19:57 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney,
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: <2647d37b-3482-4fc9-8da2-1158ebdc919e@lucifer.local>
On Tue, Oct 22, 2024 at 9:35 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
> On Tue, Oct 22, 2024 at 09:08:53PM +0200, Jann Horn wrote:
> > On Mon, Oct 21, 2024 at 10:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > > On 10/21/24 22:27, Lorenzo Stoakes wrote:
> > > > On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> > > >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > > >> > + 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.
> > >
> > > If Jann is fine with this then it must be secure enough.
> >
> > My thinking there was:
> >
> > We can legitimately race with adjacent faults populating the area
> > we're operating on with THP pages; as long as the zapping and
> > poison-marker-setting are separate, *someone* will have to do the
> > retry. Either we do it in the kernel, or we tell userspace to handle
> > it, but having the kernel take care of it is preferable because it
> > makes the stable UAPI less messy.
> >
> > One easy way to do it in the kernel would be to return -ERESTARTNOINTR
> > after the zap_page_range_single() instead of jumping back up, which in
> > terms of locking and signal handling and such would be equivalent to
> > looping in userspace (because really that's what -ERESTARTNOINTR does
> > - it returns out to userspace and moves the instruction pointer back
> > to restart the syscall). Though if we do that immediately, it might
> > make MADV_POISON unnecessarily slow, so we should probably retry once
> > before doing that. The other easy way is to just loop here.
>
> Yes we should definitely retry probably a few times to cover the rare
> situation of a THP race as you describe under non-abusive circumstances.
>
> >
> > The cond_resched() and pending fatal signal check mean that (except on
> > CONFIG_PREEMPT_NONE) the only differences between the current
> > implementation and looping in userspace are that we don't handle
> > non-fatal signals in between iterations and that we keep hogging the
> > mmap_lock in read mode. We do already have a bunch of codepaths that
> > retry on concurrent page table changes, like when zap_pte_range()
> > encounters a pte_offset_map_lock() failure; though I guess the
> > difference is that the retry on those is just a couple instructions,
> > which would be harder to race consistently, while here we redo walks
> > across the entire range, which should be fairly easy to race
> > repeatedly.
> >
> > So I guess you have a point that this might be the easiest way to
> > stall other tasks that are trying to take mmap_lock for an extended
> > amount of time, I did not fully consider that... and then I guess you
> > could use that to slow down usercopy fault handling (once the lock
> > switches to handoff mode because of a stalled writer?) or slow down
> > other processes trying to read /proc/$pid/cmdline?
>
> Hm does that need a write lock?
No, but if you have one reader that is hogging the rwsem, and then a
writer is queued up on the rwsem afterwards, I think new readers will
sometimes be queued up behind the writer. So even though the rwsem is
only actually held by a reader, new readers can't immediately take the
rwsem because the rwsem code thinks that would be unfair to a pending
writer who just wants to make some quick change. I'm not super
familiar with this code, but basically I think it works roughly like
this:
If the rwsem code notices that a bunch of readers are preventing a
writer from taking the lock, the rwsem code will start queuing new
readers behind the queued writer. You can see in rwsem_read_trylock()
that the trylock fastpath is skipped if anyone is waiting on the rwsem
or the handoff flag is set, and in rwsem_down_read_slowpath() the
"reader optimistic lock stealing" path is skipped if the lock is
currently held by multiple readers or if the handoff bit is set.
The handoff bit can be set in rwsem_try_write_lock() by a writer "if
it is an RT task or wait in the wait queue for too long". Basically I
think it means something like "I think other users of the lock are
hogging it more than they should, stop stealing the lock from me".
And the RWSEM_WAIT_TIMEOUT for triggering handoff mode is pretty
short, RWSEM_WAIT_TIMEOUT is defined to something like 4ms, so I think
that's how long writers tolerate the lock being hogged by readers
before they prevent new readers from stealing the lock.
^ permalink raw reply
* Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-22 20:45 UTC (permalink / raw)
To: Jann Horn
Cc: Vlastimil Babka, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney,
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: <CAG48ez2rrHeq5aNPA9pB-1T6n1WjCh+g+8Hp4QrZemqVb6UsYg@mail.gmail.com>
On Tue, Oct 22, 2024 at 09:57:39PM +0200, Jann Horn wrote:
> On Tue, Oct 22, 2024 at 9:35 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > On Tue, Oct 22, 2024 at 09:08:53PM +0200, Jann Horn wrote:
> > > On Mon, Oct 21, 2024 at 10:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > > > On 10/21/24 22:27, Lorenzo Stoakes wrote:
> > > > > On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> > > > >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > > > >> > + 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.
> > > >
> > > > If Jann is fine with this then it must be secure enough.
> > >
> > > My thinking there was:
> > >
> > > We can legitimately race with adjacent faults populating the area
> > > we're operating on with THP pages; as long as the zapping and
> > > poison-marker-setting are separate, *someone* will have to do the
> > > retry. Either we do it in the kernel, or we tell userspace to handle
> > > it, but having the kernel take care of it is preferable because it
> > > makes the stable UAPI less messy.
> > >
> > > One easy way to do it in the kernel would be to return -ERESTARTNOINTR
> > > after the zap_page_range_single() instead of jumping back up, which in
> > > terms of locking and signal handling and such would be equivalent to
> > > looping in userspace (because really that's what -ERESTARTNOINTR does
> > > - it returns out to userspace and moves the instruction pointer back
> > > to restart the syscall). Though if we do that immediately, it might
> > > make MADV_POISON unnecessarily slow, so we should probably retry once
> > > before doing that. The other easy way is to just loop here.
> >
> > Yes we should definitely retry probably a few times to cover the rare
> > situation of a THP race as you describe under non-abusive circumstances.
> >
> > >
> > > The cond_resched() and pending fatal signal check mean that (except on
> > > CONFIG_PREEMPT_NONE) the only differences between the current
> > > implementation and looping in userspace are that we don't handle
> > > non-fatal signals in between iterations and that we keep hogging the
> > > mmap_lock in read mode. We do already have a bunch of codepaths that
> > > retry on concurrent page table changes, like when zap_pte_range()
> > > encounters a pte_offset_map_lock() failure; though I guess the
> > > difference is that the retry on those is just a couple instructions,
> > > which would be harder to race consistently, while here we redo walks
> > > across the entire range, which should be fairly easy to race
> > > repeatedly.
> > >
> > > So I guess you have a point that this might be the easiest way to
> > > stall other tasks that are trying to take mmap_lock for an extended
> > > amount of time, I did not fully consider that... and then I guess you
> > > could use that to slow down usercopy fault handling (once the lock
> > > switches to handoff mode because of a stalled writer?) or slow down
> > > other processes trying to read /proc/$pid/cmdline?
> >
> > Hm does that need a write lock?
>
> No, but if you have one reader that is hogging the rwsem, and then a
> writer is queued up on the rwsem afterwards, I think new readers will
> sometimes be queued up behind the writer. So even though the rwsem is
> only actually held by a reader, new readers can't immediately take the
> rwsem because the rwsem code thinks that would be unfair to a pending
> writer who just wants to make some quick change. I'm not super
> familiar with this code, but basically I think it works roughly like
> this:
>
> If the rwsem code notices that a bunch of readers are preventing a
> writer from taking the lock, the rwsem code will start queuing new
> readers behind the queued writer. You can see in rwsem_read_trylock()
> that the trylock fastpath is skipped if anyone is waiting on the rwsem
> or the handoff flag is set, and in rwsem_down_read_slowpath() the
> "reader optimistic lock stealing" path is skipped if the lock is
> currently held by multiple readers or if the handoff bit is set.
>
> The handoff bit can be set in rwsem_try_write_lock() by a writer "if
> it is an RT task or wait in the wait queue for too long". Basically I
> think it means something like "I think other users of the lock are
> hogging it more than they should, stop stealing the lock from me".
> And the RWSEM_WAIT_TIMEOUT for triggering handoff mode is pretty
> short, RWSEM_WAIT_TIMEOUT is defined to something like 4ms, so I think
> that's how long writers tolerate the lock being hogged by readers
> before they prevent new readers from stealing the lock.
Ack makes sense! -ERESTARTNOINTR should help resolve this so definitely
unless anybody has any objection to that I'll go ahead and do a respin
taking that approach (+ all otehr fixes) for v2.
Thanks for your input!
^ permalink raw reply
* Re: [PATCH v4 1/4] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Shakeel Butt @ 2024-10-23 0:20 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard
In-Reply-To: <94a3210afe96c9d1d6f9460d7d37a43e5bc5f550.1729198898.git.lorenzo.stoakes@oracle.com>
On Thu, Oct 17, 2024 at 10:05:49PM GMT, Lorenzo Stoakes wrote:
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which narrows this to one approach of pinning
> the pid, with an optional output parameters for file->f_flags to avoid the
> need to hold onto a file to retrieve this.
>
> Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
^ permalink raw reply
* Re: [PATCH v4 2/4] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Shakeel Butt @ 2024-10-23 0:53 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard
In-Reply-To: <3bf7f2d8efc768007b5de8122275405afc9942d4.1729198898.git.lorenzo.stoakes@oracle.com>
On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
> It is useful to be able to utilise the pidfd mechanism to reference the
> current thread or process (from a userland point of view - thread group
> leader from the kernel's point of view).
>
> Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
> PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
>
> For convenience and to avoid confusion from userland's perspective we alias
> these:
>
> * PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
> the user will want to use, as they would find it surprising if for
> instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
> and that failed.
>
> * PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
> have no concept of thread groups or what a thread group leader is, and
> from userland's perspective and nomenclature this is what userland
> considers to be a process.
Should users use PIDFD_SELF_PROCESS in process_madvise() for self
madvise() (once the support is added)?
>
[...]
>
> +static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags)
> +{
> + bool is_thread = pidfd == PIDFD_SELF_THREAD;
> + enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
> + struct pid *pid = *task_pid_ptr(current, type);
> +
> + /* The caller expects an elevated reference count. */
> + get_pid(pid);
Do you want this helper to work for scenarios where pid is used across
context? Otherwise can't we get rid of this get and later put for self?
> + return pid;
> +}
> +
Overall looks good to me.
Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
^ permalink raw reply
* Re: [PATCH v2 0/5] implement lightweight guard pages
From: Dmitry Vyukov @ 2024-10-23 6:24 UTC (permalink / raw)
To: fw
Cc: James.Bottomley, Liam.Howlett, akpm, arnd, brauner, chris, david,
deller, hch, ink, jannh, jcmvbkbc, jeffxu, jhubbard, linux-alpha,
linux-api, linux-arch, linux-kernel, linux-kselftest, linux-mips,
linux-mm, linux-parisc, lorenzo.stoakes, mattst88, muchun.song,
paulmck, richard.henderson, shuah, sidhartha.kumar, surenb,
tsbogend, vbabka, willy, elver
In-Reply-To: <87a5eysmj1.fsf@mid.deneb.enyo.de>
Hi Florian, Lorenzo,
This looks great!
What I am VERY interested in is if poisoned pages cause SIGSEGV even when
the access happens in the kernel. Namely, the syscall still returns EFAULT,
but also SIGSEGV is queued on return to user-space.
Catching bad accesses in system calls is currently the weak spot for
all user-space bug detection tools (GWP-ASan, libefence, libefency, etc).
It's almost possible with userfaultfd, but catching faults in the kernel
requires admin capability, so not really an option for generic bug
detection tools (+inconvinience of userfaultfd setup/handler).
Intercepting all EFAULT from syscalls is not generally possible
(w/o ptrace, usually not an option as well), and EFAULT does not always
mean a bug.
Triggering SIGSEGV even in syscalls would be not just a performance
optimization, but a new useful capability that would allow it to catch
more bugs.
Thanks
^ 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