VM_IO fixes

PATCH In review Andrew Morton 7 participants 10 messages Feb 7, 2002 – Feb 8, 2002

Subsystems (11)

Networking Memory management Media (V4L/DVB) USB +7 more subsystems

lore

Share & tools

lore

Andrew Morton

author

2002-02-07 06:15 UTC #0 lore raw

PATCH 1/1 VM_IO fixes

We've had a few bugs recently where device drivers were forgetting to set vma->vm_flags:VM_IO in their mmap() methods.

This causes kernel deadlocks when applications which have the relevant device mapped try to dump core (the pagefault handler deadlocks on mmap_sem).

Failing to set VM_IO also causes kernel oopses when PTRACE_PEEKUSR tries to read the device mapping - the region has no page structs, but access_process_vm() acts as if it does.

This patch doesn't fix the PTRACE_PEEKUSR bug - for that we need
this patch as well as the patch Andrea, Manfred and I pieced
together - it's at http://www.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.4/2.4.18pre7aa2/00_get_user_pages-2
I understand that Manfred will be sending you a version of that patch.

The drivers which are forgetting to set VM_IO include a whole bunch of fbmem drivers, one or more AGP drivers and I don't know what else.

It's simpler to make VM_IO default to `on', and to only clear it in places where we know that it's safe to dump the memory to a core file, and where it's safe to PTRACE_PEEKUSR it. That's what this patch does. We only clear VM_IO in generic_file_mmap() and in ncp_mmap() and in coda_file_mmap().

Any filesystem which implements its own mmap() method, and which does not call generic_file_mmap() needs to be changed to clear VM_IO inside its mmap function. All in-kernel filesystems are OK, as is XFS. And the only breakage this can cause to out-of-kernel filesystems is failure to include mappings in core files, and inability to use PEEKUSR.

With this patch in place we can go through and remove the setting of VM_IO in all the drivers which _do_ remember to set it. But that's a cleanup which can await 2.4.19-pre.

mm/mmap.c +5 -0

@@ -534,6 +534,11 @@ munmap_back:
 		}
 		vma->vm_file = file;
 		get_file(file);
+		/*
+		 * Subdrivers can clear VM_IO if their mappings are
+		 * valid pages inside mem_map[]
+		 */
+		vma->vm_flags |= VM_IO;
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;

mm/filemap.c +1 -0

