On Tue, Sep 16, 2025 at 03:11:54PM +0100, Lorenzo Stoakes wrote:
> +/* What action should be taken after an .mmap_prepare call is complete? */
> +enum mmap_action_type {
> + MMAP_NOTHING, /* Mapping is complete, no further action. */
> + MMAP_REMAP_PFN, /* Remap PFN range. */
Seems like it would be a bit tider to include MMAP_IO_REMAP_PFN here
instead of having the is_io_remap bool.
> @@ -1155,15 +1155,18 @@ int __compat_vma_mmap_prepare(const struct
> file_operations *f_op,
> .vm_file = vma->vm_file,
> .vm_flags = vma->vm_flags,
> .page_prot = vma->vm_page_prot,
> +
> + .action.type = MMAP_NOTHING, /* Default */
> };
> int err;
>
> err = f_op->mmap_prepare(&desc);
> if (err)
> return err;
> - set_vma_from_desc(vma, &desc);
>
> - return 0;
> + mmap_action_prepare(&desc.action, &desc);
> + set_vma_from_desc(vma, &desc);
> + return mmap_action_complete(&desc.action, vma);
> }
> EXPORT_SYMBOL(__compat_vma_mmap_prepare);
A function called prepare that now calls complete has become a bit oddly named??
> +int mmap_action_complete(struct mmap_action *action,
> + struct vm_area_struct *vma)
> +{
> + int err = 0;
> +
> + switch (action->type) {
> + case MMAP_NOTHING:
> + break;
> + case MMAP_REMAP_PFN:
> + VM_WARN_ON_ONCE((vma->vm_flags & VM_REMAP_FLAGS) !=
> + VM_REMAP_FLAGS);
This is checked in remap_pfn_range_complete() IIRC? Probably not
needed here as well then.
> + if (action->remap.is_io_remap)
> + err = io_remap_pfn_range_complete(vma,
> action->remap.start,
> + action->remap.start_pfn, action->remap.size,
> + action->remap.pgprot);
> + else
> + err = remap_pfn_range_complete(vma, action->remap.start,
> + action->remap.start_pfn, action->remap.size,
> + action->remap.pgprot);
> + break;
> + }
> +
> + /*
> + * If an error occurs, unmap the VMA altogether and return an error. We
> + * only clear the newly allocated VMA, since this function is only
> + * invoked if we do NOT merge, so we only clean up the VMA we created.
> + */
> + if (err) {
> + const size_t len = vma_pages(vma) << PAGE_SHIFT;
> +
> + do_munmap(current->mm, vma->vm_start, len, NULL);
> +
> + if (action->error_hook) {
> + /* We may want to filter the error. */
> + err = action->error_hook(err);
> +
> + /* The caller should not clear the error. */
> + VM_WARN_ON_ONCE(!err);
> + }
> + return err;
> + }
> +
> + if (action->success_hook)
> + err = action->success_hook(vma);
> +
> + return err;
I would write this as
if (action->success_hook)
return action->success_hook(vma);
return 0;
Just for emphasis this is the success path.
> +int mmap_action_complete(struct mmap_action *action,
> + struct vm_area_struct *vma)
> +{
> + int err = 0;
> +
> + switch (action->type) {
> + case MMAP_NOTHING:
> + break;
> + case MMAP_REMAP_PFN:
> + WARN_ON_ONCE(1); /* nommu cannot handle this. */
> +
> + break;
> + }
> +
> + /*
> + * If an error occurs, unmap the VMA altogether and return an error. We
> + * only clear the newly allocated VMA, since this function is only
> + * invoked if we do NOT merge, so we only clean up the VMA we created.
> + */
> + if (err) {
> + const size_t len = vma_pages(vma) << PAGE_SHIFT;
> +
> + do_munmap(current->mm, vma->vm_start, len, NULL);
> +
> + if (action->error_hook) {
> + /* We may want to filter the error. */
> + err = action->error_hook(err);
> +
> + /* The caller should not clear the error. */
> + VM_WARN_ON_ONCE(!err);
> + }
> + return err;
> + }
err is never !0 here, so this should go to a later patch/series.
Also seems like this cleanup wants to be in a function that is not
protected by #ifdef nommu since the code is identical on both branches.
> + if (action->success_hook)
> + err = action->success_hook(vma);
> +
> + return 0;
return err, though prefer to match above, and probably this sequence
should be pulled into the same shared function as above too.
Jason
