Message-ID: <1d78a0f4-5057-4c68-94d0-6e07cedf3ae7@lucifer.local>
Date: Tue, 16 Sep 2025 18:57:56 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>, Matthew Wilcox <willy@...radead.org>,
        Guo Ren <guoren@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        "David S . Miller" <davem@...emloft.net>,
        Andreas Larsson <andreas@...sler.com>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dave Jiang <dave.jiang@...el.com>, Nicolas Pitre <nico@...xnic.net>,
        Muchun Song <muchun.song@...ux.dev>,
        Oscar Salvador <osalvador@...e.de>,
        David Hildenbrand <david@...hat.com>,
        Konstantin Komarov <almaz.alexandrovich@...agon-software.com>,
        Baoquan He <bhe@...hat.com>, Vivek Goyal <vgoyal@...hat.com>,
        Dave Young <dyoung@...hat.com>, Tony Luck <tony.luck@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Dave Martin <Dave.Martin@....com>, James Morse <james.morse@....com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Hugh Dickins <hughd@...gle.com>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Uladzislau Rezki <urezki@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-csky@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-s390@...r.kernel.org, sparclinux@...r.kernel.org,
        nvdimm@...ts.linux.dev, linux-cxl@...r.kernel.org, linux-mm@...ck.org,
        ntfs3@...ts.linux.dev, kexec@...ts.infradead.org,
        kasan-dev@...glegroups.com, iommu@...ts.linux.dev,
        Kevin Tian <kevin.tian@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH v3 08/13] mm: add ability to take further action in
 vm_area_desc

On Tue, Sep 16, 2025 at 02:28:36PM -0300, Jason Gunthorpe wrote:
> 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.

Well, I did start with this, but felt simpler to treat it as a remap, and also
semantically, as it's more-or-less a remap with maybe different PFN...

But you know, thinking about it, yeah that's probably nicer, will change.

Often with these things you are back and forth on 'hmm maybe this maybe that' :)

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

That's a very good point... :) I mean it's kinda right in a way because it is a
compatibility layer for mmap_prepare for stacked filesystems etc. that can only
(for now) call .mmap() and are confronted with an underlying thing that has
.mmap_prepare, but also it's confusing now.

Will rename.

>
> > +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.

Ah ok will drop 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.

Ack. That is nicer actually.

>
> > +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.

Right yeah. Doh! Will drop.

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

Not sure which cleanup you mean, this is new code :)

I don't at all like functions that if #ifdef nommu embedded in them.

And I frankly resent that we support nommu so I'm not inclined to share code
between that and code for arches that people actually use.

Anyway, we can probably simplify this quite a bit.

	WARN_ON_ONCE(action->type != MMAP_NOTHING);
	return 0;

>
> > +	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.

Yeah I mean, you're not going to make me actually have to ack nommu properly are
you?..

I suppose we could be tasteful and have a separate 'handle hooks' function or
something here or something.

Let me put my bias aside and take a look at that.

>
> Jason

Cheers, Lorenzo

Powered by blists - more mailing lists