@@ -2111,6 +2111,7 @@ int generic_file_mmap(struct file * file
 		return -ENOEXEC;
 	UPDATE_ATIME(inode);
 	vma->vm_ops = &generic_file_vm_ops;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

fs/coda/file.c +1 -0

@@ -97,6 +97,7 @@ coda_file_mmap(struct file *file, struct
 	if (!cfile->f_op || !cfile->f_op->mmap)
 		return -ENODEV;
 
+	vma->vm_flags &= ~VM_IO;
 	down(&inode->i_sem);
 	ret = cfile->f_op->mmap(cfile, vma);
 	UPDATE_ATIME(inode);

fs/ncpfs/mmap.c +1 -0

@@ -119,5 +119,6 @@ int ncp_mmap(struct file *file, struct v
 	}
 
 	vma->vm_ops = &ncp_file_mmap;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

David S. Miller

2002-02-07 06:52 UTC #1 lore raw

From: Andrew Morton <[email protected]>
Date: Wed, 06 Feb 2002 22:14:28 -0800

I completely agree with this change.

Roman Zippel

2002-02-07 12:07 UTC #2 lore raw

In reply to Andrew Morton

Hi,

Quoted 6 lines · Andrew Morton

Any filesystem which implements its own mmap() method, and which does not call generic_file_mmap() needs to be changed to clear VM_IO inside its mmap function. All in-kernel filesystems are OK, as is XFS. And the only breakage this can cause to out-of-kernel filesystems is failure to include mappings in core files, and inability to use PEEKUSR.

You forgot shared memory via mm/shmem.c and ipc/shm.c. Another possibility is to test whether the driver provides a nopage function, as i/o areas are usually mapped with io_remap_page_range. The only exception I found are drivers under drivers/sgi/char, which use some really dirty tricks in their nopage function.

bye, Roman

Marcelo Tosatti

2002-02-07 17:45 UTC #3 lore raw

Quoted 4 lines · Roman Zippel

Hi,
On Wed, 6 Feb 2002, Andrew Morton wrote:

Quoted 8 lines

Quoted 6 lines
Any filesystem which implements its own mmap() method, and which does not call generic_file_mmap() needs to be changed to clear VM_IO inside its mmap function. All in-kernel filesystems are OK, as is XFS. And the only breakage this can cause to out-of-kernel filesystems is failure to include mappings in core files, and inability to use PEEKUSR.
You forgot shared memory via mm/shmem.c and ipc/shm.c.

Andrew, could you please send me an uptodated patch to fix that ?

Quoted 2 lines

Another possibility is to test whether the driver provides a nopage function, as i/o areas are usually mapped with io_remap_page_range.

Eek, thats too kludgy. ;)

Andrew Morton

author

2002-02-07 20:52 UTC #4 lore raw

In reply to Andrew Morton

Quoted 12 lines · Roman Zippel

Hi,
Quoted 6 lines · Andrew Morton
Any filesystem which implements its own mmap() method, and which does not call generic_file_mmap() needs to be changed to clear VM_IO inside its mmap function. All in-kernel filesystems are OK, as is XFS. And the only breakage this can cause to out-of-kernel filesystems is failure to include mappings in core files, and inability to use PEEKUSR.

Quoted 5 lines

You forgot shared memory via mm/shmem.c and ipc/shm.c. Another possibility is to test whether the driver provides a nopage function, as i/o areas are usually mapped with io_remap_page_range. The only exception I found are drivers under drivers/sgi/char, which use some really dirty tricks in their nopage function.

ugh. This just isn't working out.

Sure, inverting the default value of VM_IO is much safer, because the penalty for getting it wrong is broken coredumps and ptrace, rather than kernel crashes.

But there are still many areas which need changing, and thinking about. Stuff like ftape_mmap, sound_mmap, packet_mmap, mmap_mem, mmap_zero, ...

I'm wondering if we can automagically determine whether the mapping is safe to peek and coredump within the mmap and mremap functions themselves? All this would take is the ability to determine that the entire vma is backed by valid page structs.

Is there a fast and portable way of doing this?

Manfred Spraul

2002-02-07 21:34 UTC #5 lore raw

In reply to Andrew Morton

Quoted 5 lines · Andrew Morton

This patch doesn't fix the PTRACE_PEEKUSR bug - for that we need
this patch as well as the patch Andrea, Manfred and I pieced
together - it's at http://www.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.4/2.4.18pre7aa2/00_get_user_pages-2
I understand that Manfred will be sending you a version of that patch.

My patch is below.
The only difference between my and Andrea's version is one indentation and a new comment that warns about possible cache coherency problems.

Tested only on i386 with -pre9, the PTRACE_PEEKUSR oops is fixed (ok, I've tested pread from /proc/pid/mem, but that's the same code)

--
Manfred
<<<<<<<<<<<<<

mm/memory.c +23 -2

@@ -442,6 +442,13 @@
 	return page;
 }
 
+/*
+ * Please read Documentation/cachetlb.txt before using this function,
+ * accessing foreign memory spaces can cause cache coherency problems.
+ *
+ * Accessing a VM_IO area is even more dangerous, therefore the function
+ * fails if pages is != NULL and a VM_IO area is found.
+ */
 int get_user_pages(struct task_struct *tsk, struct mm_struct *mm, unsigned long start,
 		int len, int write, int force, struct page **pages, struct vm_area_struct **vmas)
 {

@@ -453,6 +460,7 @@
 		vma = find_extend_vma(mm, start);
 
 		if ( !vma ||
+		    (pages && vma->vm_flags & VM_IO) ||
 		    (!force &&
 		     	((write && (!(vma->vm_flags & VM_WRITE))) ||
 		    	 (!write && (!(vma->vm_flags & VM_READ))) ) )) {

@@ -486,8 +494,9 @@
 				/* FIXME: call the correct function,
 				 * depending on the type of the found page
 				 */
-				if (pages[i])
-					page_cache_get(pages[i]);
+				if (!pages[i])
+					goto bad_page;
+				page_cache_get(pages[i]);
 			}
 			if (vmas)
 				vmas[i] = vma;

@@ -497,7 +506,19 @@
 		} while(len && start < vma->vm_end);
 		spin_unlock(&mm->page_table_lock);
 	} while(len);
