Re: [PATCH mm-hotfixes] mm/vma: do not try to unmap a VMA if mmap_prepare() invoked from mmap()

[Lorenzo Stoakes <ljs@kernel.org>]

On Tue, Apr 21, 2026 at 11:52:23AM +0100, Pedro Falcato wrote:
> On Tue, Apr 21, 2026 at 11:21:50AM +0100, Lorenzo Stoakes wrote:
> > The mmap_prepare hook functionality includes the ability to invoke
> > mmap_prepare() from the mmap() hook of existing 'stacked' drivers, that is
> > ones which are capable of calling the mmap hooks of other drivers/file
> > systems (e.g. overlayfs, shm).
> >
> > As part of the mmap_prepare action functionality, we deal with errors by
> > unmapping the VMA should one arise. This works in the usual mmap_prepare
> > case, as we invoke this action at the last moment, when the VMA is
> > established in the maple tree.
> >
> > However, the mmap() hook passes a not-fully-established VMA pointer to the
> > caller (which is the motivation behind the mmap_prepare() work), which is
> > detached.
> >
> > So attempting to unmap a VMA in this state will be problematic, with the
> > most obvious symptom being a warning in vma_mark_detached(), because the
> > VMA is already detached.
> >
> > It's also unncessary - the mmap() handler will clean up the VMA on error.
> >
> > So to fix this issue, this patch propagates whether or not an mmap action
> > is being completed via the compatibility layer or directly.
> >
> > If the former, then we do not attempt VMA cleanup, if the latter, then we
> > do.
> >
> > This patch also updates the userland VMA tests to reflect the change.
> >
> > Fixes: ac0a3fc9c07d ("mm: add ability to take further action in vm_area_desc")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+db390288d141a1dccf96@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/69e69734.050a0220.24bfd3.0027.GAE@google.com/
> > Signed-off-by: Lorenzo Stoakes <ljs@kernel.org>
>
> How about something like the following:

(Normally I'd love helper struct stuf but...)

I was going to say in the commit message as to why I'm not doing that :) but
thought maybe I shouldn't spell it out.

But to spell it out - I don't want to do that, because it could be abused by
drivers and I don't want to add any possibility of doing that, as it defeats the
purpose of the change.

And adding checks to make sure a driver didn't mess with it complicates
everything.

I'd rather live with the added param, it's temporary and can be removed once the
mmap() hook is removed...

>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index a308e2c23b82..c29165de6d5c 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -868,6 +868,12 @@ struct mmap_action {
>          * completely set up.
>          */
>         bool hide_from_rmap_until_complete :1;
> +
> +       /*
> +        * Set if this mmap_action is part of compatibility with ->mmap().
> +        * Internal flag.
> +        */
> +       bool compat_mmap :1;
>  };
>
>  /*
> diff --git a/mm/util.c b/mm/util.c
> index 232c3930a662..5134f879566d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1229,6 +1229,7 @@ int __compat_vma_mmap(struct vm_area_desc *desc,
>         err = mmap_action_prepare(desc);
>         if (err)
>                 return err;
> +       desc->action.compat_mmap = 1;
>         /* Update the VMA from the descriptor. */
>         compat_set_vma_from_desc(vma, desc);
>         /* Complete any specified mmap actions. */
> @@ -1400,7 +1401,11 @@ static int mmap_action_finish(struct vm_area_struct *vma,
>
>         /* do_munmap() might take rmap lock, so release if held. */
>         maybe_rmap_unlock_action(vma, action);
> -       if (!err)
> +       /*
> +        * If this is invoked from the compatibility layer, post-mmap() hook
> +        * logic will handle cleanup for us.
> +        */
> +       if (!err || action->compat_mmap)
>                 return 0;
>
>         /*
>
>
> We have plenty of free bits in mmap_action and this is a little nicer than
> passing is_compat bools down the callchain.
>
> (that comment over compat_mmap is really... vague and bad, but I couldn't
> think of something else)
>
> --
> Pedro

Cheers, Lorenzo