2009-12-30 20:17:43

by Hugh Dickins

[permalink] [raw]
Subject: [PATCH] mm: move sys_mmap_pgoff from util.c

Move sys_mmap_pgoff() from mm/util.c to mm/mmap.c and mm/nommu.c,
where we'd expect to find such code: especially now that it contains
the MAP_HUGETLB handling. Revert mm/util.c to how it was in 2.6.32.

This patch just ignores MAP_HUGETLB in the nommu case, as in 2.6.32,
whereas 2.6.33-rc2 reported -ENOSYS. Perhaps validate_mmap_request()
should reject it with -EINVAL? Add that later if necessary.

Signed-off-by: Hugh Dickins <[email protected]>
---

mm/mmap.c | 40 ++++++++++++++++++++++++++++++++++++++++
mm/nommu.c | 25 +++++++++++++++++++++++++
mm/util.c | 44 --------------------------------------------
3 files changed, 65 insertions(+), 44 deletions(-)

--- 2.6.33-rc2/mm/mmap.c 2009-12-18 11:42:54.000000000 +0000
+++ linux/mm/mmap.c 2009-12-26 18:28:30.000000000 +0000
@@ -1043,6 +1043,46 @@ unsigned long do_mmap_pgoff(struct file
}
EXPORT_SYMBOL(do_mmap_pgoff);

+SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
+ unsigned long, prot, unsigned long, flags,
+ unsigned long, fd, unsigned long, pgoff)
+{
+ struct file *file = NULL;
+ unsigned long retval = -EBADF;
+
+ if (!(flags & MAP_ANONYMOUS)) {
+ if (unlikely(flags & MAP_HUGETLB))
+ return -EINVAL;
+ file = fget(fd);
+ if (!file)
+ goto out;
+ } else if (flags & MAP_HUGETLB) {
+ struct user_struct *user = NULL;
+ /*
+ * VM_NORESERVE is used because the reservations will be
+ * taken when vm_ops->mmap() is called
+ * A dummy user value is used because we are not locking
+ * memory so no accounting is necessary
+ */
+ len = ALIGN(len, huge_page_size(&default_hstate));
+ file = hugetlb_file_setup(HUGETLB_ANON_FILE, len, VM_NORESERVE,
+ &user, HUGETLB_ANONHUGE_INODE);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+ }
+
+ flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
+
+ down_write(&current->mm->mmap_sem);
+ retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+ up_write(&current->mm->mmap_sem);
+
+ if (file)
+ fput(file);
+out:
+ return retval;
+}
+
/*
* Some shared mappigns will want the pages marked read-only
* to track write events. If so, we'll downgrade vm_page_prot
--- 2.6.33-rc2/mm/nommu.c 2009-12-18 11:42:54.000000000 +0000
+++ linux/mm/nommu.c 2009-12-26 18:28:30.000000000 +0000
@@ -1398,6 +1398,31 @@ error_getting_region:
}
EXPORT_SYMBOL(do_mmap_pgoff);

+SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
+ unsigned long, prot, unsigned long, flags,
+ unsigned long, fd, unsigned long, pgoff)
+{
+ struct file *file = NULL;
+ unsigned long retval = -EBADF;
+
+ if (!(flags & MAP_ANONYMOUS)) {
+ file = fget(fd);
+ if (!file)
+ goto out;
+ }
+
+ flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
+
+ down_write(&current->mm->mmap_sem);
+ retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+ up_write(&current->mm->mmap_sem);
+
+ if (file)
+ fput(file);
+out:
+ return retval;
+}
+
/*
* split a vma into two pieces at address 'addr', a new vma is allocated either
* for the first part or the tail.
--- 2.6.33-rc2/mm/util.c 2009-12-18 11:42:55.000000000 +0000
+++ linux/mm/util.c 2009-12-26 18:28:30.000000000 +0000
@@ -4,10 +4,6 @@
#include <linux/module.h>
#include <linux/err.h>
#include <linux/sched.h>
-#include <linux/hugetlb.h>
-#include <linux/syscalls.h>
-#include <linux/mman.h>
-#include <linux/file.h>
#include <asm/uaccess.h>

#define CREATE_TRACE_POINTS
@@ -272,46 +268,6 @@ int __attribute__((weak)) get_user_pages
}
EXPORT_SYMBOL_GPL(get_user_pages_fast);

-SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
- unsigned long, prot, unsigned long, flags,
- unsigned long, fd, unsigned long, pgoff)
-{
- struct file * file = NULL;
- unsigned long retval = -EBADF;
-
- if (!(flags & MAP_ANONYMOUS)) {
- if (unlikely(flags & MAP_HUGETLB))
- return -EINVAL;
- file = fget(fd);
- if (!file)
- goto out;
- } else if (flags & MAP_HUGETLB) {
- struct user_struct *user = NULL;
- /*
- * VM_NORESERVE is used because the reservations will be
- * taken when vm_ops->mmap() is called
- * A dummy user value is used because we are not locking
- * memory so no accounting is necessary
- */
- len = ALIGN(len, huge_page_size(&default_hstate));
- file = hugetlb_file_setup(HUGETLB_ANON_FILE, len, VM_NORESERVE,
- &user, HUGETLB_ANONHUGE_INODE);
- if (IS_ERR(file))
- return PTR_ERR(file);
- }
-
- flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
-
- down_write(&current->mm->mmap_sem);
- retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
- up_write(&current->mm->mmap_sem);
-
- if (file)
- fput(file);
-out:
- return retval;
-}
-
/* Tracepoints definitions. */
EXPORT_TRACEPOINT_SYMBOL(kmalloc);
EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc);


