[PATCH] execute-in-place fixes

This patch includes feedback from Andrew and Christoph. Thanks for taking time to review. Use of empty_zero_page was eliminated to fix compilation for architectures that don't have it. This patch removes setting pages up-to-date in ext2_get_xip_page and all bug checks to verify that the page is indeed up to date. Setting the page state on mapping to userland is bogus. None of the code patchs involved with these pages in mm cares about the page state. still on my ToDo list: identify a place outside second extended where __inode_direct_access should reside Signed-off-by: Carsten Otte <cotte@de.ibm.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

[PATCH] execute-in-place fixes
This patch includes feedback from Andrew and Christoph. Thanks for taking time to review. Use of empty_zero_page was eliminated to fix compilation for architectures that don't have it. This patch removes setting pages up-to-date in ext2_get_xip_page and all bug checks to verify that the page is indeed up to date. Setting the page state on mapping to userland is bogus. None of the code patchs involved with these pages in mm cares about the page state. still on my ToDo list: identify a place outside second extended where __inode_direct_access should reside Signed-off-by: Carsten Otte <cotte@de.ibm.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Carsten Otte · Linus Torvalds
1 parent c5287ba132
Showing 2 changed files with 55 additions and 49 deletions Side-by-side Diff
fs/ext2/xip.c
mm/filemap_xip.c
@@ -15,67 +15,80 @@
 #include "xip.h"
  
 static inline int
