mm: improve remapping of vmalloc regions

PATCH In review Nick Piggin 2 participants 15 messages Apr 21, 2006

Subsystems (7)

Memory management Media (V4L/DVB) MM core Memory mapping +3 more subsystems

lore

Share & tools

lore

Nick Piggin

author

2006-04-21 06:43 UTC #0 lore raw

cover mm: improve remapping of vmalloc regions

OK, I've added my fixes, and removed vmalloc_to_pfn as per Christoph's suggestion.

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

Nick Piggin

author

2006-04-21 06:43 UTC #1 lore raw

PATCH 1/5 mm: remap_vmalloc_range

Add a remap_vmalloc_range and get rid of as many remap_pfn_range and vm_insert_page loops as possible.

remap_vmalloc_range can do a whole lot of nice range checking even if the caller gets it wrong (which it looks like one or two do).

Signed-off-by Nick Piggin <[email protected]>

drivers/media/video/cpia.c +3 ~~-15~~

Index: linux-2.6/drivers/media/video/cpia.c
===================================================================
@@ -3750,9 +3750,7 @@ static int cpia_ioctl(struct inode *inod
 static int cpia_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct video_device *dev = file->private_data;
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end - vma->vm_start;
-	unsigned long page, pos;
 	struct cam_data *cam = dev->priv;
 	int retval;

@@ -3778,19 +3776,9 @@ static int cpia_mmap(struct file *file, 
 		}
 	}
 
