[PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[David Howells]

From: David Howells <dhowells@redhat.com>

Set the backing device info capabilities for /dev/mem and /dev/kmem to permit
direct sharing under no-MMU conditions.

Also comment the capabilities for /dev/zero.

Signed-Off-By: David Howells <dhowells@redhat.com>
---

 drivers/char/mem.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 917b204..4c29619 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -238,6 +238,19 @@ #endif
 }
 #endif
 
+#ifndef CONFIG_MMU
+static unsigned long get_unmapped_area_mem(struct file *file,
+					   unsigned long addr,
+					   unsigned long len,
+					   unsigned long pgoff,
+					   unsigned long flags)
+{
+	if (!valid_mmap_phys_addr_range(pgoff, len))
+		return (unsigned long) -EINVAL;
+	return pgoff;
+}
+#endif
+
 static int mmap_mem(struct file * file, struct vm_area_struct * vma)
 {
 	size_t size = vma->vm_end - vma->vm_start;
@@ -245,6 +258,12 @@ static int mmap_mem(struct file * file, 
 	if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
 		return -EINVAL;
 
+#ifndef CONFIG_MMU
+	/* can't do an in-place private mapping if there's no MMU */
+	if (!(vma->vm_flags & VM_MAYSHARE))
+		return -ENOSYS;
+#endif
+
 	vma->vm_page_prot = phys_mem_access_prot(file, vma->vm_pgoff,
 						 size,
 						 vma->vm_page_prot);
@@ -782,6 +801,9 @@ static const struct file_operations mem_
 	.write		= write_mem,
 	.mmap		= mmap_mem,
 	.open		= open_mem,
+#ifndef CONFIG_MMU
+	.get_unmapped_area = get_unmapped_area_mem,
+#endif
 };
 
 static const struct file_operations kmem_fops = {
@@ -790,6 +812,9 @@ static const struct file_operations kmem
 	.write		= write_kmem,
 	.mmap		= mmap_kmem,
 	.open		= open_kmem,
+#ifndef CONFIG_MMU
+	.get_unmapped_area = get_unmapped_area_mem,
+#endif
 };
 
 static const struct file_operations null_fops = {
@@ -815,10 +840,25 @@ static const struct file_operations zero
 	.mmap		= mmap_zero,
 };
 
+/*
+ * capabilities for /dev/zero
+ * - permits private mappings, "copies" are taken of the source of zeros
+ */
 static struct backing_dev_info zero_bdi = {
 	.capabilities	= BDI_CAP_MAP_COPY,
 };
 
+/*
+ * capabilities for /dev/mem and /dev/kmem
+ * - permits shared mmap for read, write and/or exec
+ * - does not permit private mmap (add BDI_CAP_MAP_COPY to permit this)
+ */
+static struct backing_dev_info mem_bdi = {
+	.capabilities	= (BDI_CAP_MAP_DIRECT |
+			   BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP |
+			   BDI_CAP_EXEC_MAP),
+};
+
 static const struct file_operations full_fops = {
 	.llseek		= full_lseek,
 	.read		= read_full,
@@ -861,9 +901,11 @@ static int memory_open(struct inode * in
 {
 	switch (iminor(inode)) {
 		case 1:
+			filp->f_mapping->backing_dev_info = &mem_bdi;
 			filp->f_op = &mem_fops;
 			break;
 		case 2:
+			filp->f_mapping->backing_dev_info = &mem_bdi;
 			filp->f_op = &kmem_fops;
 			break;
 		case 3:

[PATCH 2/2] NOMMU: Check that access_process_vm() has a valid target

[David Howells]

From: David Howells <dhowells@redhat.com>

Check that access_process_vm() is accessing a valid mapping in the target
process.

This limits ptrace() accesses and accesses through /proc/<pid>/maps to only
those regions actually mapped by a program.

Signed-Off-By: David Howells <dhowells@redhat.com>
---

 kernel/ptrace.c |   54 ------------------------------------------------------
 mm/memory.c     |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/nommu.c      |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 54 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 9a111f7..8aad033 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -241,60 +241,6 @@ int ptrace_detach(struct task_struct *ch
 	return 0;
 }
 
-/*
- * Access another process' address space.
- * Source/target buffer must be kernel space, 
- * Do not walk the page table directly, use get_user_pages
- */
-
-int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
-{
-	struct mm_struct *mm;
-	struct vm_area_struct *vma;
-	struct page *page;
-	void *old_buf = buf;
-
-	mm = get_task_mm(tsk);
-	if (!mm)
-		return 0;
-
-	down_read(&mm->mmap_sem);
-	/* ignore errors, just check how much was sucessfully transfered */
-	while (len) {
-		int bytes, ret, offset;
-		void *maddr;
-
-		ret = get_user_pages(tsk, mm, addr, 1,
-				write, 1, &page, &vma);
-		if (ret <= 0)
-			break;
-
-		bytes = len;
-		offset = addr & (PAGE_SIZE-1);
-		if (bytes > PAGE_SIZE-offset)
-			bytes = PAGE_SIZE-offset;
-
-		maddr = kmap(page);
-		if (write) {
-			copy_to_user_page(vma, page, addr,
-					  maddr + offset, buf, bytes);
-			set_page_dirty_lock(page);
-		} else {
-			copy_from_user_page(vma, page, addr,
-					    buf, maddr + offset, bytes);
-		}
-		kunmap(page);
-		page_cache_release(page);
-		len -= bytes;
-		buf += bytes;
-		addr += bytes;
-	}
-	up_read(&mm->mmap_sem);
-	mmput(mm);
-	
-	return buf - old_buf;
-}
-
 int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
 {
 	int copied = 0;
diff --git a/mm/memory.c b/mm/memory.c
index 109e986..f915984 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2505,3 +2505,56 @@ #endif
 }
 
 #endif	/* __HAVE_ARCH_GATE_AREA */
+
+/*
+ * Access another process' address space.
+ * Source/target buffer must be kernel space, 
+ * Do not walk the page table directly, use get_user_pages
+ */
+int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
+{
+	struct mm_struct *mm;
+	struct vm_area_struct *vma;
+	struct page *page;
+	void *old_buf = buf;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return 0;
+
+	down_read(&mm->mmap_sem);
+	/* ignore errors, just check how much was sucessfully transfered */
+	while (len) {
+		int bytes, ret, offset;
+		void *maddr;
+
+		ret = get_user_pages(tsk, mm, addr, 1,
+				write, 1, &page, &vma);
+		if (ret <= 0)
+			break;
+
+		bytes = len;
+		offset = addr & (PAGE_SIZE-1);
+		if (bytes > PAGE_SIZE-offset)
+			bytes = PAGE_SIZE-offset;
+
+		maddr = kmap(page);
+		if (write) {
+			copy_to_user_page(vma, page, addr,
+					  maddr + offset, buf, bytes);
+			set_page_dirty_lock(page);
+		} else {
+			copy_from_user_page(vma, page, addr,
+					    buf, maddr + offset, bytes);
+		}
+		kunmap(page);
+		page_cache_release(page);
+		len -= bytes;
+		buf += bytes;
+		addr += bytes;
+	}
+	up_read(&mm->mmap_sem);
+	mmput(mm);
+	
+	return buf - old_buf;
+}
diff --git a/mm/nommu.c b/mm/nommu.c
index c576df7..663ec1c 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1206,3 +1206,50 @@ struct page *filemap_nopage(struct vm_ar
 	BUG();
 	return NULL;
 }
+
+/*
+ * Access another process' address space.
+ * - source/target buffer must be kernel space
+ */
+int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
+{
+	struct vm_list_struct *vml;
+	struct vm_area_struct *vma;
+	struct mm_struct *mm;
+
+	if (addr + len < addr)
+		return 0;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return 0;
+
+	down_read(&mm->mmap_sem);
+
+	/* the access must start within one of the target process's mappings */
+	for (vml = mm->context.vmlist; vml; vml = vml->next)
+		if (addr >= vml->vma->vm_start && addr < vml->vma->vm_end)
+			break;
+
+	if (vml) {
+		vma = vml->vma;
+
+		/* don't overrun this mapping */
+		if (addr + len >= vma->vm_end)
+			len = vma->vm_end - addr;
+
+		/* only read or write mappings where it is permitted */
+		if (write && vma->vm_flags & VM_WRITE)
+			len -= copy_to_user((void *) addr, buf, len);
+		else if (!write && vma->vm_flags & VM_READ)
+			len -= copy_from_user(buf, (void *) addr, len);
+		else
+			len = 0;
+	} else {
+		len = 0;
+	}
+
+	up_read(&mm->mmap_sem);
+	mmput(mm);
+	return len;
+}

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[Andrew Morton]

On Tue, 29 Aug 2006 18:59:49 +0100
David Howells <dhowells@redhat.com> wrote:

> From: David Howells <dhowells@redhat.com>
> 
> Set the backing device info capabilities for /dev/mem and /dev/kmem to permit
> direct sharing under no-MMU conditions.
> 
> Also comment the capabilities for /dev/zero.
> 
> Signed-Off-By: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/char/mem.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 917b204..4c29619 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -238,6 +238,19 @@ #endif
>  }
>  #endif
>  
> +#ifndef CONFIG_MMU
> +static unsigned long get_unmapped_area_mem(struct file *file,
> +					   unsigned long addr,
> +					   unsigned long len,
> +					   unsigned long pgoff,
> +					   unsigned long flags)
> +{
> +	if (!valid_mmap_phys_addr_range(pgoff, len))
> +		return (unsigned long) -EINVAL;
> +	return pgoff;
> +}

#else
#define get_unmapped_area_mem NULL
#endif

> @@ -782,6 +801,9 @@ static const struct file_operations mem_
>  	.write		= write_mem,
>  	.mmap		= mmap_mem,
>  	.open		= open_mem,
> +#ifndef CONFIG_MMU
> +	.get_unmapped_area = get_unmapped_area_mem,
> +#endif
>  };

zap ifdefs.

>  static const struct file_operations kmem_fops = {
> @@ -790,6 +812,9 @@ static const struct file_operations kmem
>  	.write		= write_kmem,
>  	.mmap		= mmap_kmem,
>  	.open		= open_kmem,
> +#ifndef CONFIG_MMU
> +	.get_unmapped_area = get_unmapped_area_mem,
> +#endif
>  };

Ditto.

> +/*
> + * capabilities for /dev/zero
> + * - permits private mappings, "copies" are taken of the source of zeros
> + */
>  static struct backing_dev_info zero_bdi = {
>  	.capabilities	= BDI_CAP_MAP_COPY,
>  };
>  
> +/*
> + * capabilities for /dev/mem and /dev/kmem
> + * - permits shared mmap for read, write and/or exec
> + * - does not permit private mmap (add BDI_CAP_MAP_COPY to permit this)
> + */
> +static struct backing_dev_info mem_bdi = {
> +	.capabilities	= (BDI_CAP_MAP_DIRECT |
> +			   BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP |
> +			   BDI_CAP_EXEC_MAP),
> +};

This changes behaviour, doesn't it?  But only for !CONFIG_MMU kernels?

Perhaps some additional commentary around this is needed.


>  static const struct file_operations full_fops = {
>  	.llseek		= full_lseek,
>  	.read		= read_full,
> @@ -861,9 +901,11 @@ static int memory_open(struct inode * in
>  {
>  	switch (iminor(inode)) {
>  		case 1:
> +			filp->f_mapping->backing_dev_info = &mem_bdi;
>  			filp->f_op = &mem_fops;
>  			break;
>  		case 2:
> +			filp->f_mapping->backing_dev_info = &mem_bdi;
>  			filp->f_op = &kmem_fops;
>  			break;
>  		case 3:

Perhaps one could make mem_bdi==NULL if !CONFIG_MMU, for a minor space
saving.

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> #else
> #define get_unmapped_area_mem NULL
> #endif

Blech.

Of course, I could just declare the new symbols weak, and stick
get_unmapped_area_mem() and mem_bdi in their own file which would be
conditional on !CONFIG_MMU.

> This changes behaviour, doesn't it?

Yes.

>  But only for !CONFIG_MMU kernels?

Yes.  For the moment, nothing in MMU world actually looks at these
capabilities, though perhaps they should.

> Perhaps some additional commentary around this is needed.

Perhaps... or perhaps it should have different capabilities if there's an MMU.

Is doing a private mapping of /dev/mem a valid thing to do anyway, even if
there is an MMU?

David

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[Andrew Morton]

On Tue, 29 Aug 2006 19:39:54 +0100
David Howells <dhowells@redhat.com> wrote:

> Andrew Morton <akpm@osdl.org> wrote:
> 
> > #else
> > #define get_unmapped_area_mem NULL
> > #endif
> 
> Blech.
> 
> Of course, I could just declare the new symbols weak, and stick
> get_unmapped_area_mem() and mem_bdi in their own file which would be
> conditional on !CONFIG_MMU.

Or you could use the approach I suggested, like wot everyone else does.

> > This changes behaviour, doesn't it?
> 
> Yes.
> 
> >  But only for !CONFIG_MMU kernels?
> 
> Yes.  For the moment, nothing in MMU world actually looks at these
> capabilities, though perhaps they should.
> 
> > Perhaps some additional commentary around this is needed.
> 
> Perhaps... or perhaps it should have different capabilities if there's an MMU.
> 
> Is doing a private mapping of /dev/mem a valid thing to do anyway, even if
> there is an MMU?

It would be strange, I guess.  But the important thing is to not change
behaviour.

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> Or you could use the approach I suggested, like wot everyone else does.

Ummm...  I don't recall ever coming across a construct like that in the
kernel.  That's not to say there isn't one, but if I did come across it, it
can't have been clear.

I have seen the use #ifdefs to selectively fill in an ops structure.  Take
Ext3 for example:

	struct inode_operations ext3_file_inode_operations = {
		.truncate	= ext3_truncate,
		.setattr	= ext3_setattr,
	#ifdef CONFIG_EXT3_FS_XATTR
		.setxattr	= generic_setxattr,
		.getxattr	= generic_getxattr,
		.listxattr	= ext3_listxattr,
		.removexattr	= generic_removexattr,
	#endif
		.permission	= ext3_permission,
	};

So, no, _not_ everyone else follows your suggestion.  This example makes it
instantly clear to anyone looking at it that those four ops are only used if
Ext3 is configured to use xattrs.  Anything else lacks clarity.

> > Is doing a private mapping of /dev/mem a valid thing to do anyway, even if
> > there is an MMU?
> 
> It would be strange, I guess.  But the important thing is to not change
> behaviour.

Yeah, okay.

David

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[Andrew Morton]

On Wed, 30 Aug 2006 09:38:42 +0100
David Howells <dhowells@redhat.com> wrote:

> > Or you could use the approach I suggested, like wot everyone else does.
> 
> Ummm...  I don't recall ever coming across a construct like that in the
> kernel.

box:/usr/src/linux-2.6.18-rc5> grep -r '^#define.*suspend[      ]*NULL' . | wc -l
75

We do it to reduce (and to localise) ifdefs.

Re: [PATCH 1/2] NOMMU: Set BDI capabilities for /dev/mem and /dev/kmem

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> Perhaps one could make mem_bdi==NULL if !CONFIG_MMU, for a minor space
> saving.

The problem is that takes the #ifdef-avoidance scheme a step too far.  If we
do that, the kernel will crash.  Observe the following:

  Breakpoint 1, memory_open (inode=0xc0f7da04, filp=0xc09d8ba0) at fs.h:635
  635		return MINOR(inode->i_rdev);
  (gdb) n
  634	{
  (gdb) 
  904				filp->f_mapping->backing_dev_info = &mem_bdi;
  (gdb) i sym filp->f_mapping->backing_dev_info
  default_backing_dev_info in section .data

Note how the BDI pointer is already set to the default which we then override.
We either need to fill in mem_bdi correctly for MMU or only override the BDI
pointer if !MMU.

Of course, that doesn't touch on the matter of the compiler not letting you do
&NULL, though I assume you meant make &mem_bdi == NULL.

Also, the default BDI is incorrect for /dev/mem and /dev/kmem since it does
not permit shared mappings, so I think we need mem_bdi anyway, though I should
perhaps make it more general, so that it can apply to all mappable chardevs.

David