Make vma_is_anonymous() reliable

PATCH Superseded Kirill A. Shutemov 9 participants 21 messages Jul 13, 2015 – Jul 27, 2015

Subsystems (22)

DRM drivers Memory management Media (V4L/DVB) x86 +18 more subsystems

lore

Share & tools

lore

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #0 lore raw

cover Make vma_is_anonymous() reliable

We rely on ->vm_ops == NULL to detect anonymous VMA but this check is not always reliable:

- MPX sets ->vm_ops on anonymous VMA;

- many drivers don't set ->vm_ops. See for instance hpet_mmap().

This patchset makes vma_is_anonymous() more reliable and makes few cleanups around the code.

Kirill A. Shutemov (5):
mm: mark most vm_operations_struct const
x86, mpx: do not set ->vm_ops on mpx VMAs
mm: make sure all file VMAs have ->vm_ops set
mm, madvise: use vma_is_anonymous() to check for anon VMA mm, memcontrol: use vma_is_anonymous() to check for anon VMA

 arch/x86/kernel/vsyscall_64.c                  |  2 +-
 arch/x86/mm/mmap.c                             |  7 +++++++
 arch/x86/mm/mpx.c                              | 20 +-------------------
 drivers/android/binder.c                       |  2 +-
 drivers/gpu/drm/vgem/vgem_drv.c                |  2 +-
 drivers/hsi/clients/cmt_speech.c               |  2 +-
 drivers/infiniband/hw/qib/qib_file_ops.c       |  2 +-
 drivers/infiniband/hw/qib/qib_mmap.c           |  2 +-
 drivers/media/platform/omap/omap_vout.c        |  2 +-
 drivers/misc/genwqe/card_dev.c                 |  2 +-
 drivers/staging/android/ion/ion.c              |  2 +-
 drivers/staging/comedi/comedi_fops.c           |  2 +-
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c |  2 +-
 drivers/xen/gntalloc.c                         |  2 +-
 drivers/xen/gntdev.c                           |  2 +-
 drivers/xen/privcmd.c                          |  4 ++--
 fs/ceph/addr.c                                 |  2 +-
 fs/cifs/file.c                                 |  2 +-
 mm/madvise.c                                   |  2 +-
 mm/memcontrol.c                                |  2 +-
 mm/mmap.c                                      |  8 ++++++++
 security/selinux/selinuxfs.c                   |  2 +-
 22 files changed, 36 insertions(+), 39 deletions(-)

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #1 lore raw

PATCH 1/5 mm: mark most vm_operations_struct const

With two excetions (drm/qxl and drm/radeon) all vm_operations_struct structs should be constant.

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 arch/x86/kernel/vsyscall_64.c                  | 2 +-
 drivers/android/binder.c                       | 2 +-
 drivers/gpu/drm/vgem/vgem_drv.c                | 2 +-
 drivers/hsi/clients/cmt_speech.c               | 2 +-
 drivers/infiniband/hw/qib/qib_file_ops.c       | 2 +-
 drivers/infiniband/hw/qib/qib_mmap.c           | 2 +-
 drivers/media/platform/omap/omap_vout.c        | 2 +-
 drivers/misc/genwqe/card_dev.c                 | 2 +-
 drivers/staging/android/ion/ion.c              | 2 +-
 drivers/staging/comedi/comedi_fops.c           | 2 +-
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 2 +-
 drivers/xen/gntalloc.c                         | 2 +-
 drivers/xen/gntdev.c                           | 2 +-
 drivers/xen/privcmd.c                          | 4 ++--
 fs/ceph/addr.c                                 | 2 +-
 fs/cifs/file.c                                 | 2 +-
 security/selinux/selinuxfs.c                   | 2 +-
 17 files changed, 18 insertions(+), 18 deletions(-)

arch/x86/kernel/vsyscall_64.c +1 -1

index 2dcc6ff6fdcc..30f3a83291ad 100644
@@ -277,7 +277,7 @@ static const char *gate_vma_name(struct vm_area_struct *vma)
 {
 	return "[vsyscall]";
 }
