Skip to contentSkip to conversation

hugetlb MAP_PRIVATE mapping vs /dev/zero

BUG Oleg Nesterov 4 participants 8 messages Jul 1, 2004 – Jul 2, 2004

Subsystems (2)
Char and misc drivers HugeTLB
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

Hello.

Hugetlbfs mmap with MAP_PRIVATE becomes MAP_SHARED silently, but vma->vm_flags have no VM_SHARED bit. I think it make sense to forbid MAP_PRIVATE in hugetlbfs_file_mmap() because it may confuse user space applications. But the real bug is that reading from /dev/zero into hugetlb will do:

read_zero()

	read_zero_pagealigned()
		if (vma->vm_flags & VM_SHARED)
			break;	// OK if MAP_PRIVATE
		zap_page_range();
		zeromap_page_range();

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Oleg.

Signed-off-by Oleg Nesterov <[email protected]>
drivers/char/mem.c +1 -1
diff -urp 2.6.7-clean/drivers/char/mem.c 2.6.7-mmap/drivers/char/mem.c
@@ -417,7 +417,7 @@ static inline size_t read_zero_pagealign
 
 		if (vma->vm_start > addr || (vma->vm_flags & VM_WRITE) == 0)
 			goto out_up;
-		if (vma->vm_flags & VM_SHARED)
+		if (vma->vm_flags & (VM_SHARED | VM_HUGETLB))
 			break;
 		count = vma->vm_end - addr;
 		if (count > size)
fs/hugetlbfs/inode.c +4 -0
diff -urp 2.6.7-clean/fs/hugetlbfs/inode.c 2.6.7-mmap/fs/hugetlbfs/inode.c
@@ -28,6 +28,7 @@
 #include <linux/security.h>
 
 #include <asm/uaccess.h>
+#include <asm/mman.h>
 
 /* some random number */
 #define HUGETLBFS_MAGIC	0x958458f6

@@ -52,6 +53,9 @@ static int hugetlbfs_file_mmap(struct fi
 	loff_t len, vma_len;
 	int ret;
 
+	if (!(vma->vm_flags & VM_MAYSHARE))
+		return -EINVAL;
+
 	if (vma->vm_start & ~HPAGE_MASK)
 		return -EINVAL;
#1 lore raw
Quoted 7 lines · Oleg Nesterov

Hugetlbfs mmap with MAP_PRIVATE becomes MAP_SHARED silently, but vma->vm_flags have no VM_SHARED bit. I think it make sense to forbid MAP_PRIVATE in hugetlbfs_file_mmap() because it may confuse user space applications. But the real bug is that reading from /dev/zero into hugetlb will do:
read_zero()

Quoted 7 lines
	read_zero_pagealigned()
		if (vma->vm_flags & VM_SHARED)
			break;	// OK if MAP_PRIVATE
		zap_page_range();
		zeromap_page_range();

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Best to fix hugetlbfs_file_mmap().

-- wli

#2 lore raw
In reply to Oleg Nesterov
Quoted 9 lines · Oleg Nesterov

Hugetlbfs mmap with MAP_PRIVATE becomes MAP_SHARED silently, but vma->vm_flags have no VM_SHARED bit. I think it make sense to forbid MAP_PRIVATE in hugetlbfs_file_mmap() because it may confuse user space applications. But the real bug is that reading from /dev/zero into hugetlb will do:

read_zero()

Quoted 8 lines
	read_zero_pagealigned()
		if (vma->vm_flags & VM_SHARED)
			break;	// OK if MAP_PRIVATE
		zap_page_range();
		zeromap_page_range();

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

This could break existing applications, yes?

If so, would an appropriate fixup be to coerce private mappings of hugetlb files into shared ones in-kernel?

#3 lore raw
In reply to Oleg Nesterov
Quoted 10 lines · Oleg Nesterov

Hello.

Hugetlbfs mmap with MAP_PRIVATE becomes MAP_SHARED silently, but vma->vm_flags have no VM_SHARED bit. I think it make sense to forbid MAP_PRIVATE in hugetlbfs_file_mmap() because it may confuse user space applications. But the real bug is that reading from /dev/zero into hugetlb will do:

read_zero()

Quoted 8 lines
	read_zero_pagealigned()
		if (vma->vm_flags & VM_SHARED)
			break;	// OK if MAP_PRIVATE
		zap_page_range();
		zeromap_page_range();

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Err... surely we need to fix both, yes?

Signature · 4 lines
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson
#4 lore raw
Quoted 2 lines · Oleg Nesterov
Quoted 2 lines

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Quoted 1 line · David Gibson

Err... surely we need to fix both, yes?

No. /dev/zero is innocent. hugetlb is demanding VM_SHARED semantics without actually setting VM_SHARED. /dev/zero tripping over its nonstandard pagetable structure is not something to be dealt with in /dev/zero itself.

-- wli

#5 lore raw
Quoted 11 lines · William Lee Irwin
Quoted 2 lines · Oleg Nesterov
Quoted 2 lines

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Quoted 1 line · David Gibson

Err... surely we need to fix both, yes?

No. /dev/zero is innocent. hugetlb is demanding VM_SHARED semantics without actually setting VM_SHARED. /dev/zero tripping over its nonstandard pagetable structure is not something to be dealt with in /dev/zero itself.

Duh, sorry, misread the sense of the VM_SHARED test in the zeromap code.

Signature · 4 lines
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson
#6 lore raw
Quoted 8 lines · David Gibson
Quoted 7 lines · William Lee Irwin
Quoted 2 lines · Oleg Nesterov
Quoted 2 lines

We can fix hugetlbfs_file_mmap() or read_zero_pagealigned() or both.

Quoted 1 line · David Gibson

Err... surely we need to fix both, yes?

Quoted 7 lines
Quoted 4 lines

No. /dev/zero is innocent. hugetlb is demanding VM_SHARED semantics without actually setting VM_SHARED. /dev/zero tripping over its nonstandard pagetable structure is not something to be dealt with in /dev/zero itself.

Duh, sorry, misread the sense of the VM_SHARED test in the zeromap code.

On second thoughts, though, I think logically it should be fixed in both places. For now forcing VM_SHARED in the hugetlbfs code is sufficient, but if we ever allow (real) MAP_PRIVATE hugepage mappings (by implementing hugepage COW, for example), then the zeromap code will need fixing.

Conceptually it's not so much the fact that the hugepage memory is shared which is tripping up zeromap as the fact that it isn't mapped in the normal way.

Of course, one could argue that the whole zeromap idea is just too damn clever for its own good...

Signature · 4 lines
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson
#7 lore raw
Quoted 2 lines · David Gibson
Quoted 2 lines

Duh, sorry, misread the sense of the VM_SHARED test in the zeromap code.

Quoted 10 lines · David Gibson

On second thoughts, though, I think logically it should be fixed in both places. For now forcing VM_SHARED in the hugetlbfs code is sufficient, but if we ever allow (real) MAP_PRIVATE hugepage mappings (by implementing hugepage COW, for example), then the zeromap code will need fixing.
Conceptually it's not so much the fact that the hugepage memory is shared which is tripping up zeromap as the fact that it isn't mapped in the normal way.
Of course, one could argue that the whole zeromap idea is just too damn clever for its own good...

Better that there should be a zeromap_hugepage_range() than pollution of random pseudodrivers.

-- wli