-	pos = (unsigned long)(cam->frame_buf);
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-			mutex_unlock(&cam->busy_lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
+	if (remap_vmalloc_range(vma, cam->frame_buf, 0)) {
+		mutex_unlock(&cam->busy_lock);
+		return -EAGAIN;
 	}
 
 	DBG("cpia_mmap: %ld\n", size);

drivers/media/video/meye.c +4 ~~-17~~

Index: linux-2.6/drivers/media/video/meye.c
===================================================================
@@ -1699,13 +1699,10 @@ static struct vm_operations_struct meye_
 
 static int meye_mmap(struct file *file, struct vm_area_struct *vma)
 {
-	unsigned long start = vma->vm_start;
 	unsigned long size = vma->vm_end - vma->vm_start;
-	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
-	unsigned long page, pos;
 
 	mutex_lock(&meye.lock);
-	if (size > gbuffers * gbufsize) {
+	if (size > gbuffers * gbufsize) { /* XXX: should be size + vm_pgoff? */
 		mutex_unlock(&meye.lock);
 		return -EINVAL;
 	}

@@ -1722,20 +1719,10 @@ static int meye_mmap(struct file *file, 
 		for (i = 0; i < gbuffers; i++)
 			meye.vma_use_count[i] = 0;
 	}
-	pos = (unsigned long)meye.grab_fbuffer + offset;
 
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-			mutex_unlock(&meye.lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
+	if (remap_vmalloc_range(vma, meye.grab_fbuffer, vma->vm_pgoff)) {
+		mutex_unlock(&meye.lock);
+		return -EAGAIN;
 	}
 
 	vma->vm_ops = &meye_vm_ops;

drivers/media/video/ov511.c +3 ~~-15~~

Index: linux-2.6/drivers/media/video/ov511.c
===================================================================
@@ -4616,10 +4616,8 @@ static int
 ov51x_v4l1_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct video_device *vdev = file->private_data;
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end - vma->vm_start;
 	struct usb_ov511 *ov = video_get_drvdata(vdev);
-	unsigned long page, pos;
 
 	if (ov->dev == NULL)
 		return -EIO;

@@ -4634,19 +4632,9 @@ ov51x_v4l1_mmap(struct file *file, struc
 	if (mutex_lock_interruptible(&ov->lock))
 		return -EINTR;
 
-	pos = (unsigned long)ov->fbuf;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-			mutex_unlock(&ov->lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
+	if (remap_vmalloc_range(vma, ov->fbuf, 0)) {
+		mutex_unlock(&ov->lock);
+		return -EAGAIN;
 	}
 
 	mutex_unlock(&ov->lock);

drivers/media/video/pwc/pwc-if.c +5 ~~-17~~

Index: linux-2.6/drivers/media/video/pwc/pwc-if.c
===================================================================
@@ -1602,28 +1602,16 @@ static int pwc_video_mmap(struct file *f
 {
 	struct video_device *vdev = file->private_data;
 	struct pwc_device *pdev;
-	unsigned long start = vma->vm_start;
-	unsigned long size  = vma->vm_end-vma->vm_start;
-	unsigned long page, pos;
 
-	Trace(TRACE_MEMORY, "mmap(0x%p, 0x%lx, %lu) called.\n", vdev, start, size);
+	/* XXX: should check ranges */
+	Trace(TRACE_MEMORY, "mmap(0x%p, 0x%lx, %lu) called.\n", vdev,
+				vma->vm_start, vma->vm_end - vma->vm_start);
 	pdev = vdev->priv;
 
 	vma->vm_flags |= VM_IO;
 
-	pos = (unsigned long)pdev->image_data;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-			return -EAGAIN;
-
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
-	}
+	if (remap_vmalloc_range(vma, pdev->image_data, 0))
+		return -EAGAIN;
 
 	return 0;
 }

drivers/media/video/se401.c +3 ~~-15~~

Index: linux-2.6/drivers/media/video/se401.c
===================================================================
@@ -1153,9 +1153,7 @@ static int se401_mmap(struct file *file,
 {
 	struct video_device *dev = file->private_data;
 	struct usb_se401 *se401 = (struct usb_se401 *)dev;
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end-vma->vm_start;
-	unsigned long page, pos;
 
 	mutex_lock(&se401->lock);

@@ -1167,19 +1165,9 @@ static int se401_mmap(struct file *file,
 		mutex_unlock(&se401->lock);
 		return -EINVAL;
 	}
-	pos = (unsigned long)se401->fbuf;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-			mutex_unlock(&se401->lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
+	if (remap_vmalloc_range(vma, se401->fbuf, 0)) {
+		mutex_unlock(&se401->lock);
+		return -EAGAIN;
 	}
 	mutex_unlock(&se401->lock);

drivers/media/video/stv680.c +3 ~~-15~~

Index: linux-2.6/drivers/media/video/stv680.c
===================================================================
@@ -1254,9 +1254,7 @@ static int stv680_mmap (struct file *fil
 {
 	struct video_device *dev = file->private_data;
 	struct usb_stv *stv680 = video_get_drvdata(dev);
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end-vma->vm_start;
-	unsigned long page, pos;
 
 	mutex_lock(&stv680->lock);

@@ -1269,19 +1267,9 @@ static int stv680_mmap (struct file *fil
 		mutex_unlock(&stv680->lock);
 		return -EINVAL;
 	}
-	pos = (unsigned long) stv680->fbuf;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-			mutex_unlock(&stv680->lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
+	if (remap_vmalloc_range(vma, stv680->fbuf, 0)) {
+		mutex_unlock(&stv680->lock);
+		return -EAGAIN;
 	}
 	mutex_unlock(&stv680->lock);

drivers/media/video/usbvideo/usbvideo.c +2 ~~-15~~

Index: linux-2.6/drivers/media/video/usbvideo/usbvideo.c
===================================================================
@@ -1068,9 +1068,7 @@ EXPORT_SYMBOL(usbvideo_RegisterVideoDevi
 static int usbvideo_v4l_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct uvd *uvd = file->private_data;
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end-vma->vm_start;
-	unsigned long page, pos;
 
 	if (!CAMERA_IS_OPERATIONAL(uvd))
 		return -EFAULT;

@@ -1078,19 +1076,8 @@ static int usbvideo_v4l_mmap(struct file
 	if (size > (((USBVIDEO_NUMFRAMES * uvd->max_frame_size) + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1)))
 		return -EINVAL;
 
-	pos = (unsigned long) uvd->fbuf;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-			return -EAGAIN;
-
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
-	}
+	if (remap_vmalloc_range(vma, uvd->fbuf, 0))
+		return -EAGAIN;
 
 	return 0;
 }

drivers/media/video/usbvideo/vicam.c +9 ~~-19~~

Index: linux-2.6/drivers/media/video/usbvideo/vicam.c
===================================================================
@@ -1029,8 +1029,6 @@ static int
 vicam_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	// TODO: allocate the raw frame buffer if necessary
-	unsigned long page, pos;
-	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end-vma->vm_start;
 	struct vicam_camera *cam = file->private_data;

@@ -1039,25 +1037,17 @@ vicam_mmap(struct file *file, struct vm_
 
 	DBG("vicam_mmap: %ld\n", size);
 
-	/* We let mmap allocate as much as it wants because Linux was adding 2048 bytes
-	 * to the size the application requested for mmap and it was screwing apps up.
-	 if (size > VICAM_FRAMES*VICAM_MAX_FRAME_SIZE)
-	 return -EINVAL;
+	/* We let mmap allocate as much as it wants because Linux was adding
+	 * 2048 bytes to the size the application requested for mmap and it was
+	 * screwing apps up.
+	 *
+	 * It shouldn't have been, so let's try this check again -np
 	 */
+	 if (size > VICAM_FRAMES*VICAM_MAX_FRAME_SIZE)
+		return -EINVAL;
 
-	pos = (unsigned long)cam->framebuf;
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-			return -EAGAIN;
-
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
-	}
+	if (remap_vmalloc_range(vma, cam->framebuf, 0))
+		return -EAGAIN;
 
 	return 0;
 }

drivers/media/video/w9968cf.c +3 ~~-13~~

Index: linux-2.6/drivers/media/video/w9968cf.c
===================================================================
@@ -2861,10 +2861,7 @@ static int w9968cf_mmap(struct file* fil
 	struct w9968cf_device* cam = (struct w9968cf_device*)
 				     video_get_drvdata(video_devdata(filp));
 	unsigned long vsize = vma->vm_end - vma->vm_start,
-		      psize = cam->nbuffers * cam->frame[0].size,
-		      start = vma->vm_start,
-		      pos = (unsigned long)cam->frame[0].buffer,
-		      page;
+		      psize = cam->nbuffers * cam->frame[0].size;
 
 	if (cam->disconnected) {
 		DBG(2, "Device not present")

@@ -2881,15 +2878,8 @@ static int w9968cf_mmap(struct file* fil
 	if (vsize > psize - (vma->vm_pgoff << PAGE_SHIFT))
 		return -EINVAL;
 
-	while (vsize > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page + vma->vm_pgoff,
-						PAGE_SIZE, vma->vm_page_prot))
-			return -EAGAIN;
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		vsize -= PAGE_SIZE;
-	}
+	if (remap_vmalloc_range(vma, cam->frame[0].buffer, vma->vm_pgoff))
+		return -EAGAIN;
 
 	DBG(5, "mmap method successfully called")
 	return 0;

arch/ia64/kernel/perfmon.c +1 ~~-20~~

Index: linux-2.6/arch/ia64/kernel/perfmon.c
===================================================================
@@ -2237,25 +2237,6 @@ pfm_free_fd(int fd, struct file *file)
 	put_unused_fd(fd);
 }
 
-static int
-pfm_remap_buffer(struct vm_area_struct *vma, unsigned long buf, unsigned long addr, unsigned long size)
-{
-	DPRINT(("CPU%d buf=0x%lx addr=0x%lx size=%ld\n", smp_processor_id(), buf, addr, size));
-
-	while (size > 0) {
-		unsigned long pfn = ia64_tpa(buf) >> PAGE_SHIFT;
-
-
-		if (remap_pfn_range(vma, addr, pfn, PAGE_SIZE, PAGE_READONLY))
-			return -ENOMEM;
-
-		addr  += PAGE_SIZE;
-		buf   += PAGE_SIZE;
-		size  -= PAGE_SIZE;
-	}
-	return 0;
-}
-
 /*
  * allocate a sampling buffer and remaps it into the user address space of the task
  */

@@ -2343,7 +2324,7 @@ pfm_smpl_buffer_alloc(struct task_struct
 	DPRINT(("aligned size=%ld, hdr=%p mapped @0x%lx\n", size, ctx->ctx_smpl_hdr, vma->vm_start));
 
 	/* can only be applied to current task, need to have the mm semaphore held when called */
-	if (pfm_remap_buffer(vma, (unsigned long)smpl_buf, vma->vm_start, size)) {
+	if (remap_vmalloc_range(vma, smpl_buf, 0)) {
 		DPRINT(("Can't remap buffer\n"));
 		up_write(&task->mm->mmap_sem);
 		goto error;

include/linux/vmalloc.h +3 -0

Index: linux-2.6/include/linux/vmalloc.h
===================================================================
@@ -45,6 +45,9 @@ extern void vfree(void *addr);
 extern void *vmap(struct page **pages, unsigned int count,
 			unsigned long flags, pgprot_t prot);
 extern void vunmap(void *addr);
+
+extern int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
+							unsigned long pgoff);
  
 /*
  *	Lowlevel-APIs (not for driver use!)

mm/vmalloc.c +71 -0

80 lines not on this page · raw message

drivers/media/video/em28xx/em28xx-video.c +6 ~~-13~~

Index: linux-2.6/drivers/media/video/em28xx/em28xx-video.c
===================================================================
@@ -34,6 +34,7 @@
 #include <linux/version.h>
 #include <linux/video_decoder.h>
 #include <linux/mutex.h>
+#include <linux/vmalloc.h>
 
 #include "em28xx.h"
 #include <media/tuner.h>

@@ -582,9 +583,7 @@ static struct vm_operations_struct em28x
  */
 static int em28xx_v4l2_mmap(struct file *filp, struct vm_area_struct *vma)
 {
-	unsigned long size = vma->vm_end - vma->vm_start,
-	    start = vma->vm_start;
-	void *pos;
+	unsigned long size = vma->vm_end - vma->vm_start;
 	u32 i;
 
 	struct em28xx *dev = filp->private_data;

@@ -625,16 +624,10 @@ static int em28xx_v4l2_mmap(struct file 
 	vma->vm_flags |= VM_IO;
 	vma->vm_flags |= VM_RESERVED;	/* avoid to swap out this VMA */
 
-	pos = dev->frame[i].bufmem;
-	while (size > 0) {	/* size is page-aligned */
-		if (vm_insert_page(vma, start, vmalloc_to_page(pos))) {
-			em28xx_videodbg("mmap: vm_insert_page failed\n");
-			mutex_unlock(&dev->fileop_lock);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		size -= PAGE_SIZE;
+	if (remap_vmalloc_range(vma, dev->frame[i].bufmem, 0)) {
+		em28xx_videodbg("mmap: remap_vmalloc_range failed\n");
+		mutex_unlock(&dev->fileop_lock);
+		return -EAGAIN;
 	}
 
 	vma->vm_ops = &em28xx_vm_ops;

drivers/media/video/et61x251/et61x251_core.c +4 ~~-12~~

Index: linux-2.6/drivers/media/video/et61x251/et61x251_core.c
===================================================================
@@ -1464,9 +1464,7 @@ static struct vm_operations_struct et61x
 static int et61x251_mmap(struct file* filp, struct vm_area_struct *vma)
 {
 	struct et61x251_device* cam = video_get_drvdata(video_devdata(filp));
-	unsigned long size = vma->vm_end - vma->vm_start,
-		      start = vma->vm_start;
-	void *pos;
+	unsigned long size = vma->vm_end - vma->vm_start;
 	u32 i;
 
 	if (mutex_lock_interruptible(&cam->fileop_mutex))

@@ -1503,15 +1501,9 @@ static int et61x251_mmap(struct file* fi
 	vma->vm_flags |= VM_IO;
 	vma->vm_flags |= VM_RESERVED;
 
-	pos = cam->frame[i].bufmem;
-	while (size > 0) { /* size is page-aligned */
-		if (vm_insert_page(vma, start, vmalloc_to_page(pos))) {
-			mutex_unlock(&cam->fileop_mutex);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		size -= PAGE_SIZE;
+	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
+		mutex_unlock(&cam->fileop_mutex);
+		return -EAGAIN;
 	}
 
 	vma->vm_ops = &et61x251_vm_ops;

drivers/media/video/sn9c102/sn9c102_core.c +4 ~~-12~~

Index: linux-2.6/drivers/media/video/sn9c102/sn9c102_core.c
===================================================================
@@ -1728,9 +1728,7 @@ static struct vm_operations_struct sn9c1
 static int sn9c102_mmap(struct file* filp, struct vm_area_struct *vma)
 {
 	struct sn9c102_device* cam = video_get_drvdata(video_devdata(filp));
-	unsigned long size = vma->vm_end - vma->vm_start,
-		      start = vma->vm_start;
-	void *pos;
+	unsigned long size = vma->vm_end - vma->vm_start;
 	u32 i;
 
 	if (mutex_lock_interruptible(&cam->fileop_mutex))

@@ -1767,15 +1765,9 @@ static int sn9c102_mmap(struct file* fil
 	vma->vm_flags |= VM_IO;
 	vma->vm_flags |= VM_RESERVED;
 
-	pos = cam->frame[i].bufmem;
-	while (size > 0) { /* size is page-aligned */
-		if (vm_insert_page(vma, start, vmalloc_to_page(pos))) {
-			mutex_unlock(&cam->fileop_mutex);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		size -= PAGE_SIZE;
+	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
+		mutex_unlock(&cam->fileop_mutex);
+		return -EAGAIN;
 	}
 
 	vma->vm_ops = &sn9c102_vm_ops;

drivers/media/video/zc0301/zc0301_core.c +4 ~~-12~~

Index: linux-2.6/drivers/media/video/zc0301/zc0301_core.c
===================================================================
@@ -929,9 +929,7 @@ static struct vm_operations_struct zc030
 static int zc0301_mmap(struct file* filp, struct vm_area_struct *vma)
 {
 	struct zc0301_device* cam = video_get_drvdata(video_devdata(filp));
-	unsigned long size = vma->vm_end - vma->vm_start,
-		      start = vma->vm_start;
-	void *pos;
+	unsigned long size = vma->vm_end - vma->vm_start;
 	u32 i;
 
 	if (mutex_lock_interruptible(&cam->fileop_mutex))

@@ -968,15 +966,9 @@ static int zc0301_mmap(struct file* filp
 	vma->vm_flags |= VM_IO;
 	vma->vm_flags |= VM_RESERVED;
 
-	pos = cam->frame[i].bufmem;
-	while (size > 0) { /* size is page-aligned */
-		if (vm_insert_page(vma, start, vmalloc_to_page(pos))) {
-			mutex_unlock(&cam->fileop_mutex);
-			return -EAGAIN;
-		}
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		size -= PAGE_SIZE;
+	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
+		mutex_unlock(&cam->fileop_mutex);
+		return -EAGAIN;
 	}
 
 	vma->vm_ops = &zc0301_vm_ops;

Nick Piggin

author

2006-04-21 06:43 UTC #10 lore raw

PATCH 2/5 mm: remove vmalloc_to_pfn

In reply to Nick Piggin

===================================================================

include/linux/mm.h +0 -1

@@ -1002,7 +1002,6 @@ static inline unsigned long vma_pages(st
 
 struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 struct page *vmalloc_to_page(void *addr);
-unsigned long vmalloc_to_pfn(void *addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn, unsigned long size, pgprot_t);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);

mm/memory.c +0 ~~-10~~

Index: linux-2.6/mm/memory.c
===================================================================
@@ -2394,16 +2394,6 @@ struct page * vmalloc_to_page(void * vma
 
 EXPORT_SYMBOL(vmalloc_to_page);
 
-/*
- * Map a vmalloc()-space virtual address to the physical page frame number.
- */
-unsigned long vmalloc_to_pfn(void * vmalloc_addr)
-{
-	return page_to_pfn(vmalloc_to_page(vmalloc_addr));
-}
-
-EXPORT_SYMBOL(vmalloc_to_pfn);
-
 #if !defined(__HAVE_ARCH_GATE_AREA)
 
 #if defined(AT_SYSINFO_EHDR)

Nick Piggin

author

2006-04-21 06:43 UTC #12 lore raw

PATCH 3/5 mm: remove rvmalloc

In reply to Nick Piggin

Some distros will be using 2.6.16ish kernels, which gives us a good chance to start getting rid of the SetPageReserved which has stuck around until now for its ability to catch paper bag type reference counting bugs.

Signed-off-by Nick Piggin <[email protected]>

drivers/ieee1394/dv1394-private.h +1 -1

Index: linux-2.6/drivers/ieee1394/dv1394-private.h
===================================================================
@@ -478,7 +478,7 @@ struct video_card {
 	/* support asynchronous I/O signals (SIGIO) */
 	struct fasync_struct *fasync;
 
-	/* the large, non-contiguous (rvmalloc()) ringbuffer for DV
+	/* the large, non-contiguous (vmalloc()) ringbuffer for DV
            data, exposed to user-space via mmap() */
 	unsigned long      dv_buf_size;
 	struct dma_region  dv_buf;

drivers/media/video/cpia.c +8 ~~-50~~

106 lines not on this page · raw message

drivers/media/video/cpia2/cpia2_core.c +3 ~~-43~~

64 lines not on this page · raw message

drivers/media/video/meye.c +4 ~~-41~~

73 lines not on this page · raw message

drivers/media/video/ov511.c +2 ~~-46~~

72 lines not on this page · raw message

drivers/media/video/pwc/pwc-if.c +2 ~~-43~~

63 lines not on this page · raw message

drivers/media/video/se401.c +2 ~~-46~~

66 lines not on this page · raw message

drivers/media/video/stv680.c +4 ~~-59~~

82 lines not on this page · raw message

drivers/media/video/usbvideo/usbvideo.c +5 ~~-45~~

86 lines not on this page · raw message

drivers/media/video/usbvideo/vicam.c +3 ~~-47~~

74 lines not on this page · raw message

drivers/media/video/w9968cf.c +6 ~~-52~~

104 lines not on this page · raw message

include/linux/vmalloc.h +2 -0

Index: linux-2.6/include/linux/vmalloc.h
===================================================================
@@ -32,9 +32,11 @@ struct vm_struct {
  *	Highlevel APIs for driver use
  */
 extern void *vmalloc(unsigned long size);
+extern void *vmalloc_user(unsigned long size);
 extern void *vmalloc_node(unsigned long size, int node);
 extern void *vmalloc_exec(unsigned long size);
 extern void *vmalloc_32(unsigned long size);
+extern void *vmalloc_32_user(unsigned long size);
 extern void *__vmalloc(unsigned long size, gfp_t gfp_mask, pgprot_t prot);
 extern void *__vmalloc_area(struct vm_struct *area, gfp_t gfp_mask,
 				pgprot_t prot);

kernel/power/snapshot.c +3 -4

Index: linux-2.6/kernel/power/snapshot.c
===================================================================
@@ -89,10 +89,9 @@ static int save_highmem_zone(struct zone
 			continue;
 		page = pfn_to_page(pfn);
 		/*
-		 * This condition results from rvmalloc() sans vmalloc_32()
-		 * and architectural memory reservations. This should be
-		 * corrected eventually when the cases giving rise to this
-		 * are better understood.
+		 * This condition results from architectural memory
+		 * reservations. This should be corrected eventually when the
+		 * cases giving rise to this are better understood.
 		 */
 		if (PageReserved(page))
 			continue;

mm/vmalloc.c +28 -2

Index: linux-2.6/mm/vmalloc.c
===================================================================
@@ -511,11 +511,24 @@ EXPORT_SYMBOL(__vmalloc);
  */
 void *vmalloc(unsigned long size)
 {
-       return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
+	return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 EXPORT_SYMBOL(vmalloc);
 
 /**
+ *	vmalloc_user  -  allocate virtually contiguous memory which has
+ *			   been zeroed so it can be mapped to userspace without
+ *			   leaking data.
+ *
+ *	@size:		allocation size
+ */
+void *vmalloc_user(unsigned long size)
+{
+	return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
+}
+EXPORT_SYMBOL(vmalloc_user);
+
+/**
  *	vmalloc_node  -  allocate memory on a specific node
  *
  *	@size:		allocation size

@@ -529,7 +542,7 @@ EXPORT_SYMBOL(vmalloc);
  */
 void *vmalloc_node(unsigned long size, int node)
 {
-       return __vmalloc_node(size, GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL, node);
+	return __vmalloc_node(size, GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL, node);
 }
 EXPORT_SYMBOL(vmalloc_node);

@@ -569,6 +582,19 @@ void *vmalloc_32(unsigned long size)
 }
 EXPORT_SYMBOL(vmalloc_32);
 
+/**
+ *	vmalloc_32_user  -  allocate virtually contiguous memory (32bit
+ *			      addressable) which is zeroed so it can be
+ *			      mapped to userspace without leaking data.
+ *
+ *	@size:		allocation size
+ */
+void *vmalloc_32_user(unsigned long size)
+{
+	return __vmalloc(size, GFP_KERNEL | __GFP_ZERO, PAGE_KERNEL);
+}
+EXPORT_SYMBOL(vmalloc_32_user);
+
 long vread(char *buf, char *addr, unsigned long count)
 {
 	struct vm_struct *tmp;

arch/ia64/kernel/perfmon.c +8 ~~-56~~

105 lines not on this page · raw message

Nick Piggin

author

2006-04-21 06:43 UTC #13 lore raw

PATCH 4/5 mm: extra remap_vmalloc_range check

In reply to Nick Piggin

Add a flag to ensure all remap_vmalloc_range memory has been allocated with the vmalloc _user variants, so data does not get leaked.

Signed-off-by Nick Piggin <[email protected]>

include/linux/vmalloc.h +1 -0

Index: linux-2.6/include/linux/vmalloc.h
===================================================================
@@ -8,6 +8,7 @@
 #define VM_IOREMAP	0x00000001	/* ioremap() and friends */
 #define VM_ALLOC	0x00000002	/* vmalloc() */
 #define VM_MAP		0x00000004	/* vmap()ed pages */
+#define VM_USERMAP	0x00000008	/* suitable for remap_vmalloc_range */
 /* bits [20..32] reserved for arch specific ioremap internals */
 
 /*

drivers/media/video/et61x251/et61x251_core.c +2 -1

Index: linux-2.6/drivers/media/video/et61x251/et61x251_core.c
===================================================================
@@ -133,7 +133,8 @@ et61x251_request_buffers(struct et61x251
 
 	cam->nbuffers = count;
 	while (cam->nbuffers > 0) {
-		if ((buff = vmalloc_32(cam->nbuffers * PAGE_ALIGN(imagesize))))
+		if ((buff = vmalloc_32_user(cam->nbuffers *
+						PAGE_ALIGN(imagesize))))
 			break;
 		cam->nbuffers--;
 	}

drivers/media/video/sn9c102/sn9c102_core.c +1 -1

Index: linux-2.6/drivers/media/video/sn9c102/sn9c102_core.c
===================================================================
@@ -149,7 +149,7 @@ sn9c102_request_buffers(struct sn9c102_d
 
 	cam->nbuffers = count;
 	while (cam->nbuffers > 0) {
-		if ((buff = vmalloc_32(cam->nbuffers * PAGE_ALIGN(imagesize))))
+		if ((buff = vmalloc_32_user(cam->nbuffers * PAGE_ALIGN(imagesize))))
 			break;
 		cam->nbuffers--;
 	}

drivers/media/video/zc0301/zc0301_core.c +1 -1

Index: linux-2.6/drivers/media/video/zc0301/zc0301_core.c
===================================================================
@@ -136,7 +136,7 @@ zc0301_request_buffers(struct zc0301_dev
 
 	cam->nbuffers = count;
 	while (cam->nbuffers > 0) {
-		if ((buff = vmalloc_32(cam->nbuffers * PAGE_ALIGN(imagesize))))
+		if ((buff = vmalloc_32_user(cam->nbuffers * PAGE_ALIGN(imagesize))))
 			break;
 		cam->nbuffers--;
 	}

mm/vmalloc.c +23 -2

Index: linux-2.6/mm/vmalloc.c
===================================================================
@@ -524,7 +524,16 @@ EXPORT_SYMBOL(vmalloc);
  */
 void *vmalloc_user(unsigned long size)
 {
-	return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
+	struct vm_struct *area;
+	void *ret;
+
+	ret = __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
+	write_lock(&vmlist_lock);
+	area = __find_vm_area(ret);
+	area->flags |= VM_USERMAP;
+	write_unlock(&vmlist_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(vmalloc_user);

@@ -591,7 +600,16 @@ EXPORT_SYMBOL(vmalloc_32);
  */
 void *vmalloc_32_user(unsigned long size)
 {
-	return __vmalloc(size, GFP_KERNEL | __GFP_ZERO, PAGE_KERNEL);
+	struct vm_struct *area;
+	void *ret;
+
+	ret = __vmalloc(size, GFP_KERNEL | __GFP_ZERO, PAGE_KERNEL);
+	write_lock(&vmlist_lock);
+	area = __find_vm_area(ret);
+	area->flags |= VM_USERMAP;
+	write_unlock(&vmlist_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(vmalloc_32_user);

@@ -700,6 +718,9 @@ int remap_vmalloc_range(struct vm_area_s
 	if (!area)
 		goto out_einval_locked;
 
+	if (!(area->flags & VM_USERMAP))
+		goto out_einval_locked;
+
 	if (usize + (pgoff << PAGE_SHIFT) > area->size - PAGE_SIZE)
 		goto out_einval_locked;
 	read_unlock(&vmlist_lock);

drivers/media/video/em28xx/em28xx-core.c +1 -3

Index: linux-2.6/drivers/media/video/em28xx/em28xx-core.c
===================================================================
@@ -79,10 +79,8 @@ u32 em28xx_request_buffers(struct em28xx
 
 	dev->num_frames = count;
 	while (dev->num_frames > 0) {
-		if ((buff = vmalloc_32(dev->num_frames * imagesize))) {
-			memset(buff, 0, dev->num_frames * imagesize);
+		if ((buff = vmalloc_32_user(dev->num_frames * imagesize)))
 			break;
-		}
 		dev->num_frames--;
 	}

Nick Piggin

author

2006-04-21 06:44 UTC #14 lore raw

PATCH 5/5 drivers: leave vm_flags alone

In reply to Nick Piggin

Get rid of some vm_flags twiddling from driver code. The net result of this + the last 4 patches is that all converted remap_vmalloc_range memory can support get_user_pages - do we want that? Can't hurt, can it?

Signed-off-by Nick Piggin <[email protected]>

drivers/media/video/em28xx/em28xx-video.c +0 -4

Index: linux-2.6/drivers/media/video/em28xx/em28xx-video.c
===================================================================
@@ -620,10 +620,6 @@ static int em28xx_v4l2_mmap(struct file 
 		return -EINVAL;
 	}
 
-	/* VM_IO is eventually going to replace PageReserved altogether */
-	vma->vm_flags |= VM_IO;
-	vma->vm_flags |= VM_RESERVED;	/* avoid to swap out this VMA */
-
 	if (remap_vmalloc_range(vma, dev->frame[i].bufmem, 0)) {
 		em28xx_videodbg("mmap: remap_vmalloc_range failed\n");
 		mutex_unlock(&dev->fileop_lock);

drivers/media/video/et61x251/et61x251_core.c +0 -3

Index: linux-2.6/drivers/media/video/et61x251/et61x251_core.c
===================================================================
@@ -1499,9 +1499,6 @@ static int et61x251_mmap(struct file* fi
 		return -EINVAL;
 	}
 
-	vma->vm_flags |= VM_IO;
-	vma->vm_flags |= VM_RESERVED;
-
 	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
 		mutex_unlock(&cam->fileop_mutex);
 		return -EAGAIN;

drivers/media/video/meye.c +0 -2

Index: linux-2.6/drivers/media/video/meye.c
===================================================================
@@ -1689,8 +1689,6 @@ static int meye_mmap(struct file *file, 
 	}
 
 	vma->vm_ops = &meye_vm_ops;
-	vma->vm_flags &= ~VM_IO;	/* not I/O memory */
-	vma->vm_flags |= VM_RESERVED;	/* avoid to swap out this VMA */
 	vma->vm_private_data = (void *) (offset / gbufsize);
 	meye_vm_open(vma);

drivers/media/video/pwc/pwc-if.c +0 -2

Index: linux-2.6/drivers/media/video/pwc/pwc-if.c
===================================================================
@@ -1567,8 +1567,6 @@ static int pwc_video_mmap(struct file *f
 				vma->vm_start, vma->vm_end - vma->vm_start);
 	pdev = vdev->priv;
 
-	vma->vm_flags |= VM_IO;
-
 	if (remap_vmalloc_range(vma, pdev->image_data, 0))
 		return -EAGAIN;

drivers/media/video/sn9c102/sn9c102_core.c +0 -3

Index: linux-2.6/drivers/media/video/sn9c102/sn9c102_core.c
===================================================================
@@ -1762,9 +1762,6 @@ static int sn9c102_mmap(struct file* fil
 		return -EINVAL;
 	}
 
-	vma->vm_flags |= VM_IO;
-	vma->vm_flags |= VM_RESERVED;
-
 	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
 		mutex_unlock(&cam->fileop_mutex);
 		return -EAGAIN;

drivers/media/video/zc0301/zc0301_core.c +0 -3

Index: linux-2.6/drivers/media/video/zc0301/zc0301_core.c
===================================================================
@@ -963,9 +963,6 @@ static int zc0301_mmap(struct file* filp
 		return -EINVAL;
 	}
 
-	vma->vm_flags |= VM_IO;
-	vma->vm_flags |= VM_RESERVED;
-
 	if (remap_vmalloc_range(vma, cam->frame[i].bufmem, 0)) {
 		mutex_unlock(&cam->fileop_mutex);
 		return -EAGAIN;

Nick Piggin

author

2006-04-21 06:55 UTC #11 lore raw

Re: [patch 2/5] mm: remove vmalloc_to_pfn

In reply to Nick Piggin

Sigh, missed nommu, sorry.

---

vmalloc_to_pfn no longer has any callers left in the kernel. It was previously used so remap_pfn_range can be used on vmalloc memory, but is obsolete with the introduction of remap_vmalloc_range.

Signed-off-by Nick Piggin <[email protected]>

include/linux/mm.h +0 -1

Index: linux-2.6/include/linux/mm.h
===================================================================
@@ -1002,7 +1002,6 @@ static inline unsigned long vma_pages(st
 
 struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 struct page *vmalloc_to_page(void *addr);
-unsigned long vmalloc_to_pfn(void *addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn, unsigned long size, pgprot_t);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);

mm/memory.c +0 ~~-10~~

Index: linux-2.6/mm/memory.c
===================================================================
@@ -2394,16 +2394,6 @@ struct page * vmalloc_to_page(void * vma
 
 EXPORT_SYMBOL(vmalloc_to_page);
 
-/*
- * Map a vmalloc()-space virtual address to the physical page frame number.
- */
-unsigned long vmalloc_to_pfn(void * vmalloc_addr)
-{
-	return page_to_pfn(vmalloc_to_page(vmalloc_addr));
-}
-
-EXPORT_SYMBOL(vmalloc_to_pfn);
-
 #if !defined(__HAVE_ARCH_GATE_AREA)
 
 #if defined(AT_SYSINFO_EHDR)

mm/nommu.c +0 -6

Index: linux-2.6/mm/nommu.c
===================================================================
@@ -167,12 +167,6 @@ struct page * vmalloc_to_page(void *addr
 	return virt_to_page(addr);
 }
 
-unsigned long vmalloc_to_pfn(void *addr)
-{
-	return page_to_pfn(virt_to_page(addr));
-}
-
-
 long vread(char *buf, char *addr, unsigned long count)
 {
 	memcpy(buf, addr, count);

Andrew Morton

2006-04-21 07:17 UTC #2 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Nick Piggin

Quoted 9 lines · Nick Piggin

Add a remap_vmalloc_range and get rid of as many remap_pfn_range and vm_insert_page loops as possible.
remap_vmalloc_range can do a whole lot of nice range checking even if the caller gets it wrong (which it looks like one or two do).
(quoted diff) +0 -1
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {

Review

Quoted 10 lines

(quoted diff) +0 ~~-10~~

-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page + vma->vm_pgoff,
-						PAGE_SIZE, vma->vm_page_prot))
-		if (remap_pfn_range(vma, addr, pfn, PAGE_SIZE, PAGE_READONLY))

You've removed the ability for the caller to set the pte protections - it now always uses vma->vm_page_prot.

please explain...

Andrew Morton

2006-04-21 07:29 UTC #6 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Nick Piggin

Quoted 15 lines · Nick Piggin

+/**
+ * remap_vmalloc_range - map vmalloc pages to userspace
+ *
+ * @vma: vma to cover (map full range of vma)
+ * @addr: vmalloc memory
+ * @pgoff: number of pages into addr before first page to map
+ * @returns: 0 for success, -Exxx on failure
+ *
+ * This function checks that addr is a valid vmalloc'ed area, and
+ * that it is big enough to cover the vma. Will return failure if
+ * that criteria isn't met.
+ *
+ * Similar to remap_pfn_range (see mm/memory.c)
+ */

When replacing calls to remap_pfn_rage() with calls to remap_valloc_range():

- remap_pfn_range() sets VM_IO|VM_RESERVED|VM_PFNMAP on the user's vma. remap_valloc_range() sets only VM_RESERVED.

- remap_pfn_range() has special handling for COWable user vma's, but remap_valloc_range() does not.

- are vma->vm_start and vma->vm_end always a multiple of PAGE_SIZE? (I always forget). If not, remap_valloc_range() looks a tad buggy.

pls explain.

- remap_valloc_range() can use ~PAGE_MASK, not PAGE_SIZE-1

- remap_valloc_range() would lose a whole buncha typecasts if you use the gcc pointer-arith-with-void* extension.

Nick Piggin

author

2006-04-21 07:33 UTC #3 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Andrew Morton

Quoted 24 lines · Andrew Morton

Quoted 9 lines · Nick Piggin
Add a remap_vmalloc_range and get rid of as many remap_pfn_range and vm_insert_page loops as possible.
remap_vmalloc_range can do a whole lot of nice range checking even if the caller gets it wrong (which it looks like one or two do).
(quoted diff) +0 -1
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
Review
Quoted 10 lines
(quoted diff) +0 ~~-10~~
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page + vma->vm_pgoff,
-						PAGE_SIZE, vma->vm_page_prot))
-		if (remap_pfn_range(vma, addr, pfn, PAGE_SIZE, PAGE_READONLY))
You've removed the ability for the caller to set the pte protections - it now always uses vma->vm_page_prot.

Quoted 1 line

please explain...

They should use vma->vm_page_prot?

The callers affected are the PAGE_SHARED ones (the others are unchanged). Isn't it correct to provide readonly mappings if userspace asks for it?

I assumed this is why Linus went this way too with the new vm_insert_page interface.

Nick Piggin

author

2006-04-21 07:41 UTC #7 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Andrew Morton

Quoted 5 lines · Andrew Morton

When replacing calls to remap_pfn_rage() with calls to remap_valloc_range():
- remap_pfn_range() sets VM_IO|VM_RESERVED|VM_PFNMAP on the user's vma. remap_valloc_range() sets only VM_RESERVED.

Yep, it doesn't use PFNMAPs (we can always user the underlying struct
page), nor is it IO space. The only change that should be seen, as
noted in patch 4/5, is that get_user_pages will work on all mappings now. I don't think there is a downside to this?

Quoted 3 lines

- remap_pfn_range() has special handling for COWable user vma's, but remap_valloc_range() does not.

That's only for PFNMAPs. COW should continue to work fine.

Quoted 3 lines

- are vma->vm_start and vma->vm_end always a multiple of PAGE_SIZE? (I always forget). If not, remap_valloc_range() looks a tad buggy.

I hope so.

Quoted 6 lines

pls explain.
- remap_valloc_range() can use ~PAGE_MASK, not PAGE_SIZE-1

I initially did that when coding the function in mm/memory.c, but when adding all the vmalloc range checking I tried to stick with vmalloc convention.

Quoted 3 lines

- remap_valloc_range() would lose a whole buncha typecasts if you use the gcc pointer-arith-with-void* extension.

Should I?

Nick Piggin

author

2006-04-21 07:43 UTC #8 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

Quoted 11 lines · Nick Piggin

Quoted 5 lines · Andrew Morton
When replacing calls to remap_pfn_rage() with calls to remap_valloc_range():
- remap_pfn_range() sets VM_IO|VM_RESERVED|VM_PFNMAP on the user's vma. remap_valloc_range() sets only VM_RESERVED.
Yep, it doesn't use PFNMAPs (we can always user the underlying struct
page), nor is it IO space. The only change that should be seen, as
noted in patch 4/5, is that get_user_pages will work on all mappings now. I don't think there is a downside to this?

By "change that should be seen", I mean the only change that userspace should see.

Andrew Morton

2006-04-21 07:59 UTC #4 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Nick Piggin

Quoted 26 lines · Nick Piggin

Quoted 24 lines · Andrew Morton
Quoted 9 lines · Nick Piggin
Add a remap_vmalloc_range and get rid of as many remap_pfn_range and vm_insert_page loops as possible.
remap_vmalloc_range can do a whole lot of nice range checking even if the caller gets it wrong (which it looks like one or two do).
(quoted diff) +0 -1
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
Review
Quoted 10 lines
(quoted diff) +0 ~~-10~~
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-		if (remap_pfn_range(vma, start, page + vma->vm_pgoff,
-						PAGE_SIZE, vma->vm_page_prot))
-		if (remap_pfn_range(vma, addr, pfn, PAGE_SIZE, PAGE_READONLY))
You've removed the ability for the caller to set the pte protections - it now always uses vma->vm_page_prot.

Quoted 6 lines

Quoted 1 line
please explain...
They should use vma->vm_page_prot?
The callers affected are the PAGE_SHARED ones (the others are unchanged). Isn't it correct to provide readonly mappings if userspace asks for it?

Dunno. I assume perfmon was using PAGE_READONLY because it doesn't want userspace altering the memory. One would think that this should be enforced at mmap()-time, and that mprotect() might be able to override it anyway. But I haven't looked that closely.

First impression is that there's some potential for breaking stuff in all this - convince me otherwise ;)

Andrew Morton

2006-04-21 08:02 UTC #9 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Nick Piggin

Quoted 6 lines · Nick Piggin

Quoted 4 lines
- are vma->vm_start and vma->vm_end always a multiple of PAGE_SIZE? (I always forget). If not, remap_valloc_range() looks a tad buggy.
I hope so.

do_mmap_pgoff() seems to dtrt. It makes one wonder why vm_start and vm_end aren't in units of PAGE_SIZE.

Quoted 5 lines

Quoted 3 lines
- remap_valloc_range() would lose a whole buncha typecasts if you use the gcc pointer-arith-with-void* extension.
Should I?

Well it's a gccism, and a good one. We have lots of gccisms and using this one here will neaten things up.

Nick Piggin

author

2006-04-21 08:06 UTC #5 lore raw

Re: [patch 1/5] mm: remap_vmalloc_range

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 5 lines · Nick Piggin
They should use vma->vm_page_prot?
The callers affected are the PAGE_SHARED ones (the others are unchanged). Isn't it correct to provide readonly mappings if userspace asks for it?
Dunno. I assume perfmon was using PAGE_READONLY because it doesn't want userspace altering the memory. One would think that this should be enforced at mmap()-time, and that mprotect() might be able to override it anyway. But I haven't looked that closely.

Oh yes definitely, and the perfmon case is OK, because it sets PAGE_READONLY in ->vm_page_prot. About the mprotect issue -- I'm not sure, this might be a problem for perfmon?

Quoted 2 lines

First impression is that there's some potential for breaking stuff in all this - convince me otherwise ;)

Well, the PAGE_SHARED guys might break if userspace if they expect to be able to write to readonly mappings. Unfortunate, but we could just put our feet down and tell them to fix the code?