| Message ID | cover.1729628198.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix error handling in mmap_region() and refactor | expand |
On Tue, Oct 22, 2024 at 09:40:51PM +0100, 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. > > 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 To clarify - this applies against Andrew's tree [0] in the mm-hotfixes-unstable branch in order to be applicable against outstanding hotfix patches for 6.12. [0]:https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/
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? > 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
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 >
On Tue, 22 Oct 2024 21:40:51 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > 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. I'm not sure that patches 4-8 are strictly 6.12 material. What's the thinking here?
On Wed, 23 Oct 2024 03:22:36 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 22 Oct 2024 21:40:51 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > 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. > > I'm not sure that patches 4-8 are strictly 6.12 material. What's the > thinking here? > A downside to merging all 8 patches into 6.12-rcX is that when -stable takes patches 1-4, they will have had negligible testing without the presence of patches 5-8. Patches 5-8 might accidentally fix something..
On Wed, Oct 23, 2024 at 03:22:36AM -0700, Andrew Morton wrote: > On Tue, 22 Oct 2024 21:40:51 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > 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. > > I'm not sure that patches 4-8 are strictly 6.12 material. What's the > thinking here? > Patch 4 is absolutely critical and without which 1-3 don't really matter all that much. It also needs to be backported (I will be doing some manual backports for that once landed). Patches 5-8 could be held back to 6.13 if preferred as, while nice to have as they do further work to eliminate this class of bugs, are not as important.