Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Oct 21, 2024 at 07:00:53PM +0200, David Hildenbrand wrote:
> On 21.10.24 18:51, Lorenzo Stoakes wrote:
> > On Mon, Oct 21, 2024 at 06:44:04PM +0200, David Hildenbrand wrote:
> > > On 21.10.24 18:23, Lorenzo Stoakes wrote:
> > > > On Mon, Oct 21, 2024 at 06:00:20PM +0200, David Hildenbrand wrote:
> > > > [snip]
> > > > > >
> > > > > > To summarise for on-list:
> > > > > >
> > > > > > * MADV_FREE, while ostensibly being a 'lazy free' mechanism, has the
> > > > > >      ability to be 'cancelled' if you write to the memory. Also, after the
> > > > > >      freeing is complete, you can write to the memory to reuse it, the mapping
> > > > > >      is still there.
> > > > > >
> > > > > > * For hardware poison markers it makes sense to drop them as you're
> > > > > >      effectively saying 'I am done with this range that is now unbacked and
> > > > > >      expect to get an empty page should I use it now'. UFFD WP I am not sure
> > > > > >      about but presumably also fine.
> > > > > >
> > > > > > * However, guard pages are different - if you 'cancel' and you are left
> > > > > >      with a block of memory allocated to you by a pthread or userland
> > > > > >      allocator implementation, you don't want to then no longer be protected
> > > > > >      from overrunning into other thread memory.
> > > > >
> > > > > Agreed. What happens on MADV_DONTNEED/MADV_FREE on guard pages? Ignored or
> > > > > error? It sounds like a usage "error" to me (in contrast to munmap()).
> > > >
> > > > It's ignored, no errror. On MADV_DONTNEED we already left the guard pages in
> > > > place, from v3 we will do the same for MADV_FREE.
> > > >
> > > > I'm not sure I'd say it's an error per se, as somebody might have a use case
> > > > where they want to zap over a range but keep guard pages, perhaps an allocator
> > > > or something?
> > >
> > > Hm, not sure I see use for that.
> > >
> > > Staring at madvise_walk_vmas(), we return ENOMEM on VMA holes, but would
> > > process PROT_NONE. So current behavior is at least consistent with PROT_NONE
> > > handling (where something could be mapped, though).
> >
> > Err, the handling of holes is terrible, yes we return ENOMEM, but we _carry out
> > the whole procedure_ then return an error, an error _indistinguishable from an
> > error arising from any of the individual parts_.
> >
> > Which is just, awful.
>
> Yes, absolutely. I don't know why we decided to continue. And why we return
> ENOMEM ...

Anyway UAPI so no turning back now :)

>
> >
> > >
> > > No strong opinion.
> >
> > Well you used up your strong opinion on the naming ;)
>
> He, and I am out of energy for this year ;)

Haha understandable...

>
> In retrospective, "install or remove a guard PTE" is just much better than
> anything else ...
>
> So I should never have been mislead to suggest poison/unpoison as a
> replacement for poison/remedy :P

You know the reason ;)

>
> >
> > >
> > > >
> > > > Also the existing logic is that existing markers (HW poison, uffd-simulated HW
> > > > poison, uffd wp marker) are retained and no error raised on MADV_DONTNEED, and
> > > > no error on MADV_FREE either, so it'd be consistent with existing behaviour.
> > >
> > >
> > > HW poison / uffd-simulated HW poison are expected to be zapped: it's just
> > > like a mapped page with HWPOISON. So that is correct.
> >
> > Well, poison is _not_ zapped on MADV_DONTNEED but _is_ on MADV_FREE :) anyway, I
>
> Huh?
>
> madvise_dontneed_single_vma()->zap_page_range_single(details=NULL)->unmap_single_vma(details=NULL)
> ... zap_pte_range()
>
> } else if (is_hwpoison_entry(entry) ||
> 	   is_poisoned_swp_entry(entry)) {
> 	if (!should_zap_cows(details))
> 		continue;
> 	...
>
> Should just zap them.
>
> What am I missing?

Yeah ok it's me who's missing something here, I hadn't noticed details == NULL
so should_zap_cows() is true, my mistake!

In any case we explicitly add code here to prevent guard pages from going. I
will correct everything where I wrongly say otherwise, doh!

>
> > mean the MADV flags are a confusing mess generally, as per Vlasta's comments
> > which to begin with I strongly disagreed with then, discussing further, realsed
> > that no this is just a bit insane and had driven _me_ insane.
> >
> > >
> > > UFFD-WP behavior is ... weird. Would not expect MADV_DONTNEED to zap uffd-wp
> > > entries.
> > >
> > > >
> > > > Also semantically you are achieving what the calls expect you are freeing the
> > > > ranges since the guard page regions are unbacked so are already freed... so yeah
> > > > I don't think an error really makes sense here.
> > >
> > > I you compare it to a VMA hole, it make sense to fail. If we treat it like
> > > PROT_NONE, it make sense to skip them.
> > >
> > > >
> > > > We might also be limiting use cases by assuming they might _only_ be used for
> > > > allocators and such.
> > >
> > > I don't buy that as an argument, sorry :)
> > >
> > > "Let's map the kernel writable into all user space because otherwise we
> > > might be limiting use cases"
> >
> > That's a great idea! Patch series incoming, 1st April 2025... :>)
>
> :) Just flip the bit on x86 and we're done!

;)

>
> > >
> > >
> > > :P
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb
> > >
> >
> > Overall I think just always leaving in place except on remedy err sorry sorry
> > unpoison and munmap and not returning an error if encountered elsewhere (other
> > than, of course, GUP) is the right way forward and most in line with user
> > expectation and practical usage.
>
>
> Fine with me, make sure to document that is behaves like a PROT_NONE VMA,
> not like a memory hole, except when something would trigger a fault (GUP
> etc).

Ack will make sure to document.

>
>
> --
> Cheers,
>
> David / dhildenb
>