+out:
 	return i;
+
+	/*
+	 * We found an invalid page in the VMA.  Release all we have
+	 * so far and fail.
+	 */
+bad_page:
+	spin_unlock(&mm->page_table_lock);
+	while (i--)
+		page_cache_release(pages[i]);
+	i = -EFAULT;
+	goto out;
 }
 
 /*

<<<<<<<<<<

Andrea Arcangeli

2002-02-07 22:19 UTC #6 lore raw

In reply to Andrew Morton

Quoted 9 lines · Manfred Spraul

Quoted 5 lines · Andrew Morton
This patch doesn't fix the PTRACE_PEEKUSR bug - for that we need
this patch as well as the patch Andrea, Manfred and I pieced
together - it's at http://www.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.4/2.4.18pre7aa2/00_get_user_pages-2
I understand that Manfred will be sending you a version of that patch.
My patch is below.
The only difference between my and Andrea's version is one indentation and a new comment that warns about possible cache coherency problems.

Fine, thanks.

Andrea

Andrew Morton

author

2002-02-08 05:05 UTC #7 lore raw

In reply to Andrew Morton

Quoted 3 lines · Andrew Morton

Is there a fast and portable way of doing this?

Seems that there isn't.

Here's what I have. We allow coredumps and ptrace peeking of:

- file mappings
- sysv ipc mappings
- shmem mappings
- /dev/mem if the mapping is page_struct backed
- /dev/kmem
- /dev/zero mappings
- socket mappings
- ftape's dma buffer
- many audio card DMA buffers
- videodev dma buffer mappings

Is anything missing?

mm/mmap.c +5 -0

@@ -534,6 +534,11 @@ munmap_back:
 		}
 		vma->vm_file = file;
 		get_file(file);
+		/*
+		 * Subdrivers can clear VM_IO if their mappings are
+		 * valid pages inside mem_map[]
+		 */
+		vma->vm_flags |= VM_IO;
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;

mm/filemap.c +1 -0