2010-01-04 12:39:09

by Eric B Munson

[permalink] [raw]
Subject: Re: [PATCH] mm: move sys_mmap_pgoff from util.c

On Wed, 30 Dec 2009, Hugh Dickins wrote:

> Move sys_mmap_pgoff() from mm/util.c to mm/mmap.c and mm/nommu.c,
> where we'd expect to find such code: especially now that it contains
> the MAP_HUGETLB handling. Revert mm/util.c to how it was in 2.6.32.
>
> This patch just ignores MAP_HUGETLB in the nommu case, as in 2.6.32,
> whereas 2.6.33-rc2 reported -ENOSYS. Perhaps validate_mmap_request()
> should reject it with -EINVAL? Add that later if necessary.
>
> Signed-off-by: Hugh Dickins <[email protected]>

I think that -ENOSYS is the correcet response in the nommu case, but
I that can be added in a later patch.

Acked-by: Eric B Munson <[email protected]>


Attachments:
(No filename) (678.00 B)
signature.asc (197.00 B)
Digital signature
Download all attachments

2010-01-04 17:21:31

by Hugh Dickins

[permalink] [raw]
Subject: Re: [PATCH] mm: move sys_mmap_pgoff from util.c

On Mon, 4 Jan 2010, Eric B Munson wrote:
> On Wed, 30 Dec 2009, Hugh Dickins wrote:
>
> > Move sys_mmap_pgoff() from mm/util.c to mm/mmap.c and mm/nommu.c,
> > where we'd expect to find such code: especially now that it contains
> > the MAP_HUGETLB handling. Revert mm/util.c to how it was in 2.6.32.
> >
> > This patch just ignores MAP_HUGETLB in the nommu case, as in 2.6.32,
> > whereas 2.6.33-rc2 reported -ENOSYS. Perhaps validate_mmap_request()
> > should reject it with -EINVAL? Add that later if necessary.
> >
> > Signed-off-by: Hugh Dickins <[email protected]>
>
> I think that -ENOSYS is the correcet response in the nommu case, but
> I that can be added in a later patch.
>
> Acked-by: Eric B Munson <[email protected]>

