Re: [PATCH 2/4] powerpc/mm: Handle VDSO unmapping via close() rather than arch_unmap()

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Linus Torvalds <torvalds@linux-foundation.org> [240807 23:21]:
> On Wed, 7 Aug 2024 at 16:20, Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >

...

> 
> That said, I don't love how special powerpc is here.

I think more (all?) archs should be doing something like ppc when the
vdso is removed.  If someone removes the vdso, then the speed up
provided should just go away and the function calls shouldn't try to use
the quick look up and crash.

I view this as another 'caching of a vma pointer' issue that isn't
cleaned up when the vma goes away.

> 
> What we could do is to is
> 
>  - stop calling these things "special mappings", and just admit that
> it's for different vdso mappings and nothing else (for some odd reason
> arm and nios2 calls it a "kuser helper" rather than vdso, but it's the
> exact same thing)

But isn't it a special mapping?  We don't allow for merging of the vma,
the mlock handling has some odd behaviour with this vma, and there is
the comment in mm/internal.h's mlock_vma_folio about ignoring these
special vmas in a race.

There is also some other 'special mapping' of vvars too?  I haven't
looked deeply into this yet as my investigation was preempted by
vacation.

> 
>  - don't do this whole indirect function pointer thing with mremap and
> close at all, and just do this all unapologetically and for all
> architectures in the generic VM layer together with "if (vma->vm_start
> == mm->context.vdso)" etc.
> 
> that would get rid of the conceptual complexity of having different
> architectures doing different things (and the unnecessary overhead of
> having an indirect function pointer that just points to one single
> thing).
> 
> But I think the current "clean up the existing mess" is probably the
> less invasive one over "make the existing mess be explicitly about
> vdso and avoid unnecessary per-architecture differences".

Okay, sure.

> 
> If people want to, we can do the unification (and stop pretending the
> "special mappings" could be something else) later.
> 

I was planning to use the regular vma vm_ops to jump into the 'special
unmap code' and then do all the checks there.  IOW, keep the vma flagged
as VM_SPECIAL and call the special_mapping_whatever() function as a
regular vmops for, say, ->remove_vma() or ->mremap().  Keeping the flag
means all the race avoidance/locking/merging works the same as it does
today.

What I am trying to avoid is another arch_get_unmapped_area() scenario
where a bug exists for a decade in some versions of the cloned code.

Thanks,
Liam