@@ -2111,6 +2111,7 @@ int generic_file_mmap(struct file * file
 		return -ENOEXEC;
 	UPDATE_ATIME(inode);
 	vma->vm_ops = &generic_file_vm_ops;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

fs/ncpfs/mmap.c +1 -0

@@ -119,5 +119,6 @@ int ncp_mmap(struct file *file, struct v
 	}
 
 	vma->vm_ops = &ncp_file_mmap;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

ipc/shm.c +1 -0

@@ -159,6 +159,7 @@ static int shm_mmap(struct file * file, 
 {
 	UPDATE_ATIME(file->f_dentry->d_inode);
 	vma->vm_ops = &shm_vm_ops;
+	vma->vm_flags &= ~VM_IO;
 	shm_inc(file->f_dentry->d_inode->i_ino);
 	return 0;
 }

mm/shmem.c +1 -0

@@ -657,6 +657,7 @@ static int shmem_mmap(struct file * file
 		return -EACCES;
 	UPDATE_ATIME(inode);
 	vma->vm_ops = ops;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

drivers/char/mem.c +4 -3

@@ -198,10 +198,10 @@ static int mmap_mem(struct file * file, 
 	vma->vm_flags |= VM_RESERVED;
 
 	/*
-	 * Don't dump addresses that are not real memory to a core file.
+	 * Dump addresses that are real memory to a core file.
 	 */
-	if (offset >= __pa(high_memory) || (file->f_flags & O_SYNC))
-		vma->vm_flags |= VM_IO;
+	if (offset < __pa(high_memory) && !(file->f_flags & O_SYNC))
+		vma->vm_flags &= ~VM_IO;
 
 	if (remap_page_range(vma->vm_start, offset, vma->vm_end-vma->vm_start,
 			     vma->vm_page_prot))

@@ -473,6 +473,7 @@ static int mmap_zero(struct file * file,
 		return shmem_zero_setup(vma);
 	if (zeromap_page_range(vma->vm_start, vma->vm_end - vma->vm_start, vma->vm_page_prot))
 		return -EAGAIN;
+	vma->vm_flags &= ~VM_IO;
 	return 0;
 }

net/socket.c +1 -0

@@ -705,6 +705,7 @@ static int sock_mmap(struct file * file,
 {
 	struct socket *sock = socki_lookup(file->f_dentry->d_inode);
 
+	vma->vm_flags &= ~VM_IO;
 	return sock->ops->mmap(file, sock, vma);
 }

drivers/char/ftape/zftape/zftape-init.c +1 -0

@@ -204,6 +204,7 @@ static int  zft_mmap(struct file *filep,
 	sigfillset(&current->blocked);
 	lock_kernel();
 	if ((result = ftape_mmap(vma)) >= 0) {
+		vma->vm_flags &= ~VM_IO;
 #ifndef MSYNC_BUG_WAS_FIXED
 		static struct vm_operations_struct dummy = { NULL, };
 		vma->vm_ops = &dummy;

drivers/sound/soundcard.c +1 -0

@@ -481,6 +481,7 @@ static int sound_mmap(struct file *file,
 		return -EAGAIN;
 	}
 
+	vma->vm_flags &= ~VM_IO;
 	dmap->mapping_flags |= DMA_MAP_MAPPED;
 
 	if( audio_devs[dev]->d->mmap)

drivers/sound/es1370.c +1 -0

@@ -1377,6 +1377,7 @@ static int es1370_mmap(struct file *file
 		ret = -EAGAIN;
 		goto out;
 	}
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 out:
 	up(&s->sem);

drivers/sound/maestro.c +1 -0

@@ -2512,6 +2512,7 @@ static int ess_mmap(struct file *file, s
 	ret = -EAGAIN;
 	if (remap_page_range(vma->vm_start, virt_to_phys(db->rawbuf), size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 	ret = 0;
 out:

drivers/sound/trident.c +1 -0

@@ -2077,6 +2077,7 @@ static int trident_mmap(struct file *fil
 	if (remap_page_range(vma->vm_start, virt_to_phys(dmabuf->rawbuf),
 			     size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	dmabuf->mapped = 1;
 	ret = 0;
 out:

drivers/sound/es1371.c +2 -0

@@ -1566,6 +1566,7 @@ static int es1371_mmap(struct file *file
 		ret = -EAGAIN;
 		goto out;
 	}
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 out:
 	up(&s->sem);

@@ -2133,6 +2134,7 @@ static int es1371_mmap_dac(struct file *
 	ret = -EAGAIN;
 	if (remap_page_range(vma->vm_start, virt_to_phys(s->dma_dac1.rawbuf), size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	s->dma_dac1.mapped = 1;
 	ret = 0;
 out:

drivers/sound/cs4281/cs4281m.c +1 -0

@@ -3239,6 +3239,7 @@ static int cs4281_mmap(struct file *file
 	if (remap_page_range
 	    (vma->vm_start, virt_to_phys(db->rawbuf), size,
 	     vma->vm_page_prot)) return -EAGAIN;
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 
 	CS_DBGOUT(CS_FUNCTION | CS_PARMS | CS_OPEN, 4,

drivers/sound/cmpci.c +1 -0

@@ -1754,6 +1754,7 @@ static int cm_mmap(struct file *file, st
 	ret = -EINVAL;
 	if (remap_page_range(vma->vm_start, virt_to_phys(db->rawbuf), size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 	ret = 0;
 out:

drivers/sound/i810_audio.c +1 -0

@@ -1672,6 +1672,7 @@ static int i810_mmap(struct file *file, 
 	if (remap_page_range(vma->vm_start, virt_to_phys(dmabuf->rawbuf),
 			     size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	dmabuf->mapped = 1;
 	dmabuf->trigger = 0;
 	ret = 0;

drivers/sound/sonicvibes.c +1 -0

@@ -1551,6 +1551,7 @@ static int sv_mmap(struct file *file, st
 	ret = -EAGAIN;
 	if (remap_page_range(vma->vm_start, virt_to_phys(db->rawbuf), size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 	ret = 0;
 out:

drivers/sound/esssolo1.c +1 -0

@@ -1247,6 +1247,7 @@ static int solo1_mmap(struct file *file,
 	ret = -EAGAIN;
 	if (remap_page_range(vma->vm_start, virt_to_phys(db->rawbuf), size, vma->vm_page_prot))
 		goto out;
+	vma->vm_flags &= ~VM_IO;
 	db->mapped = 1;
 	ret = 0;
 out:

drivers/sound/maestro3.c +1 -0

@@ -1557,6 +1557,7 @@ static int m3_mmap(struct file *file, st
     ret = -EAGAIN;
     if (remap_page_range(vma->vm_start, virt_to_phys(db->rawbuf), size, vma->vm_page_prot))
         goto out;
+    vma->vm_flags &= ~VM_IO;
 
     db->mapped = 1;
     ret = 0;

drivers/sound/ymfpci.c +1 -0

@@ -1507,6 +1507,7 @@ static int ymf_mmap(struct file *file, s
 	if (remap_page_range(vma->vm_start, virt_to_phys(dmabuf->rawbuf),
 			     size, vma->vm_page_prot))
 		return -EAGAIN;
+	vma->vm_flags &= ~VM_IO;
 	dmabuf->mapped = 1;
 
 /* P3 */ printk(KERN_INFO "ymfpci: using memory mapped sound, untested!\n");

drivers/sound/cs46xx.c +1 -0

@@ -2468,6 +2468,7 @@ static int cs_mmap(struct file *file, st
 		ret = -EAGAIN;
 		goto out;
 	}
+	vma->vm_flags &= ~VM_IO;
 	dmabuf->mapped = 1;
 
 	CS_DBGOUT(CS_FUNCTION, 2, printk("cs46xx: cs_mmap()-\n") );

drivers/sound/rme96xx.c +1 -1

@@ -1429,7 +1429,7 @@ static int rm96xx_mmap(struct file *file
 
 
 /* this is the mapping */
-
+	vma->vm_flags &= ~VM_IO;
 	dma->mmapped = 1;
 	unlock_kernel();
 	return 0;

drivers/sound/ite8172.c +1 -0

@@ -1111,6 +1111,7 @@ static int it8172_mmap(struct file *file
 	unlock_kernel();
 	return -EAGAIN;
     }
+    vma->vm_flags &= ~VM_IO;
     db->mapped = 1;
     unlock_kernel();
     return 0;

drivers/usb/audio.c +1 -0

@@ -2341,6 +2341,7 @@ static int usb_audio_mmap(struct file *f
 	if (vma->vm_pgoff != 0)
 		goto out;
 
+	vma->vm_flags &= ~VM_IO;
 	ret = dmabuf_mmap(db,  vma->vm_start, vma->vm_end - vma->vm_start, vma->vm_page_prot);
 out:
 	unlock_kernel();

drivers/media/video/videodev.c +1 -0

@@ -208,6 +208,7 @@ int video_mmap(struct file *file, struct
 		lock_kernel();
 		ret = vfl->mmap(vfl, (char *)vma->vm_start, 
 				(unsigned long)(vma->vm_end-vma->vm_start));
+		vma->vm_flags &= ~VM_IO;
 		unlock_kernel();
 	}
 	return ret;

David S. Miller

2002-02-08 05:18 UTC #8 lore raw

From: Andrew Morton <[email protected]>
Date: Thu, 07 Feb 2002 21:05:05 -0800

Is anything missing?

ia64 perfmon mappings

David Mosberger

2002-02-08 05:25 UTC #9 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · David S. Miller

DaveM> From: Andrew Morton <[email protected]> Date: Thu, 07 Feb
DaveM> 2002 21:05:05 -0800

DaveM> Is anything missing?

DaveM> ia64 perfmon mappings

I forwarded your comment to Stephane, the ia64 perfmon maintainer.

Thanks,

	--david