Re: [PATCH] /dev/zero: make private mapping full anonymous mapping

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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

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.

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

>
> Signed-off-by: Yang Shi <yang@os.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;

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've not checked the function but do we mark this as a special mapping of
some kind?

> +
>  	return 0;
>  }
>
> --
> 2.47.0
>