Re: [PATCH v4 17/20] mm/vma: only permit MAP_PRIVATE /dev/zero to be mapped anonymous

On 8/6/26 22:21, Lorenzo Stoakes (ARM) wrote:
> In order to use mmap_prepare() with MAP_PRIVATE mappings of /dev/zero
> without the success_hook hack we explicitly permitted mmap_prepare handlers
> to set NULL vm_ops.

The sentence is a bit hard to get as you are mixing "with" with another "without".

> 
> However this is dangerous and we really only want to allow this for
> MAP_PRIVATE-mapped /dev/zero.
> 
> Make it possible to explicitly identify /dev/zero by setting a global
> DEVZERO_MINOR device minor number then explicitly check for this in mmap
> code for a MAP_PRIVATE mapping and only set the VMA anonymous if we have
> positively identified it.
> 
> Then remove all ability for mmap_prepare or mmap hooks to set a VMA
> anonymous and update mmap_zero_prepare() to leave it to the core mmap code
> to mark the VMA anonymous.
> 
> Note that this disallows nested MAP_PRIVATE-mappings of /dev/zero
> regions. Doing this would be broken in any case.

What exactly do you mean by "nested MAP_PRIVATE mappings"? You mean, reusing
parts in other drives?

Do you mean things like ...

[...]

> 
> An example of this is drm_gem_shmem_mmap() which deliberately clears
> vma->vm_ops before handing the VMA to dma-buf. Cases such as this will be
> updated when they are converted to mmap_prepare.

... this?

> 
> Also, in order to avoid a single commit bisection hazard, add a temporary
> workaround to set the VMA anonymous only after vma->vm_file is assigned in
> __mmap_new_file_vma().
> 
> This is because vma_set_range() calls vma_set_pgoff() and
> assert_sane_pgoff() in turn, prior to the vma->vm_file being assigned. If
> we set the VMA anonymous early then this assert will fail.
> 
> This is removed in the subsequent commit.
> 
> Also update the VMA userland tests to reflect the change.
> 
> Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
> ---

[...]

>  
> +static bool map_is_dev_zero(const struct mmap_state *map)
> +{
> +	const struct file *file = map->file;
> +	struct inode *inode;
> +
> +	if (!file)
> +		return false;
> +	inode = file_inode(file);
> +	if (!S_ISCHR(inode->i_mode))
> +		return false;
> +	return imajor(inode) == MEM_MAJOR && iminor(inode) == DEVZERO_MINOR;
> +}

My brain is a bit slow after digging through this series.

We identify shmem, for example, through shmem_vm_ops/shmem_anon_vm_ops.

So naturally I am wondering: couldn't we do something similar to identify that?
Like, checking for zero_fops?

> +
> +static bool map_is_private(const struct mmap_state *map)
> +{
> +	return !vma_flags_test(&map->vma_flags, VMA_SHARED_BIT);

Can't we use the is_cow_mapping() helper instead somehow?




-- 
Cheers,

David