[BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[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()
	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 <oleg@tv-sign.ru>

diff -urp 2.6.7-clean/drivers/char/mem.c 2.6.7-mmap/drivers/char/mem.c
--- 2.6.7-clean/drivers/char/mem.c	2004-05-30 13:25:49.000000000 +0400
+++ 2.6.7-mmap/drivers/char/mem.c	2004-07-01 19:51:52.000000000 +0400
@@ -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)
diff -urp 2.6.7-clean/fs/hugetlbfs/inode.c 2.6.7-mmap/fs/hugetlbfs/inode.c
--- 2.6.7-clean/fs/hugetlbfs/inode.c	2004-05-24 14:16:11.000000000 +0400
+++ 2.6.7-mmap/fs/hugetlbfs/inode.c	2004-07-01 19:54:16.000000000 +0400
@@ -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;

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[William Lee Irwin III]

On Thu, Jul 01, 2004 at 08:29:18PM +0400, Oleg Nesterov wrote:
> 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.

Best to fix hugetlbfs_file_mmap().


-- wli

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[Andrew Morton]

Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> 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.

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?

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[David Gibson]

On Thu, Jul 01, 2004 at 08:29:18PM +0400, Oleg Nesterov wrote:
> 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.

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

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

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[William Lee Irwin III]

On Thu, Jul 01, 2004 at 08:29:18PM +0400, Oleg Nesterov wrote:
>> We can fix hugetlbfs_file_mmap() or read_zero_pagealigned()
>> or both.

On Fri, Jul 02, 2004 at 11:20:12AM +1000, David Gibson wrote:
> 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

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[David Gibson]

On Thu, Jul 01, 2004 at 07:44:22PM -0700, William Lee Irwin wrote:
> On Thu, Jul 01, 2004 at 08:29:18PM +0400, Oleg Nesterov wrote:
> >> We can fix hugetlbfs_file_mmap() or read_zero_pagealigned()
> >> or both.
> 
> On Fri, Jul 02, 2004 at 11:20:12AM +1000, David Gibson wrote:
> > 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.

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

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[David Gibson]

On Fri, Jul 02, 2004 at 01:49:37PM +1000, David Gibson wrote:
> On Thu, Jul 01, 2004 at 07:44:22PM -0700, William Lee Irwin wrote:
> > On Thu, Jul 01, 2004 at 08:29:18PM +0400, Oleg Nesterov wrote:
> > >> We can fix hugetlbfs_file_mmap() or read_zero_pagealigned()
> > >> or both.
> > 
> > On Fri, Jul 02, 2004 at 11:20:12AM +1000, David Gibson wrote:
> > > 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.

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

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

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[William Lee Irwin III]

On Fri, Jul 02, 2004 at 01:49:37PM +1000, David Gibson wrote:
>> Duh, sorry, misread the sense of the VM_SHARED test in the zeromap
>> code.

On Fri, Jul 02, 2004 at 02:12:15PM +1000, David Gibson wrote:
> 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

Re: [BUG] hugetlb MAP_PRIVATE mapping vs /dev/zero

[Oleg Nesterov]

Andrew Morton wrote:
> > We can fix hugetlbfs_file_mmap() or read_zero_pagealigned()
> > or both.
>
> This could break existing applications, yes?

Personally, i think that mmap(PROT_WRITE, MAP_PRIVATE, hugetlbfd)
should fail, just because kernel currently does not support this.
Well written application can break, beacuse it succeeds.

It is also security bug. Current behavior allows:
open("hugetlbfs_file_owned_by_root_with_mode:-rwxr-xr-x", O_RDONLY),
and then mmap(PROT_READ|PROT_WRITE, MAP_PRIVATE).

Perhaps, hugetlbfs_file_mmap() must do:
if (!(vma->vm_flags & VM_MAYSHARE) && (vma->vm_flags & VM_WRITE))
	return -EINVAL;

sys_mprotect() does not work with is_vm_hugetlb_page(vma), so it
seems to me, it is ok to allow private readonly hugetlb mappings.

David Gibson wrote:
> 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.

I completely agree.

Oleg.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

diff -urp 2.6.7-clean/Documentation/vm/hugetlbpage.txt 2.6.7-mmap/Documentation/vm/hugetlbpage.txt
--- 2.6.7-clean/Documentation/vm/hugetlbpage.txt	2004-05-24 14:15:57.000000000 +0400
+++ 2.6.7-mmap/Documentation/vm/hugetlbpage.txt	2004-07-02 15:59:31.000000000 +0400
@@ -13,6 +13,8 @@ available.
 Users can use the huge page support in Linux kernel by either using the mmap
 system call or standard SYSv shared memory system calls (shmget, shmat).
 
+NOTE: private writable mappings are not supported.
+
 First the Linux kernel needs to be built with CONFIG_HUGETLB_PAGE (present
 under Processor types and feature)  and CONFIG_HUGETLBFS (present under file
 system option on config menu) config options.
diff -urp 2.6.7-clean/drivers/char/mem.c 2.6.7-mmap/drivers/char/mem.c
--- 2.6.7-clean/drivers/char/mem.c	2004-05-30 13:25:49.000000000 +0400
+++ 2.6.7-mmap/drivers/char/mem.c	2004-07-01 20:07:42.000000000 +0400
@@ -417,8 +417,9 @@ 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;
+printk(KERN_CRIT "HERE!!!\n");
 		count = vma->vm_end - addr;
 		if (count > size)
 			count = size;
Only in 2.6.7-mmap/drivers/char: mem.c~
diff -urp 2.6.7-clean/fs/hugetlbfs/inode.c 2.6.7-mmap/fs/hugetlbfs/inode.c
--- 2.6.7-clean/fs/hugetlbfs/inode.c	2004-05-24 14:16:11.000000000 +0400
+++ 2.6.7-mmap/fs/hugetlbfs/inode.c	2004-07-02 15:40:41.000000000 +0400
@@ -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 | VM_WRITE)) == VM_WRITE)
+		return -EINVAL;
+
 	if (vma->vm_start & ~HPAGE_MASK)
 		return -EINVAL;