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

[Lorenzo Stoakes]

On Mon, Jun 01, 2026 at 05:25:46PM +0200, David Hildenbrand (Arm) wrote:
> 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@xxxxxxxxxx>
> > ---
> >  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?

I thik it's what people tend to call this, it's what I've seen it called anyway!
I mean your German brain is being logical :) but by convention going with this
terminology.

>
> >  	 */
> > -	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?

I'm not sure if it's really necessary, but I don't want to break userspace here
by suddenly changing something that very plausible userspace might treat
differently from other error codes (-EAGAIN).

>
> 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?

I'd rather not, because I suspect there might be some other cases of this, and
it makes this option kinda horrible, as if it were a sane idea to convert errors
to -EAGAIN :P

Later on, once I've done more conversions, can obviously figure out a better way
or whether we can just remove it.

For now I want to eliminate these two horrible success/error hook hacks so we
can move ahead more sanely (and I can in particular figure out a less horrible
way of making the hugetlb 'vma lock' horror show work with mmap_prepare :)

>
> --
> Cheers,
>
> David

Cheers, Lorenzo