Message-ID: <10f5cb31-e9fa-4574-b36d-0e685fec5cda@lucifer.local>
Date: Tue, 14 Jan 2025 18:19:32 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Yang Shi <yang@...amperecomputing.com>
Cc: arnd@...db.de, gregkh@...uxfoundation.org, Liam.Howlett@...cle.com,
        vbabka@...e.cz, jannh@...gle.com, willy@...radead.org,
        liushixin2@...wei.com, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] /dev/zero: make private mapping full anonymous mapping

On Tue, Jan 14, 2025 at 06:14:57PM +0000, Lorenzo Stoakes wrote:
> This is getting into realms of discussion so to risk sounding rude - to be
> clear - NACK.
>
> The user-visible change to /proc/$pid/[s]maps kills this patch dead. This
> is regardless of any other discussed issue.
>
> But more importantly, I hadn't realise mmap_zero() was on the .mmap()
> callback (sorry my mistake) - you're simply not permitted to change
> vm_pgoff and vm_file fields here, the mapping logic doesn't expect it, and
> it's broken.

I see shmem_zero_page() does change vma->vm_page, this is broken... ugh. I
will audit this code (and have a look through _all_ mmap() callbacks I
guess). Duly added to TODO. But definitely can't have _another_ case of
doing this.

>
> To me the alternative would be to have a custom fault handler that hands
> back the zero page, but I"m not sure that's workable, you'd have to install
> a special mapping etc. and huge pages are weird and...
>
> I do appreciate you raising this especially as I was blissfully unaware,
> but I don't see how this patch can possibly work, sorry :(
>
> On Tue, Jan 14, 2025 at 08:53:01AM -0800, Yang Shi wrote:
> >
> >
> >
> > On 1/14/25 4:05 AM, Lorenzo Stoakes wrote:
> > > + Willy for the fs/weirdness elements of this.
> > >
> > > On Mon, Jan 13, 2025 at 02:30:33PM -0800, Yang Shi wrote:
> > > > When creating private mapping for /dev/zero, the driver makes it an
> > > > anonymous mapping by calling set_vma_anonymous().  But it just sets
> > > > vm_ops to NULL, vm_file is still valid and vm_pgoff is also file offset.
> > > Hm yikes.
> > >
> > > > This is a special case and the VMA doesn't look like either anonymous VMA
> > > > or file VMA.  It confused other kernel subsystem, for example, khugepaged [1].
> > > >
> > > > It seems pointless to keep such special case.  Making private /dev/zero
> > > > mapping a full anonymous mapping doesn't change the semantic of
> > > > /dev/zero either.
> > > My concern is that ostensibly there _is_ a file right? Are we certain that by
> > > not setting this we are not breaking something somewhere else?
> > >
> > > Are we not creating a sort of other type of 'non-such-beast' here?
> >
> > But the file is /dev/zero. I don't see this could break the semantic of
> > /dev/zero. The shared mapping of /dev/zero is not affected by this change,
> > kernel already treated private mapping of /dev/zero as anonymous mapping,
> > but with some weird settings in VMA. When reading the mapping, it returns 0
> > with zero page, when writing the mapping, a new anonymous folio is
> > allocated.
>
> You're creating a new concept of an anon but not anon but also now with
> anon vm_pgoff and missing vm_file even though it does reference a file
> and... yeah.
>
> This is not usual :)
>
> >
> > >
> > > I mean already setting it anon and setting vm_file non-NULL is really strange.
> > >
> > > > The user visible effect is the mapping entry shown in /proc/<PID>/smaps
> > > > and /proc/<PID>/maps.
> > > >
> > > > Before the change:
> > > > ffffb7190000-ffffb7590000 rw-p 00001000 00:06 8                          /dev/zero
> > > >
> > > > After the change:
> > > > ffffb6130000-ffffb6530000 rw-p 00000000 00:00 0
> > > >
> > > Yeah this seems like it might break somebody to be honest, it's really
> > > really really strange to map a file then for it not to be mapped.
> >
> > Yes, it is possible if someone really care whether the anonymous-like
> > mapping is mapped by /dev/zero or just created by malloc(). But I don't know
> > who really do...
> >
> > >
> > > But it's possibly EVEN WEIRDER to map a file and for it to seem mapped as a
> > > file but for it to be marked anonymous.
> > >
> > > God what a mess.
> > >
> > > > [1]: https://lore.kernel.org/linux-mm/20250111034511.2223353-1-liushixin2@huawei.com/
> > > I kind of hate that we have to mitigate like this for a case that should
> > > never ever happen so I'm inclined towards your solution but a lot more
> > > inclined towards us totally rethinking this.
> > >
> > > Do we _have_ to make this anonymous?? Why can't we just reference the zero
> > > page as if it were in the page cache (Willy - feel free to correct naive
> > > misapprehension here).
> >
> > TBH, I don't see why page cache has to be involved. When reading, 0 is
> > returned by zero page. When writing a CoW is triggered if page cache is
> > involved, but the content of the page cache should be just 0, so we copy 0
> > to the new folio then write to it. It doesn't make too much sense. I think
> > this is why private /dev/zero mapping is treated as anonymous mapping in the
> > first place.
>
> I'm obviously not suggesting allocating a bunch of extra folios, I was
> thinking there would be some means of handing back the actual zero
> page. But I am not sure this is workable.
>
> >
> > >
> > > > Signed-off-by: Yang Shi <yang@...amperecomputing.com>
> > > > ---
> > > >   drivers/char/mem.c | 4 ++++
> > > >   1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > > > index 169eed162a7f..dae113f7fc1b 100644
> > > > --- a/drivers/char/mem.c
> > > > +++ b/drivers/char/mem.c
> > > > @@ -527,6 +527,10 @@ static int mmap_zero(struct file *file, struct vm_area_struct *vma)
> > > >   	if (vma->vm_flags & VM_SHARED)
> > > >   		return shmem_zero_setup(vma);
> > > >   	vma_set_anonymous(vma);
> > > > +	fput(vma->vm_file);
> > > > +	vma->vm_file = NULL;
> > > > +	vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
>
> This is just not permitted. We maintain mmap state which contains the file
> and pgoff state which gets threaded through the mapping operation, and
> simply do not expect you to change these fields.
>
> In future we will assert on this or preferably, restrict users to only
> changing VMA flags, the private field and vm_ops.
>
> > > Hmm, this might have been mremap()'d _potentially_ though? And then now
> > > this will be wrong? But then we'd have no way of tracking it correctly...
> >
> > I'm not quite familiar with the subtle details and corner cases of
> > meremap(). But mmap_zero() should be called by mmap(), so the VMA has not
> > been visible to user yet at this point IIUC. How come mremap() could move
> > it?
>
> Ah OK, in that case fine on that front.
>
> But you are not permitted to touch this field (we need to enforce this...)
>
> >
> > >
> > > I've not checked the function but do we mark this as a special mapping of
> > > some kind?
> > >
> > > > +
> > > >   	return 0;
> > > >   }
> > > >
> > > > --
> > > > 2.47.0
> > > >
> >

Powered by blists - more mailing lists