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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65691afc-615a-4716-8a2e-1f43bc65111c@os.amperecomputing.com>
Date: Tue, 14 Jan 2025 08:53:01 -0800
From: Yang Shi <yang@...amperecomputing.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.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 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.

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

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

>
> I've not checked the function but do we mark this as a special mapping of
> some kind?
>
>> +
>>   	return 0;
>>   }
>>
>> --
>> 2.47.0
>>