Re: [PATCH hotfix 6.12] mm/mmap: correct error handling in mmap_region()

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

On Tue, Oct 01, 2024 at 06:42:47PM GMT, Vlastimil Babka wrote:
> On 10/1/24 15:37, Lorenzo Stoakes wrote:
> > Commit f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > changed how error handling is performed in mmap_region().
> >
> > The error value defaults to -ENOMEM, but then gets reassigned immediately
> > to the result of vms_gather_munmap_vmas() if we are performing a MAP_FIXED
> > mapping over existing VMAs (and thus unmapping them).
> >
> > This overwrites the error value, potentially clearing it.
> >
> > After this, we invoke may_expand_vm() and possibly vm_area_alloc(), and
> > check to see if they failed. If they do so, then we perform error-handling
> > logic, but importantly, we do NOT update the error code.
> >
> > This means that, if vms_gather_munmap_vmas() succeeds, but one of these
> > calls does not, the function will return indicating no error, but rather an
> > address value of zero, which is entirely incorrect.
> >
> > Correct this and avoid future confusion by strictly setting error on each
> > and every occasion we jump to the error handling logic, and set the error
> > code immediately prior to doing so.
> >
> > This way we can see at a glance that the error code is always correct.
> >
> > Many thanks to Vegard Nossum who spotted this issue in discussion around
> > this problem.
> >
> > Reported-by: Bert Karwatzki <spasswolf@web.de>
> > Link: https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@web.de/
>
> I'd be surprised if that fixed the reported issue, but yeah this is a bug to
> fix anyway.

Yeah it seems that you brought about a tear in the space-time continuum and
the very moment you said that Bert reported that yes sadly this doesn't fix
it :>)

I thought maybe some very weird bug by this function returning a non-error
result (of zero!) when it should have failed might have triggered some
later maple tree corruption but yeah, long shot I guess :(

Still, ultimately it spawned from the report and forms part of an overall
fix of the function so may as well vaguely keep the R-b tag, I kept it a
link rather than 'closes' so should be fine.

>
> > Suggested-by: Vegard Nossum <vegard.nossum@oracle.com>
> > Fixes: f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
>

Thanks!