[v3,0/5] fix error handling in mmap_region() and refactor

Message

Lorenzo Stoakes Oct. 25, 2024, 12:26 p.m. UTC

Andrew - Sorry to be a pain, but please keep the first 4 hotfix patches
("mm: resolve faulty mmap_region() error path behaviour", "mm: refactor
map_deny_write_exec()", "mm: unconditionally close VMAs on error", "mm:
avoid unsafe VMA hook invocation when error arises on mmap hook") - in
place and replace the remaining 4 from the series with these, which are
intended for mm-unstable. Thanks!



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.

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.

This series builds on the previously submitted hotfix patches (see link to
v2 below) which addresses the most critical issues around mmap_region(),
and further works to improve mmap_region() complexity, stability, and
testability.

This series moves the code to mm/vma.c to render it userland testable,
refactors and simplifies it into smaller functions that are significantly
more readable.

It additionally avoids performing an attempt at a second merge mid-way
through allocating a new VMA, a dubious proposition at best and one that is
highly subject to subtle bugs.

Rather than do this, we simply note that we ought to retry the merge and do
this as a final step.

v3:
* Separated out this patch series intended for mm-unstable from the hotfixes.
* Grouped all merge state together in __mmap_region() as suggested by Vlastimil.
* Removed unnecessary vma_iter_config() as suggested by Vlastimil.
* Defined VMG_MMAP_STATE() macro so we don't retain VMG state _at all_ and avoid
  any strange behaviour that comes from re-using it.
* Fixed typo in MMAP_STATE.
* Added additional patch to remove unnecessary reset state logic from
  vma_merge_new_range().
* Moved the merge retry back to before __mmap_complete() - let's try to match
  existing behaviour as much as possible, which previously would not have
  accounted for a potential clearing of the lock flag.

v2:
* Marked first 4 patches as hotfixes, the rest as not.
* Improved comment in vma_close() as per Vlastiml.
* Updated hole byte count as per Jann.
* Updated comment in map_deny_write_exec() as per Jann.
* Dropped unnecessary vma_iter_free() as per Vlastmil, Liam.
* Corrected vms_abort_munmap_vmas() mistaken assumption about nr_pages as
  per Vlasitmil.
* Changed order of initial checks in mmap_region() to avoid user-visible
  side effects as per Vmastlil, Liam.
* Corrected silly incorrect use of vma field.
* Various style corrects as per Liam.
* Fix horrid mistake with merge VMA, reworked the logic to avoid that
  nonsense altogether.
* Add fields to map state rather than using vmg fields to avoid
  confusion/risk of vmg state changing breaking things.
* Replaced last commit removing merge retry with one that retries the
  merge, only sanely.
https://lore.kernel.org/all/[e-mail]/

v1:
https://lore.kernel.org/all/[e-mail]/

Lorenzo Stoakes (5):
  tools: testing: add additional vma_internal.h stubs
  mm: isolate mmap internal logic to mm/vma.c
  mm: refactor __mmap_region()
  mm: remove unnecessary reset state logic on merge new VMA
  mm: defer second attempt at merge on mmap()

 mm/mmap.c                        | 234 -----------------
 mm/vma.c                         | 437 ++++++++++++++++++++++++++++++-
 mm/vma.h                         |  97 +------
 mm/vma_internal.h                |   5 +
 tools/testing/vma/vma_internal.h | 115 +++++++-
 5 files changed, 546 insertions(+), 342 deletions(-)

--
2.47.0