-__inode_direct_access(struct inode *inode, sector_t sector, unsigned long *data) {
+__inode_direct_access(struct inode *inode, sector_t sector,
+		      unsigned long *data)
+{
 	BUG_ON(!inode->i_sb->s_bdev->bd_disk->fops->direct_access);
 	return inode->i_sb->s_bdev->bd_disk->fops
 		->direct_access(inode->i_sb->s_bdev,sector,data);
 }
  
+static inline int
+__ext2_get_sector(struct inode *inode, sector_t offset, int create,
+		   sector_t *result)
+{
+	struct buffer_head tmp;
+	int rc;
+
+	memset(&tmp, 0, sizeof(struct buffer_head));
+	rc = ext2_get_block(inode, offset/ (PAGE_SIZE/512), &tmp,
+			    create);
+	*result = tmp.b_blocknr;
+
+	/* did we get a sparse block (hole in the file)? */
+	if (!(*result)) {
+		BUG_ON(create);
+		rc = -ENODATA;
+	}
+
+	return rc;
+}
+
 int
-ext2_clear_xip_target(struct inode *inode, int block) {
-	sector_t sector = block*(PAGE_SIZE/512);
+ext2_clear_xip_target(struct inode *inode, int block)
+{
+	sector_t sector = block * (PAGE_SIZE/512);
 	unsigned long data;
 	int rc;
  
 	rc = __inode_direct_access(inode, sector, &data);
-	if (rc)
-		return rc;
-	clear_page((void*)data);
-	return 0;
+	if (!rc)
+		clear_page((void*)data);
+	return rc;
 }
  
 void ext2_xip_verify_sb(struct super_block *sb)
 {
 	struct ext2_sb_info *sbi = EXT2_SB(sb);
  
-	if ((sbi->s_mount_opt & EXT2_MOUNT_XIP)) {
-		if ((sb->s_bdev == NULL) ||
-			sb->s_bdev->bd_disk == NULL ||
-			sb->s_bdev->bd_disk->fops == NULL ||
-			sb->s_bdev->bd_disk->fops->direct_access == NULL) {
-			sbi->s_mount_opt &= (~EXT2_MOUNT_XIP);
-			ext2_warning(sb, __FUNCTION__,
-				"ignoring xip option - not supported by bdev");
-		}
+	if ((sbi->s_mount_opt & EXT2_MOUNT_XIP) &&
+	    !sb->s_bdev->bd_disk->fops->direct_access) {
+		sbi->s_mount_opt &= (~EXT2_MOUNT_XIP);
+		ext2_warning(sb, __FUNCTION__,
+			     "ignoring xip option - not supported by bdev");
 	}
 }
  
-struct page*
-ext2_get_xip_page(struct address_space *mapping, sector_t blockno,
+struct page *
+ext2_get_xip_page(struct address_space *mapping, sector_t offset,
 		   int create)
 {
 	int rc;
 	unsigned long data;
-	struct buffer_head tmp;
+	sector_t sector;
  
-	tmp.b_state = 0;
-	tmp.b_blocknr = 0;
-	rc = ext2_get_block(mapping->host, blockno/(PAGE_SIZE/512) , &tmp,
-				create);
+	/* first, retrieve the sector number */
+	rc = __ext2_get_sector(mapping->host, offset, create, &sector);
 	if (rc)
-		return ERR_PTR(rc);
-	if (tmp.b_blocknr == 0) {
-		/* SPARSE block */
-		BUG_ON(create);
-		return ERR_PTR(-ENODATA);
-	}
+		goto error;
  
+	/* retrieve address of the target data */
 	rc = __inode_direct_access
-		(mapping->host,tmp.b_blocknr*(PAGE_SIZE/512) ,&data);
-	if (rc)
-		return ERR_PTR(rc);
+		(mapping->host, sector * (PAGE_SIZE/512), &data);
+	if (!rc)
+		return virt_to_page(data);
  
-	SetPageUptodate(virt_to_page(data));
-	return virt_to_page(data);
+ error:
+	return ERR_PTR(rc);
 }
@@ -68,13 +68,12 @@
 		if (unlikely(IS_ERR(page))) {
 			if (PTR_ERR(page) == -ENODATA) {
 				/* sparse */
-				page = virt_to_page(empty_zero_page);
+				page = ZERO_PAGE(0);
 			} else {
 				desc->error = PTR_ERR(page);
 				goto out;
 			}
-		} else
-			BUG_ON(!PageUptodate(page));
+		}
  
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
@@ -84,8 +83,7 @@
 			flush_dcache_page(page);
  
 		/*
-		 * Ok, we have the page, and it's up-to-date, so
-		 * now we can copy it to user space...
+		 * Ok, we have the page, so now we can copy it to user space...
 		 *
 		 * The actor routine returns how many bytes were actually used..
 		 * NOTE! This may not be the same as how much of a user buffer
@@ -164,7 +162,7 @@
  * xip_write
  *
  * This function walks all vmas of the address_space and unmaps the
- * empty_zero_page when found at pgoff. Should it go in rmap.c?
+ * ZERO_PAGE when found at pgoff. Should it go in rmap.c?
  */
 static void
 __xip_unmap (struct address_space * mapping,
@@ -187,7 +185,7 @@
 		 * We need the page_table_lock to protect us from page faults,
 		 * munmap, fork, etc...
 		 */
-		pte = page_check_address(virt_to_page(empty_zero_page), mm,
+		pte = page_check_address(ZERO_PAGE(address), mm,
 					 address);
 		if (!IS_ERR(pte)) {
 			/* Nuke the page table entry. */
@@ -230,7 +228,6 @@
  
 	page = mapping->a_ops->get_xip_page(mapping, pgoff*(PAGE_SIZE/512), 0);
 	if (!IS_ERR(page)) {
-		BUG_ON(!PageUptodate(page));
 		return page;
 	}
 	if (PTR_ERR(page) != -ENODATA)
  
@@ -245,12 +242,11 @@
 			pgoff*(PAGE_SIZE/512), 1);
 		if (IS_ERR(page))
 			return NULL;
-		BUG_ON(!PageUptodate(page));
 		/* unmap page at pgoff from all other vmas */
 		__xip_unmap(mapping, pgoff);
 	} else {
-		/* not shared and writable, use empty_zero_page */
-		page = virt_to_page(empty_zero_page);
+		/* not shared and writable, use ZERO_PAGE() */
+		page = ZERO_PAGE(address);
 	}
  
 	return page;
@@ -319,8 +315,6 @@
 			break;
 		}
  
-		BUG_ON(!PageUptodate(page));
-
 		copied = filemap_copy_from_user(page, offset, buf, bytes);
 		flush_dcache_page(page);
 		if (likely(copied > 0)) {
@@ -435,8 +429,7 @@
 			return 0;
 		else
 			return PTR_ERR(page);
-	} else
-		BUG_ON(!PageUptodate(page));
+	}
 	kaddr = kmap_atomic(page, KM_USER0);
 	memset(kaddr + offset, 0, length);
 	kunmap_atomic(kaddr, KM_USER0);
...	...	@@ -15,67 +15,80 @@
15	15	#include "xip.h"
16	16
17	17	static inline int
18		-__inode_direct_access(struct inode inode, sector_t sector, unsigned long data) {
	18	+__inode_direct_access(struct inode *inode, sector_t sector,
	19	+ unsigned long *data)
	20	+{
19	21	BUG_ON(!inode->i_sb->s_bdev->bd_disk->fops->direct_access);
20	22	return inode->i_sb->s_bdev->bd_disk->fops
21	23	->direct_access(inode->i_sb->s_bdev,sector,data);
22	24	}
23	25
	26	+static inline int
	27	+__ext2_get_sector(struct inode *inode, sector_t offset, int create,
	28	+ sector_t *result)
	29	+{
	30	+ struct buffer_head tmp;
	31	+ int rc;
	32	+
	33	+ memset(&tmp, 0, sizeof(struct buffer_head));
	34	+ rc = ext2_get_block(inode, offset/ (PAGE_SIZE/512), &tmp,
	35	+ create);
	36	+ *result = tmp.b_blocknr;
	37	+
	38	+ /* did we get a sparse block (hole in the file)? */
	39	+ if (!(*result)) {
	40	+ BUG_ON(create);
	41	+ rc = -ENODATA;
	42	+ }
	43	+
	44	+ return rc;
	45	+}
	46	+
24	47	int
25		-ext2_clear_xip_target(struct inode *inode, int block) {
26		- sector_t sector = block*(PAGE_SIZE/512);
	48	+ext2_clear_xip_target(struct inode *inode, int block)
	49	+{
	50	+ sector_t sector = block * (PAGE_SIZE/512);
27	51	unsigned long data;
28	52	int rc;
29	53
30	54	rc = __inode_direct_access(inode, sector, &data);
31		- if (rc)
32		- return rc;
33		- clear_page((void*)data);
34		- return 0;
	55	+ if (!rc)
	56	+ clear_page((void*)data);
	57	+ return rc;
35	58	}
36	59
37	60	void ext2_xip_verify_sb(struct super_block *sb)
38	61	{
39	62	struct ext2_sb_info *sbi = EXT2_SB(sb);
40	63
41		- if ((sbi->s_mount_opt & EXT2_MOUNT_XIP)) {
42		- if ((sb->s_bdev == NULL) \|\|
43		- sb->s_bdev->bd_disk == NULL \|\|
44		- sb->s_bdev->bd_disk->fops == NULL \|\|
45		- sb->s_bdev->bd_disk->fops->direct_access == NULL) {
46		- sbi->s_mount_opt &= (~EXT2_MOUNT_XIP);
47		- ext2_warning(sb, __FUNCTION__,
48		- "ignoring xip option - not supported by bdev");
49		- }
	64	+ if ((sbi->s_mount_opt & EXT2_MOUNT_XIP) &&
	65	+ !sb->s_bdev->bd_disk->fops->direct_access) {
	66	+ sbi->s_mount_opt &= (~EXT2_MOUNT_XIP);
	67	+ ext2_warning(sb, __FUNCTION__,
	68	+ "ignoring xip option - not supported by bdev");
50	69	}
51	70	}
52	71
53		-struct page*
54		-ext2_get_xip_page(struct address_space *mapping, sector_t blockno,
	72	+struct page *
	73	+ext2_get_xip_page(struct address_space *mapping, sector_t offset,
55	74	int create)
56	75	{
57	76	int rc;
58	77	unsigned long data;
59		- struct buffer_head tmp;
	78	+ sector_t sector;
60	79
61		- tmp.b_state = 0;
62		- tmp.b_blocknr = 0;
63		- rc = ext2_get_block(mapping->host, blockno/(PAGE_SIZE/512) , &tmp,
64		- create);
	80	+ /* first, retrieve the sector number */
	81	+ rc = __ext2_get_sector(mapping->host, offset, create, &sector);
65	82	if (rc)
66		- return ERR_PTR(rc);
67		- if (tmp.b_blocknr == 0) {
68		- /* SPARSE block */
69		- BUG_ON(create);
70		- return ERR_PTR(-ENODATA);
71		- }
	83	+ goto error;
72	84
	85	+ /* retrieve address of the target data */
73	86	rc = __inode_direct_access
74		- (mapping->host,tmp.b_blocknr*(PAGE_SIZE/512) ,&data);
75		- if (rc)
76		- return ERR_PTR(rc);
	87	+ (mapping->host, sector * (PAGE_SIZE/512), &data);
	88	+ if (!rc)
	89	+ return virt_to_page(data);
77	90
78		- SetPageUptodate(virt_to_page(data));
79		- return virt_to_page(data);
	91	+ error:
	92	+ return ERR_PTR(rc);
80	93	}
...	...	@@ -68,13 +68,12 @@
68	68	if (unlikely(IS_ERR(page))) {
69	69	if (PTR_ERR(page) == -ENODATA) {
70	70	/* sparse */
71		- page = virt_to_page(empty_zero_page);
	71	+ page = ZERO_PAGE(0);
72	72	} else {
73	73	desc->error = PTR_ERR(page);
74	74	goto out;
75	75	}
76		- } else
77		- BUG_ON(!PageUptodate(page));
	76	+ }
78	77
79	78	/* If users can be writing to this page using arbitrary
80	79	* virtual addresses, take care about potential aliasing
...	...	@@ -84,8 +83,7 @@
84	83	flush_dcache_page(page);
85	84
86	85	/*
87		- * Ok, we have the page, and it's up-to-date, so
88		- * now we can copy it to user space...
	86	+ * Ok, we have the page, so now we can copy it to user space...
89	87	*
90	88	* The actor routine returns how many bytes were actually used..
91	89	* NOTE! This may not be the same as how much of a user buffer
...	...	@@ -164,7 +162,7 @@
164	162	* xip_write
165	163	*
166	164	* This function walks all vmas of the address_space and unmaps the
167		- * empty_zero_page when found at pgoff. Should it go in rmap.c?
	165	+ * ZERO_PAGE when found at pgoff. Should it go in rmap.c?
168	166	*/
169	167	static void
170	168	__xip_unmap (struct address_space * mapping,
...	...	@@ -187,7 +185,7 @@
187	185	* We need the page_table_lock to protect us from page faults,
188	186	* munmap, fork, etc...
189	187	*/
190		- pte = page_check_address(virt_to_page(empty_zero_page), mm,
	188	+ pte = page_check_address(ZERO_PAGE(address), mm,
191	189	address);
192	190	if (!IS_ERR(pte)) {
193	191	/* Nuke the page table entry. */
...	...	@@ -230,7 +228,6 @@
230	228
231	229	page = mapping->a_ops->get_xip_page(mapping, pgoff*(PAGE_SIZE/512), 0);
232	230	if (!IS_ERR(page)) {
233		- BUG_ON(!PageUptodate(page));
234	231	return page;
235	232	}
236	233	if (PTR_ERR(page) != -ENODATA)
237	234
...	...	@@ -245,12 +242,11 @@
245	242	pgoff*(PAGE_SIZE/512), 1);
246	243	if (IS_ERR(page))
247	244	return NULL;
248		- BUG_ON(!PageUptodate(page));
249	245	/* unmap page at pgoff from all other vmas */
250	246	__xip_unmap(mapping, pgoff);
251	247	} else {
252		- /* not shared and writable, use empty_zero_page */
253		- page = virt_to_page(empty_zero_page);
	248	+ /* not shared and writable, use ZERO_PAGE() */
	249	+ page = ZERO_PAGE(address);
254	250	}
255	251
256	252	return page;
...	...	@@ -319,8 +315,6 @@
319	315	break;
320	316	}
321	317
322		- BUG_ON(!PageUptodate(page));
323		-
324	318	copied = filemap_copy_from_user(page, offset, buf, bytes);
325	319	flush_dcache_page(page);
326	320	if (likely(copied > 0)) {
...	...	@@ -435,8 +429,7 @@
435	429	return 0;
436	430	else
437	431	return PTR_ERR(page);
438		- } else
439		- BUG_ON(!PageUptodate(page));
	432	+ }
440	433	kaddr = kmap_atomic(page, KM_USER0);
441	434	memset(kaddr + offset, 0, length);
442	435	kunmap_atomic(kaddr, KM_USER0);