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
