Re: [PATCH hotfix 6.12 0/8] fix error handling in mmap_region() and refactor

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

On Wed, Oct 23, 2024 at 10:37:04AM +0200, Vlastimil Babka wrote:
> On 10/22/24 22:40, Lorenzo Stoakes wrote:
> > The mmap_region() function is somewhat terrifying, with spaghetti-like
> > control flow and numerous means by which issues can arise and incomplete
> > state, memory leaks and other unpleasantness can occur.
> >
> > A large amount of the complexity arises from trying to handle errors late
> > in the process of mapping a VMA, which forms the basis of recently observed
> > issues with resource leaks and observable inconsistent state.
> >
> > This series goes to great lengths to simplify how mmap_region() works and
> > to avoid unwinding errors late on in the process of setting up the VMA for
> > the new mapping, and equally avoids such operations occurring while the VMA
> > is in an inconsistent state.
> >
> > The first four patches are intended for backporting to correct the
> > possibility of people encountering corrupted state while invoking mmap()
> > which is otherwise at risk of happening.
> >
> > After this we go further, refactoring the code, placing it in mm/vma.c in
> > order to make it eventually userland testable, and significantly
> > simplifying the logic to avoid this issue arising in future.
>
> Are the latter 4 patches thus also intended as hotfix for 6.12, or was it
> just due to git applying the same PATCH prefix to the whole series, and the
> real intention is 6.13?

They are intended as they are in the same vein, so one could argue we
should hotfix that and be done with it... but at the same time I realise
it's a bit of a stretch so I'm happy to split the last four patches for
6.13.

The issue is expressing that the the last 4 have a dependency on the first
4 and this not getting lost in the mix...

To be clear though, the first 4 do very much _have_ to be hotfixes and ones
which are backported to stable.

Andrew - we can do whatever makes sense to you here.

Thanks!

>
> > Lorenzo Stoakes (8):
> >   mm: avoid unsafe VMA hook invocation when error arises on mmap hook
> >   mm: unconditionally close VMAs on error
> >   mm: refactor map_deny_write_exec()
> >   mm: resolve faulty mmap_region() error path behaviour
> >   tools: testing: add additional vma_internal.h stubs
> >   mm: insolate mmap internal logic to mm/vma.c
> >   mm: refactor __mmap_region()
> >   mm: do not attempt second merge for file-backed VMAs
> >
> >  include/linux/mman.h             |  21 +-
> >  mm/internal.h                    |  44 ++++
> >  mm/mmap.c                        | 262 ++------------------
> >  mm/mprotect.c                    |   2 +-
> >  mm/nommu.c                       |   7 +-
> >  mm/vma.c                         | 403 ++++++++++++++++++++++++++++++-
> >  mm/vma.h                         | 101 +-------
> >  mm/vma_internal.h                |   5 +
> >  tools/testing/vma/vma_internal.h | 106 +++++++-
> >  9 files changed, 591 insertions(+), 360 deletions(-)
> >
> > --
> > 2.47.0
>