Re: [PATCH v3 16/16] mm/mmap: Move may_expand_vm() check in mmap_region()

[Dmitry Safonov]

Hi Liam,

On Wed, Jul 10, 2024 at 5:09 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * LEROY Christophe <christophe.leroy2@xxxxxxxxxxxxxxxxxx> [240710 08:59]:
> >
> ...
> > >>>>
> > >>>> Assuming the removal of the vdso does not cause the application to seg
> > >>>> fault, then the user visible change is that any vdso call after a failed
> > >>>> mmap(MAP_FIXED) call would result in a seg fault.  The only reason it
> > >>>> would fail is if the mapping process was attempting to map a large
> > >>>> enough area over the vdso (which is accounted and in the vma tree,
> > >>>> afaict) and ran out of memory. Note that this situation could arise
> > >>>> already since we could run out of memory (not accounting) after the
> > >>>> arch_unmap() call within the kernel.
> > >>>>
> > >>>> The code today can suffer the same fate, but not by the accounting
> > >>>> failure.  It can happen due to failure to allocate a new vma,
> > >>>> do_vmi_munmap() failure after the arch_unmap() call, or any of the other
> > >>>> failure scenarios later in the mmap_region() function.
> > >>>>
> > >>>> At the very least, this requires an expanded change log.
> > >>>
> ...
>
> > >>> I mean why are they unmapping the VDSO, why is that valid, why does it need
> > >>> that field to be set to NULL, is it possible to signify that in some other
> > >>> way etc.?
> > >>
> > >> It was originally for CRIU. So a niche workload on a niche architecture.
> > >>
> > >> But from the commit that added it, it sounds like CRIU was using mremap,
> > >> which should be handled these days by vdso_mremap(). So it could be that
> > >> arch_unmap() is not actually needed for CRIU anymore.
> > >
> > > Oh that's interesting!
> > >
> > >>
> > >> Then I guess we have to decide if removing our arch_unmap() would be an
> > >> ABI break, regardless of whether CRIU needs it or not.
> > >
> > > Seems to me like an internal implementation detail that should hopefully
> > > not result in anything that should have visible ABI impact?
> > >
> > > I guess this is something we ought to assess. It would be useful to
> > > eliminate hooks where we can so we can better control VMA behaviour without
> > > having to worry about an arch being able to do arbitrary things at
> > > unexpected times, especially pertinent where we change the order of things.
> > >
> >
> > I see you are talking about arch_unmap(). I didn't follow the entire
> > discussion but we have some related stuff here:
> > https://github.com/linuxppc/issues/issues/241
> >
> > If I remember correctly arch_unmap() should have gone away we Dmitry's
> > series
> > https://lore.kernel.org/lkml/20210611180242.711399-1-dima@xxxxxxxxxx/#r
> > but it hasn't been applied yet.
> >
>
> That is good news!
>
> To review, ppc is the only arch using this now and it sounds like you
> want to remove it too.
>
> Considering the age of that thread and the possibility of conflict with
> my series, can I drop the entire arch_unmap() function instead of
> modifying the handling in core mm?  I'm going to assume that's okay and
> start working on this for v4 (because there hasn't been a public reply
> for v4 since 2023/10/11).

Yeah, this kind of felt through the cracks. I meant to find time to
push v4, but from my job side I got motivated to do core networking
changes that were required by customers, from the other side I got
demotivated a bit by slight pushback for v3 with "justify why is it
needed at all?". For changes that are mostly cleanups and refactoring.

So, usually I don't give up on patches sets that yet make sense to me,
but priorities+motivation changed and the set moved lower on my todo
list.

If you have time and urge to finish this patch set, you are more than
welcome to adopt it :-)
Otherwise, I'll try to find time for them, but not in near short-term
as at this moment I cook tcp & selftests changes that I'd love to see
upstream.

> This would mean less arch-specific hooks and that's always a good idea.

Thanks,
           Dmitry