-static struct vm_operations_struct gate_vma_ops = {
+static const struct vm_operations_struct gate_vma_ops = {
 	.name = gate_vma_name,
 };
 static struct vm_area_struct gate_vma = {

drivers/android/binder.c +1 -1

index 6607f3c6ace1..a39e85f9efa9 100644
@@ -2834,7 +2834,7 @@ static int binder_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
-static struct vm_operations_struct binder_vm_ops = {
+static const struct vm_operations_struct binder_vm_ops = {
 	.open = binder_vma_open,
 	.close = binder_vma_close,
 	.fault = binder_vm_fault,

drivers/gpu/drm/vgem/vgem_drv.c +1 -1

index 7a207ca547be..27c2c473d9d3 100644
@@ -125,7 +125,7 @@ static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	}
 }
 
-static struct vm_operations_struct vgem_gem_vm_ops = {
+static const struct vm_operations_struct vgem_gem_vm_ops = {
 	.fault = vgem_gem_fault,
 	.open = drm_gem_vm_open,
 	.close = drm_gem_vm_close,

drivers/hsi/clients/cmt_speech.c +1 -1

index 4983529a9c6c..914b2c2d1f2d 100644
@@ -1105,7 +1105,7 @@ static int cs_char_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	return 0;
 }
 
-static struct vm_operations_struct cs_char_vm_ops = {
+static const struct vm_operations_struct cs_char_vm_ops = {
 	.fault	= cs_char_vma_fault,
 };

drivers/infiniband/hw/qib/qib_file_ops.c +1 -1

index 725881890c4a..e449e394963f 100644
@@ -908,7 +908,7 @@ static int qib_file_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	return 0;
 }
 
-static struct vm_operations_struct qib_file_vm_ops = {
+static const struct vm_operations_struct qib_file_vm_ops = {
 	.fault = qib_file_vma_fault,
 };

drivers/infiniband/hw/qib/qib_mmap.c +1 -1

index 146cf29a2e1d..34927b700b0e 100644
@@ -75,7 +75,7 @@ static void qib_vma_close(struct vm_area_struct *vma)
 	kref_put(&ip->ref, qib_release_mmap_info);
 }
 
-static struct vm_operations_struct qib_vm_ops = {
+static const struct vm_operations_struct qib_vm_ops = {
 	.open =     qib_vma_open,
 	.close =    qib_vma_close,
 };

drivers/media/platform/omap/omap_vout.c +1 -1

index 17b189a81ec5..38a50df8d629 100644
@@ -876,7 +876,7 @@ static void omap_vout_vm_close(struct vm_area_struct *vma)
 	vout->mmap_count--;
 }
 
-static struct vm_operations_struct omap_vout_vm_ops = {
+static const struct vm_operations_struct omap_vout_vm_ops = {
 	.open	= omap_vout_vm_open,
 	.close	= omap_vout_vm_close,
 };

drivers/misc/genwqe/card_dev.c +1 -1

index c49d244265ec..70e62d6a3231 100644
@@ -418,7 +418,7 @@ static void genwqe_vma_close(struct vm_area_struct *vma)
 	kfree(dma_map);
 }
 
-static struct vm_operations_struct genwqe_vma_ops = {
+static const struct vm_operations_struct genwqe_vma_ops = {
 	.open   = genwqe_vma_open,
 	.close  = genwqe_vma_close,
 };

drivers/staging/android/ion/ion.c +1 -1

index b0b96ab31954..2bbfed9acf19 100644
@@ -997,7 +997,7 @@ static void ion_vm_close(struct vm_area_struct *vma)
 	mutex_unlock(&buffer->lock);
 }
 
-static struct vm_operations_struct ion_vma_ops = {
+static const struct vm_operations_struct ion_vma_ops = {
 	.open = ion_vm_open,
 	.close = ion_vm_close,
 	.fault = ion_vm_fault,

drivers/staging/comedi/comedi_fops.c +1 -1

index e78ddbe5a954..1649665eb633 100644
@@ -2127,7 +2127,7 @@ static void comedi_vm_close(struct vm_area_struct *area)
 	comedi_buf_map_put(bm);
 }
 
-static struct vm_operations_struct comedi_vm_ops = {
+static const struct vm_operations_struct comedi_vm_ops = {
 	.open = comedi_vm_open,
 	.close = comedi_vm_close,
 };

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -1

index 4f0cbb54d4db..d3af01c94a58 100644
@@ -1091,7 +1091,7 @@ static void mmap_user_close(struct vm_area_struct *vma)
 	omapfb_put_mem_region(rg);
 }
 
-static struct vm_operations_struct mmap_user_ops = {
+static const struct vm_operations_struct mmap_user_ops = {
 	.open = mmap_user_open,
 	.close = mmap_user_close,
 };

drivers/xen/gntalloc.c +1 -1

index e53fe191738c..696301d9dc91 100644
@@ -493,7 +493,7 @@ static void gntalloc_vma_close(struct vm_area_struct *vma)
 	mutex_unlock(&gref_mutex);
 }
 
-static struct vm_operations_struct gntalloc_vmops = {
+static const struct vm_operations_struct gntalloc_vmops = {
 	.open = gntalloc_vma_open,
 	.close = gntalloc_vma_close,
 };

drivers/xen/gntdev.c +1 -1

index 89274850741b..b112d529a2d1 100644
@@ -433,7 +433,7 @@ static struct page *gntdev_vma_find_special_page(struct vm_area_struct *vma,
 	return map->pages[(addr - map->pages_vm_start) >> PAGE_SHIFT];
 }
 
-static struct vm_operations_struct gntdev_vmops = {
+static const struct vm_operations_struct gntdev_vmops = {
 	.open = gntdev_vma_open,
 	.close = gntdev_vma_close,
 	.find_special_page = gntdev_vma_find_special_page,

drivers/xen/privcmd.c +2 -2

index 5a296161d843..56cb13fcbd0e 100644
@@ -414,7 +414,7 @@ static int alloc_empty_pages(struct vm_area_struct *vma, int numpgs)
 	return 0;
 }
 
-static struct vm_operations_struct privcmd_vm_ops;
+static const struct vm_operations_struct privcmd_vm_ops;
 
 static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 {

@@ -605,7 +605,7 @@ static int privcmd_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
-static struct vm_operations_struct privcmd_vm_ops = {
+static const struct vm_operations_struct privcmd_vm_ops = {
 	.close = privcmd_close,
 	.fault = privcmd_fault
 };

fs/ceph/addr.c +1 -1

index e162bcd105ee..9fa7b3812851 100644
@@ -1582,7 +1582,7 @@ out:
 	return err;
 }
 
-static struct vm_operations_struct ceph_vmops = {
+static const struct vm_operations_struct ceph_vmops = {
 	.fault		= ceph_filemap_fault,
 	.page_mkwrite	= ceph_page_mkwrite,
 };

fs/cifs/file.c +1 -1

index 2ac2d8471393..94f81962368c 100644
@@ -3216,7 +3216,7 @@ cifs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	return VM_FAULT_LOCKED;
 }
 
-static struct vm_operations_struct cifs_file_vm_ops = {
+static const struct vm_operations_struct cifs_file_vm_ops = {
 	.fault = filemap_fault,
 	.map_pages = filemap_map_pages,
 	.page_mkwrite = cifs_page_mkwrite,

security/selinux/selinuxfs.c +1 -1

index d2787cca1fcb..0538f7034ef2 100644
@@ -472,7 +472,7 @@ static int sel_mmap_policy_fault(struct vm_area_struct *vma,
 	return 0;
 }
 
-static struct vm_operations_struct sel_mmap_policy_ops = {
+static const struct vm_operations_struct sel_mmap_policy_ops = {
 	.fault = sel_mmap_policy_fault,
 	.page_mkwrite = sel_mmap_policy_fault,
 };

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #2 lore raw

PATCH 2/5 x86, mpx: do not set ->vm_ops on mpx VMAs

In reply to Kirill A. Shutemov

MPX setups private anonymous mapping, but uses vma->vm_ops too. This can confuse core VM, as it relies on vm->vm_ops to distinguish file VMAs from anonymous.

As result we will get SIGBUS, because handle_pte_fault() thinks it's file VMA without vm_ops->fault and it doesn't know how to handle the situation properly.

Let's fix that by not setting ->vm_ops.

We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.

The only thing left is name of VMA. I'm not sure if it's part of ABI, or we can just drop it. The patch keep it by providing arch_vma_name() on x86.

Build tested only.

Signed-off-by Kirill A. Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Thomas Gleixner <[email protected]>

---
 arch/x86/mm/mmap.c |  7 +++++++
 arch/x86/mm/mpx.c  | 20 +-------------------
 2 files changed, 8 insertions(+), 19 deletions(-)

arch/x86/mm/mmap.c +7 -0

index 9d518d693b4b..844b06d67df4 100644
@@ -126,3 +126,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
 		mm->get_unmapped_area = arch_get_unmapped_area_topdown;
 	}
 }
+
+const char *arch_vma_name(struct vm_area_struct *vma)
+{
+	if (vma->vm_flags & VM_MPX)
+		return "[mpx]";
+	return NULL;
+}

arch/x86/mm/mpx.c +1 ~~-19~~

index c439ec478216..4d1c11c07fe1 100644
@@ -18,26 +18,9 @@
 #include <asm/processor.h>
 #include <asm/fpu-internal.h>
 
-static const char *mpx_mapping_name(struct vm_area_struct *vma)
-{
-	return "[mpx]";
-}
-
-static struct vm_operations_struct mpx_vma_ops = {
-	.name = mpx_mapping_name,
-};
-
-static int is_mpx_vma(struct vm_area_struct *vma)
-{
-	return (vma->vm_ops == &mpx_vma_ops);
-}
-
 /*
  * This is really a simplified "vm_mmap". it only handles MPX
  * bounds tables (the bounds directory is user-allocated).
- *
- * Later on, we use the vma->vm_ops to uniquely identify these
- * VMAs.
  */
 static unsigned long mpx_mmap(unsigned long len)
 {

@@ -83,7 +66,6 @@ static unsigned long mpx_mmap(unsigned long len)
 		ret = -ENOMEM;
 		goto out;
 	}
-	vma->vm_ops = &mpx_vma_ops;
 
 	if (vm_flags & VM_LOCKED) {
 		up_write(&mm->mmap_sem);

@@ -661,7 +643,7 @@ static int zap_bt_entries(struct mm_struct *mm,
 		 * so stop immediately and return an error.  This
 		 * probably results in a SIGSEGV.
 		 */
-		if (!is_mpx_vma(vma))
+		if (!(vma->vm_flags & VM_MPX))
 			return -EINVAL;
 
 		len = min(vma->vm_end, end) - addr;

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Leon Romanovsky

2015-07-13 12:06 UTC #3 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Hi Kirill,

Quoted 34 lines · Kirill A. Shutemov

MPX setups private anonymous mapping, but uses vma->vm_ops too. This can confuse core VM, as it relies on vm->vm_ops to distinguish file VMAs from anonymous.
As result we will get SIGBUS, because handle_pte_fault() thinks it's file VMA without vm_ops->fault and it doesn't know how to handle the situation properly.
Let's fix that by not setting ->vm_ops.
We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.
The only thing left is name of VMA. I'm not sure if it's part of ABI, or we can just drop it. The patch keep it by providing arch_vma_name() on x86.
Build tested only.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 arch/x86/mm/mmap.c |  7 +++++++
 arch/x86/mm/mpx.c  | 20 +-------------------
 2 files changed, 8 insertions(+), 19 deletions(-)
arch/x86/mm/mmap.c +0 -0
index 9d518d693b4b..844b06d67df4 100644

arch/x86/mm/mmap.c void arch_pick_mmap_layout(struct mm_struct *mm) · line 135

Quoted 10 lines

arch/x86/mm/mmap.c +7 -0

@@ -126,3 +126,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
                mm->get_unmapped_area = arch_get_unmapped_area_topdown;
        }
 }
+
+const char *arch_vma_name(struct vm_area_struct *vma)
+{
+       if (vma->vm_flags & VM_MPX)
+               return "[mpx]";
+       return NULL;
+}

I sure that I'm missing something important. This function stores "[mpx]" string on this function stack and returns the pointer to that address. In current flow, this address is visible and accessible, however in can be a different in general case.

Quoted 56 lines

arch/x86/mm/mpx.c +1 ~~-20~~

index c439ec478216..4d1c11c07fe1 100644
@@ -18,26 +18,9 @@
 #include <asm/processor.h>
 #include <asm/fpu-internal.h>
 
-static const char *mpx_mapping_name(struct vm_area_struct *vma)
-{
-       return "[mpx]";
-}
-
-static struct vm_operations_struct mpx_vma_ops = {
-       .name = mpx_mapping_name,
-};
-
-static int is_mpx_vma(struct vm_area_struct *vma)
-{
-       return (vma->vm_ops == &mpx_vma_ops);
-}
-
 /*
  * This is really a simplified "vm_mmap". it only handles MPX
  * bounds tables (the bounds directory is user-allocated).
- *
- * Later on, we use the vma->vm_ops to uniquely identify these
- * VMAs.
  */
 static unsigned long mpx_mmap(unsigned long len)
 {

@@ -83,7 +66,6 @@ static unsigned long mpx_mmap(unsigned long len)
                ret = -ENOMEM;
                goto out;
        }
-       vma->vm_ops = &mpx_vma_ops;
 
        if (vm_flags & VM_LOCKED) {
                up_write(&mm->mmap_sem);

@@ -661,7 +643,7 @@ static int zap_bt_entries(struct mm_struct *mm,
                 * so stop immediately and return an error.  This
                 * probably results in a SIGSEGV.
                 */
-               if (!is_mpx_vma(vma))
+               if (!(vma->vm_flags & VM_MPX))
                        return -EINVAL;
 
                len = min(vma->vm_end, end) - addr;
--

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 8 lines

Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | [email protected]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kirill A. Shutemov

author

2015-07-13 12:29 UTC #4 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 45 lines · Leon Romanovsky

Hi Kirill,
Quoted 41 lines · Kirill A. Shutemov
MPX setups private anonymous mapping, but uses vma->vm_ops too. This can confuse core VM, as it relies on vm->vm_ops to distinguish file VMAs from anonymous.
As result we will get SIGBUS, because handle_pte_fault() thinks it's file VMA without vm_ops->fault and it doesn't know how to handle the situation properly.
Let's fix that by not setting ->vm_ops.
We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.
The only thing left is name of VMA. I'm not sure if it's part of ABI, or we can just drop it. The patch keep it by providing arch_vma_name() on x86.
Build tested only.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 arch/x86/mm/mmap.c |  7 +++++++
 arch/x86/mm/mpx.c  | 20 +-------------------
 2 files changed, 8 insertions(+), 19 deletions(-)
arch/x86/mm/mmap.c +4 -0
index 9d518d693b4b..844b06d67df4 100644
@@ -126,3 +126,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
                mm->get_unmapped_area = arch_get_unmapped_area_topdown;
        }
 }
+
+const char *arch_vma_name(struct vm_area_struct *vma)
+{
+       if (vma->vm_flags & VM_MPX)

Quoted 8 lines

arch/x86/mm/mmap.c void arch_pick_mmap_layout(struct mm_struct *mm) · line 135
Quoted 3 lines
arch/x86/mm/mmap.c +3 -0
+               return "[mpx]";
+       return NULL;
+}
I sure that I'm missing something important. This function stores "[mpx]" string on this function stack and returns the pointer to that address. In current flow, this address is visible and accessible, however in can be a different in general case.

The string is not on stack. String literals are in .rodata and caller is not allowed to modify it since it's "const char *".

Signature · 7 lines

 Kirill

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Leon Romanovsky

2015-07-13 12:42 UTC #5 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 55 lines · Kirill A. Shutemov

Quoted 45 lines · Leon Romanovsky
Hi Kirill,
Quoted 41 lines · Kirill A. Shutemov
MPX setups private anonymous mapping, but uses vma->vm_ops too. This can confuse core VM, as it relies on vm->vm_ops to distinguish file VMAs from anonymous.
As result we will get SIGBUS, because handle_pte_fault() thinks it's file VMA without vm_ops->fault and it doesn't know how to handle the situation properly.
Let's fix that by not setting ->vm_ops.
We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.
The only thing left is name of VMA. I'm not sure if it's part of ABI, or we can just drop it. The patch keep it by providing arch_vma_name() on x86.
Build tested only.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 arch/x86/mm/mmap.c |  7 +++++++
 arch/x86/mm/mpx.c  | 20 +-------------------
 2 files changed, 8 insertions(+), 19 deletions(-)
arch/x86/mm/mmap.c +4 -0
index 9d518d693b4b..844b06d67df4 100644
@@ -126,3 +126,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
                mm->get_unmapped_area = arch_get_unmapped_area_topdown;
        }
 }
+
+const char *arch_vma_name(struct vm_area_struct *vma)
+{
+       if (vma->vm_flags & VM_MPX)
Quoted 8 lines
arch/x86/mm/mmap.c void arch_pick_mmap_layout(struct mm_struct *mm) · line 135
Quoted 3 lines
arch/x86/mm/mmap.c +3 -0
+               return "[mpx]";
+       return NULL;
+}
I sure that I'm missing something important. This function stores "[mpx]" string on this function stack and returns the pointer to that address. In current flow, this address is visible and accessible, however in can be a different in general case.

Quoted 2 lines

The string is not on stack. String literals are in .rodata and caller is not allowed to modify it since it's "const char *".

I see, it behaves similiar to global "const char *" variable definition. Thank you for clarification.

Quoted 3 lines

--
Kirill

Signature · 8 lines

Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | [email protected]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oleg Nesterov

2015-07-13 16:55 UTC #6 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

In reply to Kirill A. Shutemov

Quoted 4 lines · Kirill A. Shutemov

We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.

Agreed.

I am wondering if something like the patch below (on top of yours) makes sense... Not sure, but mpx_mmap() doesn't look nice too, and with this change we can unexport mmap_region().

Oleg.

 arch/x86/mm/mpx.c  |   51 +++++++--------------------------------------------
 include/linux/mm.h |    3 +++
 mm/mmap.c          |   16 +++++++++++-----
 3 files changed, 21 insertions(+), 49 deletions(-)

arch/x86/mm/mpx.c +7 ~~-44~~

index 4d1c11c..da8b713 100644
@@ -24,58 +24,21 @@
  */
 static unsigned long mpx_mmap(unsigned long len)
 {
-	unsigned long ret;
-	unsigned long addr, pgoff;
+	unsigned long addr, populate;
 	struct mm_struct *mm = current->mm;
-	vm_flags_t vm_flags;
-	struct vm_area_struct *vma;
 
 	/* Only bounds table and bounds directory can be allocated here */
 	if (len != MPX_BD_SIZE_BYTES && len != MPX_BT_SIZE_BYTES)
 		return -EINVAL;
 
 	down_write(&mm->mmap_sem);
-
-	/* Too many mappings? */
-	if (mm->map_count > sysctl_max_map_count) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	/* Obtain the address to map to. we verify (or select) it and ensure
-	 * that it represents a valid section of the address space.
-	 */
-	addr = get_unmapped_area(NULL, 0, len, 0, MAP_ANONYMOUS | MAP_PRIVATE);
-	if (addr & ~PAGE_MASK) {
-		ret = addr;
-		goto out;
-	}
-
-	vm_flags = VM_READ | VM_WRITE | VM_MPX |
-			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
-
-	/* Set pgoff according to addr for anon_vma */
-	pgoff = addr >> PAGE_SHIFT;
-
-	ret = mmap_region(NULL, addr, len, vm_flags, pgoff);
-	if (IS_ERR_VALUE(ret))
-		goto out;
-
-	vma = find_vma(mm, ret);
-	if (!vma) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	if (vm_flags & VM_LOCKED) {
-		up_write(&mm->mmap_sem);
-		mm_populate(ret, len);
-		return ret;
-	}
-
-out:
+	addr = __do_mmap_pgoff(NULL, 0, len, PROT_READ | PROT_WRITE,
+			MAP_ANONYMOUS | MAP_PRIVATE, 0, &populate, VM_MPX);
 	up_write(&mm->mmap_sem);
-	return ret;
+	if (populate)
+		mm_populate(addr, populate);
+
+	return addr;
 }
 
 enum reg_type {

include/linux/mm.h +3 -0

index 0207ffa..910d475 100644
@@ -1849,6 +1849,9 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
 
 extern unsigned long mmap_region(struct file *file, unsigned long addr,
 	unsigned long len, vm_flags_t vm_flags, unsigned long pgoff);
+extern unsigned long __do_mmap_pgoff(struct file *file, unsigned long addr,
+	unsigned long len, unsigned long prot, unsigned long flags,
+	unsigned long pgoff, unsigned long *populate, vm_flags_t vm_flags);
 extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	unsigned long len, unsigned long prot, unsigned long flags,
 	unsigned long pgoff, unsigned long *populate);

mm/mmap.c +11 -5

index 2185cd9..88bc961 100644
@@ -1247,14 +1247,12 @@ static inline int mlock_future_check(struct mm_struct *mm,
 /*
  * The caller must hold down_write(&current->mm->mmap_sem).
  */
-
-unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+unsigned long __do_mmap_pgoff(struct file *file, unsigned long addr,
 			unsigned long len, unsigned long prot,
 			unsigned long flags, unsigned long pgoff,
-			unsigned long *populate)
+			unsigned long *populate, vm_flags_t vm_flags)
 {
 	struct mm_struct *mm = current->mm;
-	vm_flags_t vm_flags;
 
 	*populate = 0;

@@ -1298,7 +1296,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+	vm_flags |= calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
 			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
 	if (flags & MAP_LOCKED)

@@ -1396,6 +1394,14 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	return addr;
 }
 
+unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+			unsigned long len, unsigned long prot,
+			unsigned long flags, unsigned long pgoff,
+			unsigned long *populate)
+{
+	return __do_mmap_pgoff(file, addr, len, prot, flags, pgoff, populate, 0);
+}
+
 SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 		unsigned long, prot, unsigned long, flags,
 		unsigned long, fd, unsigned long, pgoff)

Dave Hansen

maintainer

2015-07-13 17:07 UTC #7 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 11 lines · Oleg Nesterov

Quoted 4 lines · Kirill A. Shutemov
We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.
Agreed.
I am wondering if something like the patch below (on top of yours) makes sense... Not sure, but mpx_mmap() doesn't look nice too, and with this change we can unexport mmap_region().

These both look nice to me (and they both cull specialty MPX code which is excellent). I'll run them through a quick test.

Kirill A. Shutemov

author

2015-07-16 11:05 UTC #8 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 7 lines · Dave Hansen

Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Kirill A. Shutemov
We don't really need ->vm_ops here: MPX VMA can be detected with VM_MPX flag. And vma_merge() will not merge MPX VMA with non-MPX VMA, because ->vm_flags won't match.

Quoted 8 lines

Quoted 5 lines
Agreed.
I am wondering if something like the patch below (on top of yours) makes sense... Not sure, but mpx_mmap() doesn't look nice too, and with this change we can unexport mmap_region().
These both look nice to me (and they both cull specialty MPX code which is excellent). I'll run them through a quick test.

Any update?

Signature · 7 lines

 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dave Hansen

maintainer

2015-07-16 15:53 UTC #9 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 3 lines · Kirill A. Shutemov

Quoted 2 lines
Quoted 2 lines
These both look nice to me (and they both cull specialty MPX code which is excellent). I'll run them through a quick test.
Any update?

Both patches look fine to me and test OK. Feel free to add my acked/tested/etc...

Kirill A. Shutemov

2015-07-16 16:09 UTC #10 lore raw

Re: [PATCH 2/5] x86, mpx: do not set ->vm_ops on mpx VMAs

Quoted 7 lines · Dave Hansen

Quoted 3 lines · Kirill A. Shutemov
Quoted 2 lines
Quoted 2 lines
These both look nice to me (and they both cull specialty MPX code which is excellent). I'll run them through a quick test.
Any update?
Both patches look fine to me and test OK. Feel free to add my acked/tested/etc...

Oleg, could you prepare a proper patch with description/signed-off-by?

I'll send updated patchset with all changes to Andrew.

Signature · 7 lines

 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oleg Nesterov

2015-07-16 22:27 UTC #11 lore raw

cover mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()

Quoted 5 lines · Kirill A. Shutemov

Quoted 3 lines · Dave Hansen
Quoted 2 lines · Kirill A. Shutemov
Quoted 2 lines
Quoted 2 lines
These both look nice to me (and they both cull specialty MPX code which is excellent). I'll run them through a quick test.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Any update?
Both patches look fine to me and test OK. Feel free to add my acked/tested/etc...
Oleg, could you prepare a proper patch with description/signed-off-by?
I'll send updated patchset with all changes to Andrew.

With pleasure, please see 1/1.

Changes:

- s/__do_mmap_pgoff/do_mmap/

- update mm/nommu.c too

- make do_mmap_pgoff() inline (perhaps we should kill it), this also avoids another change in nommu.c

Oleg.

Oleg Nesterov

2015-07-16 22:28 UTC #12 lore raw

PATCH 1/1 mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()

Add the additional "vm_flags_t vm_flags" argument to do_mmap_pgoff(), rename it to do_mmap(), and re-introduce do_mmap_pgoff() as a simple wrapper on top of do_mmap(). Perhaps we should update the callers of do_mmap_pgoff() and kill it later.

This way mpx_mmap() can simply call do_mmap(vm_flags => VM_MPX) and do not play with vm internals.

After this change mmap_region() has a single user outside of mmap.c, arch/tile/mm/elf.c:arch_setup_additional_pages(). It would be nice to change arch/tile/ and unexport mmap_region().

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/mm/mpx.c  |   51 +++++++--------------------------------------------
 include/linux/mm.h |   12 ++++++++++--
 mm/mmap.c          |   10 ++++------
 mm/nommu.c         |   15 ++++++++-------
 4 files changed, 29 insertions(+), 59 deletions(-)

arch/x86/mm/mpx.c +7 ~~-44~~

index 4d1c11c..fdbd3e0 100644
@@ -24,58 +24,21 @@
  */
 static unsigned long mpx_mmap(unsigned long len)
 {
-	unsigned long ret;
-	unsigned long addr, pgoff;
 	struct mm_struct *mm = current->mm;
-	vm_flags_t vm_flags;
-	struct vm_area_struct *vma;
+	unsigned long addr, populate;
 
 	/* Only bounds table and bounds directory can be allocated here */
 	if (len != MPX_BD_SIZE_BYTES && len != MPX_BT_SIZE_BYTES)
 		return -EINVAL;
 
 	down_write(&mm->mmap_sem);
-
-	/* Too many mappings? */
-	if (mm->map_count > sysctl_max_map_count) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	/* Obtain the address to map to. we verify (or select) it and ensure
-	 * that it represents a valid section of the address space.
-	 */
-	addr = get_unmapped_area(NULL, 0, len, 0, MAP_ANONYMOUS | MAP_PRIVATE);
-	if (addr & ~PAGE_MASK) {
-		ret = addr;
-		goto out;
-	}
-
-	vm_flags = VM_READ | VM_WRITE | VM_MPX |
-			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
-
-	/* Set pgoff according to addr for anon_vma */
-	pgoff = addr >> PAGE_SHIFT;
-
-	ret = mmap_region(NULL, addr, len, vm_flags, pgoff);
-	if (IS_ERR_VALUE(ret))
-		goto out;
-
-	vma = find_vma(mm, ret);
-	if (!vma) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	if (vm_flags & VM_LOCKED) {
-		up_write(&mm->mmap_sem);
-		mm_populate(ret, len);
-		return ret;
-	}
-
-out:
+	addr = do_mmap(NULL, 0, len, PROT_READ | PROT_WRITE,
+			MAP_ANONYMOUS | MAP_PRIVATE, VM_MPX, 0, &populate);
 	up_write(&mm->mmap_sem);
-	return ret;
+	if (populate)
+		mm_populate(addr, populate);
+
+	return addr;
 }
 
 enum reg_type {

include/linux/mm.h +10 -2

index 0207ffa..1f0a56e 100644
@@ -1849,11 +1849,19 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
 
 extern unsigned long mmap_region(struct file *file, unsigned long addr,
 	unsigned long len, vm_flags_t vm_flags, unsigned long pgoff);
-extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+extern unsigned long do_mmap(struct file *file, unsigned long addr,
 	unsigned long len, unsigned long prot, unsigned long flags,
-	unsigned long pgoff, unsigned long *populate);
+	vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate);
 extern int do_munmap(struct mm_struct *, unsigned long, size_t);
 
+static inline unsigned long
+do_mmap_pgoff(struct file *file, unsigned long addr,
+	unsigned long len, unsigned long prot, unsigned long flags,
+	unsigned long pgoff, unsigned long *populate)
+{
+	return do_mmap(file, addr, len, prot, flags, 0, pgoff, populate);
+}
+
 #ifdef CONFIG_MMU
 extern int __mm_populate(unsigned long addr, unsigned long len,
 			 int ignore_errors);

mm/mmap.c +4 -6

index 2185cd9..2f36ebc 100644
@@ -1247,14 +1247,12 @@ static inline int mlock_future_check(struct mm_struct *mm,
 /*
  * The caller must hold down_write(&current->mm->mmap_sem).
  */
-
-unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+unsigned long do_mmap(struct file *file, unsigned long addr,
 			unsigned long len, unsigned long prot,
-			unsigned long flags, unsigned long pgoff,
-			unsigned long *populate)
+			unsigned long flags, vm_flags_t vm_flags,
+			unsigned long pgoff, unsigned long *populate)
 {
 	struct mm_struct *mm = current->mm;
-	vm_flags_t vm_flags;
 
 	*populate = 0;

@@ -1298,7 +1296,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+	vm_flags |= calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
 			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
 	if (flags & MAP_LOCKED)

mm/nommu.c +8 -7

index e544508..e3026fd 100644
@@ -1271,13 +1271,14 @@ enomem:
 /*
  * handle mapping creation for uClinux
  */
-unsigned long do_mmap_pgoff(struct file *file,
-			    unsigned long addr,
-			    unsigned long len,
-			    unsigned long prot,
-			    unsigned long flags,
-			    unsigned long pgoff,
-			    unsigned long *populate)
+unsigned long do_mmap(struct file *file,
+			unsigned long addr,
+			unsigned long len,
+			unsigned long prot,
+			unsigned long flags,
+			vm_flags_t vm_flags,
+			unsigned long pgoff,
+			unsigned long *populate)
 {
 	struct vm_area_struct *vma;
 	struct vm_region *region;

Signature · 8 lines

1.5.5.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Paul Gortmaker

2015-07-24 14:39 UTC #13 lore raw

Re: [PATCH 1/1] mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()

Quoted 4 lines · Oleg Nesterov

Add the additional "vm_flags_t vm_flags" argument to do_mmap_pgoff(), rename it to do_mmap(), and re-introduce do_mmap_pgoff() as a simple wrapper on top of do_mmap(). Perhaps we should update the callers of do_mmap_pgoff() and kill it later.

It seems that the version of this patch in linux-next breaks all nommu builds (m86k, some arm, etc).

mm/nommu.c: In function 'do_mmap':
mm/nommu.c:1248:30: error: 'vm_flags' redeclared as different kind of symbol mm/nommu.c:1241:15: note: previous definition of 'vm_flags' was here scripts/Makefile.build:258: recipe for target 'mm/nommu.o' failed

http://kisskb.ellerman.id.au/kisskb/buildresult/12470285/

Bisect says:

31705a3a633bb63683918f055fe6032939672b61 is the first bad commit commit 31705a3a633bb63683918f055fe6032939672b61

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jul 24 09:20:30 2015 +1000

mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()

Paul.
--

Quoted 173 lines

This way mpx_mmap() can simply call do_mmap(vm_flags => VM_MPX) and do not play with vm internals.

After this change mmap_region() has a single user outside of mmap.c, arch/tile/mm/elf.c:arch_setup_additional_pages(). It would be nice to change arch/tile/ and unexport mmap_region().

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/mm/mpx.c  |   51 +++++++--------------------------------------------
 include/linux/mm.h |   12 ++++++++++--
 mm/mmap.c          |   10 ++++------
 mm/nommu.c         |   15 ++++++++-------
 4 files changed, 29 insertions(+), 59 deletions(-)

arch/x86/mm/mpx.c +7 ~~-44~~

index 4d1c11c..fdbd3e0 100644
@@ -24,58 +24,21 @@
  */
 static unsigned long mpx_mmap(unsigned long len)
 {
-       unsigned long ret;
-       unsigned long addr, pgoff;
        struct mm_struct *mm = current->mm;
-       vm_flags_t vm_flags;
-       struct vm_area_struct *vma;
+       unsigned long addr, populate;
 
        /* Only bounds table and bounds directory can be allocated here */
        if (len != MPX_BD_SIZE_BYTES && len != MPX_BT_SIZE_BYTES)
                return -EINVAL;
 
        down_write(&mm->mmap_sem);
-
-       /* Too many mappings? */
-       if (mm->map_count > sysctl_max_map_count) {
-               ret = -ENOMEM;
-               goto out;
-       }
-
-       /* Obtain the address to map to. we verify (or select) it and ensure
-        * that it represents a valid section of the address space.
-        */
-       addr = get_unmapped_area(NULL, 0, len, 0, MAP_ANONYMOUS | MAP_PRIVATE);
-       if (addr & ~PAGE_MASK) {
-               ret = addr;
-               goto out;
-       }
-
-       vm_flags = VM_READ | VM_WRITE | VM_MPX |
-                       mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
-
-       /* Set pgoff according to addr for anon_vma */
-       pgoff = addr >> PAGE_SHIFT;
-
-       ret = mmap_region(NULL, addr, len, vm_flags, pgoff);
-       if (IS_ERR_VALUE(ret))
-               goto out;
-
-       vma = find_vma(mm, ret);
-       if (!vma) {
-               ret = -ENOMEM;
-               goto out;
-       }
-
-       if (vm_flags & VM_LOCKED) {
-               up_write(&mm->mmap_sem);
-               mm_populate(ret, len);
-               return ret;
-       }
-
-out:
+       addr = do_mmap(NULL, 0, len, PROT_READ | PROT_WRITE,
+                       MAP_ANONYMOUS | MAP_PRIVATE, VM_MPX, 0, &populate);
        up_write(&mm->mmap_sem);
-       return ret;
+       if (populate)
+               mm_populate(addr, populate);
+
+       return addr;
 }
 
 enum reg_type {

include/linux/mm.h +10 -2

index 0207ffa..1f0a56e 100644
@@ -1849,11 +1849,19 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
 
 extern unsigned long mmap_region(struct file *file, unsigned long addr,
        unsigned long len, vm_flags_t vm_flags, unsigned long pgoff);
-extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+extern unsigned long do_mmap(struct file *file, unsigned long addr,
        unsigned long len, unsigned long prot, unsigned long flags,
-       unsigned long pgoff, unsigned long *populate);
+       vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate);
 extern int do_munmap(struct mm_struct *, unsigned long, size_t);
 
+static inline unsigned long
+do_mmap_pgoff(struct file *file, unsigned long addr,
+       unsigned long len, unsigned long prot, unsigned long flags,
+       unsigned long pgoff, unsigned long *populate)
+{
+       return do_mmap(file, addr, len, prot, flags, 0, pgoff, populate);
+}
+
 #ifdef CONFIG_MMU
 extern int __mm_populate(unsigned long addr, unsigned long len,
                         int ignore_errors);

mm/mmap.c +4 -6

index 2185cd9..2f36ebc 100644
@@ -1247,14 +1247,12 @@ static inline int mlock_future_check(struct mm_struct *mm,
 /*
  * The caller must hold down_write(&current->mm->mmap_sem).
  */
-
-unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
+unsigned long do_mmap(struct file *file, unsigned long addr,
                        unsigned long len, unsigned long prot,
-                       unsigned long flags, unsigned long pgoff,
-                       unsigned long *populate)
+                       unsigned long flags, vm_flags_t vm_flags,
+                       unsigned long pgoff, unsigned long *populate)
 {
        struct mm_struct *mm = current->mm;
-       vm_flags_t vm_flags;
 
        *populate = 0;

@@ -1298,7 +1296,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
         * to. we assume access permissions have been handled by the open
         * of the memory object, so we don't do any here.
         */
-       vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+       vm_flags |= calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
                        mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
        if (flags & MAP_LOCKED)

mm/nommu.c +8 -8

index e544508..e3026fd 100644
@@ -1271,13 +1271,14 @@ enomem:
 /*
  * handle mapping creation for uClinux
  */
-unsigned long do_mmap_pgoff(struct file *file,
-                           unsigned long addr,
-                           unsigned long len,
-                           unsigned long prot,
-                           unsigned long flags,
-                           unsigned long pgoff,
-                           unsigned long *populate)
+unsigned long do_mmap(struct file *file,
+                       unsigned long addr,
+                       unsigned long len,
+                       unsigned long prot,
+                       unsigned long flags,
+                       vm_flags_t vm_flags,
+                       unsigned long pgoff,
+                       unsigned long *populate)
 {
        struct vm_area_struct *vma;
        struct vm_region *region;
--

1.5.5.1

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Mark Salter

2015-07-27 19:34 UTC #14 lore raw

Re: [PATCH 1/1] mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()

Quoted 18 lines · Paul Gortmaker

Quoted 4 lines · Oleg Nesterov
Add the additional "vm_flags_t vm_flags" argument to do_mmap_pgoff(), rename it to do_mmap(), and re-introduce do_mmap_pgoff() as a simple wrapper on top of do_mmap(). Perhaps we should update the callers of do_mmap_pgoff() and kill it later.
It seems that the version of this patch in linux-next breaks all nommu builds (m86k, some arm, etc).
mm/nommu.c: In function 'do_mmap':
mm/nommu.c:1248:30: error: 'vm_flags' redeclared as different kind of symbol mm/nommu.c:1241:15: note: previous definition of 'vm_flags' was here scripts/Makefile.build:258: recipe for target 'mm/nommu.o' failed
http://kisskb.ellerman.id.au/kisskb/buildresult/12470285/
Bisect says:

Quoted 8 lines

31705a3a633bb63683918f055fe6032939672b61 is the first bad commit commit 31705a3a633bb63683918f055fe6032939672b61
Author: Oleg Nesterov <[email protected]>
Date:   Fri Jul 24 09:20:30 2015 +1000
mm, mpx: add "vm_flags_t vm_flags" arg to do_mmap_pgoff()
Paul.

This fixes the build error and runs fine on c6x:

mm/nommu.c +2 -2

index 530eea5..af2196e 100644
@@ -1245,7 +1245,7 @@ unsigned long do_mmap(struct file *file,
 	struct vm_area_struct *vma;
 	struct vm_region *region;
 	struct rb_node *rb;
-	unsigned long capabilities, vm_flags, result;
+	unsigned long capabilities, result;
 	int ret;
 
 	*populate = 0;

@@ -1263,7 +1263,7 @@ unsigned long do_mmap(struct file *file,
 
 	/* we've determined that we can make the mapping, now translate what we
 	 * now know into VMA flags */
-	vm_flags = determine_vm_flags(file, prot, flags, capabilities);
+	vm_flags |= determine_vm_flags(file, prot, flags, capabilities);
 
 	/* we're going to need to record the mapping */
 	region = kmem_cache_zalloc(vm_region_jar, GFP_KERNEL);

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #15 lore raw

PATCH 3/5 mm: make sure all file VMAs have ->vm_ops set

In reply to Kirill A. Shutemov

We rely on vma->vm_ops == NULL to detect anonymous VMA: see vma_is_anonymous(), but some drivers doesn't set ->vm_ops.

As result we can end up with anonymous page in private file mapping. That's should not lead to serious misbehaviour, but nevertheless is wrong.

Let's fix by setting up dummy ->vm_ops for file mmapping if f_op->mmap() didn't set its own.

The patch also adds sanity check into __vma_link_rb(). It will help catch broken VMAs which inserted directly into mm_struct via insert_vm_struct().

Signed-off-by Kirill A. Shutemov <[email protected]>

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

mm/mmap.c +8 -0

index 30904c16b7d3..4ce7a6f33db0 100644
@@ -612,6 +612,8 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm,
 void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma,
 		struct rb_node **rb_link, struct rb_node *rb_parent)
 {
+	WARN_ONCE(vma->vm_file && !vma->vm_ops, "missing vma->vm_ops");
+
 	/* Update tracking information for the gap following the new vma. */
 	if (vma->vm_next)
 		vma_gap_update(vma->vm_next);

@@ -1638,6 +1640,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		 */
 		WARN_ON_ONCE(addr != vma->vm_start);
 
+		/* All file mapping must have ->vm_ops set */
+		if (!vma->vm_ops) {
+			static const struct vm_operations_struct dummy_ops = {};
+			vma->vm_ops = &dummy_ops;
+		}
+
 		addr = vma->vm_start;
 		vm_flags = vma->vm_flags;
 	} else if (vm_flags & VM_SHARED) {

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #16 lore raw

PATCH 4/5 mm, madvise: use vma_is_anonymous() to check for anon VMA

In reply to Kirill A. Shutemov

!vma->vm_file is not reliable to detect anon VMA, because not all drivers bother set it. Let's use vma_is_anonymous() instead.

Signed-off-by Kirill A. Shutemov <[email protected]>

Cc Minchan Kim <[email protected]>

---
 mm/madvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/madvise.c +1 -1

index 70ce0d425d72..a4fae076f61d 100644
@@ -393,7 +393,7 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
 		return -EINVAL;
 
 	/* MADV_FREE works for only anon vma at the moment */
-	if (vma->vm_file)
+	if (!vma_is_anonymous(vma))
 		return -EINVAL;
 
 	start = max(vma->vm_start, start_addr);

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

Acked-by on v1 4/5

2015-07-15 23:50 UTC #17 lore raw

Re: [PATCH 4/5] mm, madvise: use vma_is_anonymous() to check for anon VMA

Quoted 5 lines · Kirill A. Shutemov

!vma->vm_file is not reliable to detect anon VMA, because not all drivers bother set it. Let's use vma_is_anonymous() instead.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Minchan Kim <[email protected]>

Acked-by Minchan Kim <[email protected]>

Thanks.

Kirill A. Shutemov

author

2015-07-13 10:54 UTC #18 lore raw

PATCH 5/5 mm, memcontrol: use vma_is_anonymous() to check for anon VMA

In reply to Kirill A. Shutemov

!vma->vm_file is not reliable to detect anon VMA, because not all drivers bother set it. Let's use vma_is_anonymous() instead.

Signed-off-by Kirill A. Shutemov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memcontrol.c +1 -1

index acb93c554f6e..a624709f0dd7 100644
@@ -4809,7 +4809,7 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 	struct address_space *mapping;
 	pgoff_t pgoff;
 
-	if (!vma->vm_file) /* anonymous vma */
+	if (vma_is_anonymous(vma)) /* anonymous vma */
 		return NULL;
 	if (!(mc.flags & MOVE_FILE))
 		return NULL;

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Johannes Weiner

maintainer

2015-07-13 13:08 UTC #19 lore raw

Re: [PATCH 5/5] mm, memcontrol: use vma_is_anonymous() to check for anon VMA

Quoted 15 lines · Kirill A. Shutemov

!vma->vm_file is not reliable to detect anon VMA, because not all drivers bother set it. Let's use vma_is_anonymous() instead.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +0 -0
index acb93c554f6e..a624709f0dd7 100644

mm/memcontrol.c static struct page *mc_handle_file_pte(struct vm_area_struct *vma, · line 4815

Quoted 8 lines

mm/memcontrol.c +1 -1

@@ -4809,7 +4809,7 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 	struct address_space *mapping;
 	pgoff_t pgoff;
 
-	if (!vma->vm_file) /* anonymous vma */
+	if (vma_is_anonymous(vma)) /* anonymous vma */
 		return NULL;
 	if (!(mc.flags & MOVE_FILE))
 		return NULL;

The next line does vma->vm_file->f_mapping, so it had better be !NULL.

It's not about reliably detecting anonymous vs. file, it is about whether there is a mapping against which we can do find_get_page().

Kirill A. Shutemov

author

2015-07-13 13:21 UTC #20 lore raw

Re: [PATCH 5/5] mm, memcontrol: use vma_is_anonymous() to check for anon VMA

Quoted 27 lines · Johannes Weiner

Quoted 15 lines · Kirill A. Shutemov
!vma->vm_file is not reliable to detect anon VMA, because not all drivers bother set it. Let's use vma_is_anonymous() instead.
Signed-off-by Kirill A. Shutemov <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +0 -0
index acb93c554f6e..a624709f0dd7 100644
mm/memcontrol.c static struct page *mc_handle_file_pte(struct vm_area_struct *vma, · line 4815
Quoted 8 lines
mm/memcontrol.c +1 -1
@@ -4809,7 +4809,7 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 	struct address_space *mapping;
 	pgoff_t pgoff;
 
-	if (!vma->vm_file) /* anonymous vma */
+	if (vma_is_anonymous(vma)) /* anonymous vma */
 		return NULL;
 	if (!(mc.flags & MOVE_FILE))
 		return NULL;
The next line does vma->vm_file->f_mapping, so it had better be !NULL.

Quoted 2 lines

It's not about reliably detecting anonymous vs. file, it is about whether there is a mapping against which we can do find_get_page().

You're right. This patch is totally broken.

Signature · 7 lines

 Kirill

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>