Thanks. I had believed -ENOSYS was solely for unsupported system calls,
so thought it inappropriate here; but we seem to have quite a few places
which are using it indeed for "Function not implemented", and -EINVAL is
always so very overloaded that an alternative can be a lot more helpful.
Okay, I'll send a patch to give -ENOSYS for MAP_HUGETLB on nommu, which
will be consistent with mmu.

Hugh

2010-01-05 12:37:21

by Hugh Dickins

[permalink] [raw]
Subject: [PATCH] nommu: reject MAP_HUGETLB

We've agreed to restore the rejection of MAP_HUGETLB to nommu.
Mimic what happens with mmu when hugetlb is not configured in:
say -ENOSYS, but -EINVAL if MAP_ANONYMOUS was not given too.

Signed-off-by: Hugh Dickins <[email protected]>
---

mm/nommu.c | 8 ++++++++
1 file changed, 8 insertions(+)

--- 2.6.33-rc2-git/mm/nommu.c 2009-12-31 08:08:16.000000000 +0000
+++ linux/mm/nommu.c 2010-01-05 12:08:01.000000000 +0000
@@ -1405,6 +1405,14 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned lon
struct file *file = NULL;
unsigned long retval = -EBADF;

+ if (unlikely(flags & MAP_HUGETLB)) {
+ if (flags & MAP_ANONYMOUS)
+ retval = -ENOSYS; /* like hugetlb_file_setup */
+ else
+ retval = -EINVAL;
+ goto out;
+ }
+
if (!(flags & MAP_ANONYMOUS)) {
file = fget(fd);
if (!file)

2010-01-05 12:47:24

by Eric B Munson

[permalink] [raw]
Subject: Re: [PATCH] nommu: reject MAP_HUGETLB

On Tue, 05 Jan 2010, Hugh Dickins wrote:

> We've agreed to restore the rejection of MAP_HUGETLB to nommu.
> Mimic what happens with mmu when hugetlb is not configured in:
> say -ENOSYS, but -EINVAL if MAP_ANONYMOUS was not given too.
>
> Signed-off-by: Hugh Dickins <[email protected]>

Acked-by: Eric B Munson <[email protected]>


Attachments:
(No filename) (344.00 B)
signature.asc (197.00 B)
Digital signature
Download all attachments

2010-01-05 15:24:10

by David Howells

[permalink] [raw]
Subject: Re: [PATCH] nommu: reject MAP_HUGETLB

Hugh Dickins <[email protected]> wrote:

> We've agreed to restore the rejection of MAP_HUGETLB to nommu.
> Mimic what happens with mmu when hugetlb is not configured in:
> say -ENOSYS, but -EINVAL if MAP_ANONYMOUS was not given too.

On the other hand, why not just ignore the MAP_HUGETLB flag on NOMMU?

David

2010-01-05 16:16:19

by Hugh Dickins

[permalink] [raw]
Subject: Re: [PATCH] nommu: reject MAP_HUGETLB

On Tue, 5 Jan 2010, David Howells wrote:
> Hugh Dickins <[email protected]> wrote:
>
> > We've agreed to restore the rejection of MAP_HUGETLB to nommu.
> > Mimic what happens with mmu when hugetlb is not configured in:
> > say -ENOSYS, but -EINVAL if MAP_ANONYMOUS was not given too.
>
> On the other hand, why not just ignore the MAP_HUGETLB flag on NOMMU?

I don't care very much either way: originally it was ignored,
then it became an -ENOSYS when Al moved the MAP_HUGETLB handling
into util.c, then it was ignored again when I moved that back into
mmap.c and nommu.c, now this patch makes it -ENOSYS on nommu again
- which Eric preferred.

I'd say this patch is _correct_; but I'm perfectly happy to have
you NAK it, or Linus ignore it, with the observation that nommu is
more likely to want to cut bloat than to be pedantically correct -
pedantic because I'd expect the nommu mmap() to work fine with the
MAP_HUGETLB flag there, just wouldn't be using any huge pages.

Okay with me whichever way it goes.

Hugh