Re: [PATCH 3/6] mm/vma: only permit MAP_PRIVATE /dev/zero to be mapped anonymous

[David Hildenbrand (Arm)]

>> I was wondering whether we should call this "map_is_private_anon", due to
>> MAP_ANON|MAP_SHARED. But looking at __mmap_new_vma(), the existing "is_anon" is
>> also limited to MAP_ANON|MAP_PRIVATE.
> 
> A 'shared anon' mapping is not anon at all, and that's handled early in
> do_mmap().
> 
> I wish that we didn't confuse people by allowing MAP_SHARED | MAP_ANON as a
> shorthand but there we are.
> 
> So I don't like to make that distinction on the basis that a 'shared
> anon' mapping isn't something that exists :)
> 
> And then imagine vma_is_private_anonymous() vs. vma_is_anonymous() etc. It would
> get silly, quick...
> 

Yeah, agreed.

>>
>>> +{
>>> +	if (!map_is_private(map))
>>> +		return false;
>>> +
>>> +	return !map->file || file_is_dev_zero(map->file);
>>> +}
>>> +
>>>  /*
>>>   * __mmap_new_vma() - Allocate a new VMA for the region, as merging was not
>>>   * possible.
>>> @@ -2634,8 +2647,7 @@ static int __mmap_new_file_vma(struct mmap_state *map,
>>>  static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>>>  	struct mmap_action *action)
>>>  {
>>> -	const bool is_anon = !map->file &&
>>> -		!vma_flags_test(&map->vma_flags, VMA_SHARED_BIT);
>>> +	const bool is_anon = map_is_anon(map);
>>>  	struct vma_iterator *vmi = map->vmi;
>>>  	int error = 0;
>>>  	struct vm_area_struct *vma;
>>> @@ -2651,7 +2663,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>>>
>>>  	vma_iter_config(vmi, map->addr, map->end);
>>>
>>> -	if (is_anon)
>>> +	if (is_anon && !map->file)
>>>  		vma_set_anonymous(vma);
>>>
>>>  	vma_set_range(vma, map->addr, map->end, map->pgoff, map->anon_pgoff);
>>> @@ -2669,6 +2681,10 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>>>  	else if (!is_anon)
>>>  		error = shmem_zero_setup(vma);
>>>
>>> +	/* Temporary MAP_PRIVATE-/dev/zero workaround. */
>>> +	if (is_anon && map->file)
>>> +		vma_set_anonymous(vma);
>>> +
>>>  	if (error)
>>>  		goto free_iter_vma;
>>>
>>> @@ -2777,6 +2793,10 @@ static int call_mmap_prepare(struct mmap_state *map,
>>>  	if (err)
>>>  		return err;
>>>
>>> +	/* Hooks cannot mark themselves anonymous. */
>>
>> I guess this comment will be stale soon (after #4 where you drop the
>> set_anonymous part).
> 
> Not really, it's there to catch drivers doing something silly/broken (likely by
> mistake).
> 
> I want to catch that early. I have a 36 patch series that extends this kind of
> idea... a lot :)
> 
>>
>> Should it be
>>
>> "vm_ops are strictly required with mmap_prepare"
>>
>> or sth like that?
> 
> Well that's confusing though, because desc->vm_ops defaults to &dummy_vma_ops,
> and we absolutely do not require drivers to set vm_ops at all.
> 
> And as far as the driver is concerned maybe it's NULL? They maybe don't realise
> :)
> 
> So the idea is to say don't allow them to try to do something they can't do.

Yes, but my point is that the comment

"cannot mark themselves anonymous"

will not really be correct after the next patch, no?

> 
>>
>>> +	if (!desc->vm_ops)
>>> +		return -EINVAL;
>>> +
>>>  	err = call_action_prepare(map, desc);
>>>  	if (err)
>>>  		return err;
>>> @@ -2799,10 +2819,7 @@ static int call_mmap_prepare(struct mmap_state *map,
>>>  static void set_vma_user_defined_fields(struct vm_area_struct *vma,
>>>  		struct mmap_state *map)
>>>  {
>>> -	if (map->vm_ops)
>>> -		vma->vm_ops = map->vm_ops;
>>> -	else	/* Only /dev/zero should do this. */
>>> -		vma_set_anonymous(vma);
>>> +	vma->vm_ops = map->vm_ops;
>>>  	vma->vm_private_data = map->vm_private_data;
>>>  }
>>>
>>> @@ -2882,7 +2899,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>>>  		allocated_new = true;
>>>  	}
>>>
>>> -	if (have_mmap_prepare)
>>> +	if (have_mmap_prepare && !map_is_anon(&map))
>>>  		set_vma_user_defined_fields(vma, &map);
>>
>> Ah, we have mmap_zero_prepare() for handling the shmem_zero_setup_desc(). I was
>> just about to ask whether we can just get rid of this here.
>>
>>
>> But, hold on, do we now even need that? Could core-mm now take care of that as
>> well, and we could just remove mmap_zero_prepare() entirely?
>>
>> That is, we'd make shmem_zero_setup() in __mmap_new_vma() take care of this?
>> Then we might not even need shmem_zero_setup_desc() anymore.
>>
>> Maybe harder than it sounds at first.
> 
> I think I'd rather that be a follow up :) this series is about eliminiating the
> one last (I hope?) corner case for anon VMAs.
Right; having to deal with anonymous mappings that have mmap_prepare is rather
suboptimal. Ideally we'd just handle the odd dev-zero special-casing early in
the mmap path also for MAP_SHARED, and avoid messing with mmap_prepare entirely.

So agreed that this can be done separately.

-- 
Cheers,

David