Re: [PATCH v2 3/3] mm/vma: eliminate mmap_action->error_hook, introduce error_filter

["David Hildenbrand (Arm)" <david@kernel.org>]

On 5/22/26 18:00, Lorenzo Stoakes wrote:
> Rather than providing a hook, simplify things by providing the ability to
> filter errors. This allows us to more carefully validate the value provided
> and thus ensure only a valid error code is specified, and simplifies the
> interface.
> 
> This way, we eliminate all hooks but mmap_prepare and allow only mmap
> actions to be specified (which core mm controls).
> 
> This significantly improves robustness and eliminates any unnecessary code
> duplication in driver mmap hooks.
> 
> We also update the /dev/mem logic (the only user) to use
> mmap_action->error_filter instead.
> 
> Signed-off-by: Lorenzo Stoakes <ljs@kernel.org>
> ---
>  drivers/char/mem.c              |  8 +-------
>  include/linux/mm_types.h        |  9 +++------
>  mm/util.c                       | 29 +++++++++++++++++++++--------
>  tools/testing/vma/include/dup.h |  9 +++------
>  4 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index a4297eb39887..11639d988e47 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -322,11 +322,6 @@ static const struct vm_operations_struct mmap_mem_ops = {
>  #endif
>  };
>  
> -static int mmap_filter_error(int err)
> -{
> -	return -EAGAIN;
> -}
> -
>  static int mmap_mem_prepare(struct vm_area_desc *desc)
>  {
>  	struct file *file = desc->file;
> @@ -362,8 +357,7 @@ static int mmap_mem_prepare(struct vm_area_desc *desc)
>  
>  	/* Remap-pfn-range will mark the range with the I/O flag. */
>  	mmap_action_remap_full(desc, desc->pgoff);
> -	/* We filter remap errors to -EAGAIN. */
> -	desc->action.error_hook = mmap_filter_error;
> +	desc->action.error_filter = -EAGAIN;
>  
>  	return 0;
>  }
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 945c0a5386d6..8d1fb85e7684 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -844,13 +844,10 @@ struct mmap_action {
>  	enum mmap_action_type type;
>  
>  	/*
> -	 * If specified, this hook is invoked when an error occurred when
> -	 * attempting the selected action.
> -	 *
> -	 * The hook can return an error code in order to filter the error, but
> -	 * it is not valid to clear the error here.
> +	 * If non-zero, filter errors that arise from mmap actions such that we
> +	 * return error_filter instead. Only valid error codes may be specified.

Is that really a filter or rather an "error conversion" / "error override".

"Filter" to my German brain implies that we would ... filter selected error
codes, not convert them to something else?

>  	 */
> -	int (*error_hook)(int err);
> +	int error_filter;
>  
>  	/*
>  	 * This should be set in rare instances where the operation required
> diff --git a/mm/util.c b/mm/util.c
> index 4e172990afcd..9b4e5432d45a 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1414,16 +1414,22 @@ static int mmap_action_finish(struct vm_area_struct *vma,
>  	 */
>  	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;
> +
> +	return action->error_filter ?: err;
>  }

Out of interest, why does dev/mem require this monstrosity?

If it's really a dev/mem specialty, you could just make it less generic and call
the property

"bool eagain_on_error;"

because surely, we don't want any more such monstrosity?

-- 
Cheers,

David