ksm: check for ERR_PTR from follow_page()

PATCH In review Dan Carpenter 5 participants 7 messages Apr 21, 2010

Subsystems (4)

Memory management Kernel janitors MM core Memory management - KSM (Kernel Samepage Merging)

lore

Share & tools

lore

Dan Carpenter

author

2010-04-21 10:27 UTC #0 lore raw

PATCH 1/1 ksm: check for ERR_PTR from follow_page()

The follow_page() function can potentially return -EFAULT so I added checks for this.

Also I silenced an uninitialized variable warning on my version of gcc (version 4.3.2).

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not very familiar with this code, so handle with care.

mm/ksm.c +6 -6

index 8cdfc2a..956880f 100644
@@ -365,7 +365,7 @@ static int break_ksm(struct vm_area_struct *vma, unsigned long addr)
 	do {
 		cond_resched();
 		page = follow_page(vma, addr, FOLL_GET);
-		if (!page)
+		if (IS_ERR_OR_NULL(page))
 			break;
 		if (PageKsm(page))
 			ret = handle_mm_fault(vma->vm_mm, vma, addr,

@@ -447,7 +447,7 @@ static struct page *get_mergeable_page(struct rmap_item *rmap_item)
 		goto out;
 
 	page = follow_page(vma, addr, FOLL_GET);
-	if (!page)
+	if (IS_ERR_OR_NULL(page))
 		goto out;
 	if (PageAnon(page)) {
 		flush_anon_page(vma, page, addr);

@@ -1086,7 +1086,7 @@ struct rmap_item *unstable_tree_search_insert(struct rmap_item *rmap_item,
 		cond_resched();
 		tree_rmap_item = rb_entry(*new, struct rmap_item, node);
 		tree_page = get_mergeable_page(tree_rmap_item);
-		if (!tree_page)
+		if (IS_ERR_OR_NULL(tree_page))
 			return NULL;
 
 		/*

@@ -1294,7 +1294,7 @@ next_mm:
 			if (ksm_test_exit(mm))
 				break;
 			*page = follow_page(vma, ksm_scan.address, FOLL_GET);
-			if (*page && PageAnon(*page)) {
+			if (!IS_ERR_OR_NULL(*page) && PageAnon(*page)) {
 				flush_anon_page(vma, *page, ksm_scan.address);
 				flush_dcache_page(*page);
 				rmap_item = get_next_rmap_item(slot,

@@ -1308,7 +1308,7 @@ next_mm:
 				up_read(&mm->mmap_sem);
 				return rmap_item;
 			}
-			if (*page)
+			if (!IS_ERR_OR_NULL(*page))
 				put_page(*page);
 			ksm_scan.address += PAGE_SIZE;
 			cond_resched();

@@ -1367,7 +1367,7 @@ next_mm:
 static void ksm_do_scan(unsigned int scan_npages)
 {
 	struct rmap_item *rmap_item;
-	struct page *page;
+	struct page *uninitialized_var(page);
 
 	while (scan_npages--) {
 		cond_resched();

Rik van Riel

Acked-by on v1 1/1

2010-04-21 15:24 UTC #1 lore raw

Quoted 7 lines · Dan Carpenter

The follow_page() function can potentially return -EFAULT so I added checks for this.
Also I silenced an uninitialized variable warning on my version of gcc (version 4.3.2).
Signed-off-by Dan Carpenter<[email protected]>

Acked-by Rik van Riel <[email protected]>

Andrea Arcangeli

2010-04-21 17:46 UTC #2 lore raw

Quoted 10 lines · Rik van Riel

Quoted 7 lines · Dan Carpenter
The follow_page() function can potentially return -EFAULT so I added checks for this.
Also I silenced an uninitialized variable warning on my version of gcc (version 4.3.2).
Signed-off-by Dan Carpenter<[email protected]>
Acked-by Rik van Riel <[email protected]>

  	    	while (!(page = follow_page(vma, start, foll_flags)))
  	    	{

gup only checks for null, so when exactly is follow_page going to return -EFAULT? It's not immediately clear.

Johannes Weiner

2010-04-21 20:53 UTC #3 lore raw

Quoted 10 lines · Andrea Arcangeli

Quoted 9 lines · Rik van Riel
Quoted 7 lines · Dan Carpenter
The follow_page() function can potentially return -EFAULT so I added checks for this.
Also I silenced an uninitialized variable warning on my version of gcc (version 4.3.2).
Signed-off-by Dan Carpenter<[email protected]>

Quoted 7 lines

Quoted 1 line
Acked-by Rik van Riel <[email protected]>
  	    	while (!(page = follow_page(vma, start, foll_flags)))
  	    	{
gup only checks for null, so when exactly is follow_page going to return -EFAULT? It's not immediately clear.

Check below that loop. If it returns non-null, the first check is whether it IS_ERR().

How about the below?

Hannes

---
From: Johannes Weiner <[email protected]>
Subject: mm: document follow_page()

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/memory.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

mm/memory.c +11 -2

index 833952d..119b7cc 100644
@@ -1227,8 +1227,17 @@ int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
 }
 EXPORT_SYMBOL_GPL(zap_vma_ptes);
 
-/*
- * Do a quick page-table lookup for a single page.
+/**
+ * follow_page - look up a page descriptor from a user-virtual address
+ * @vma: vm_area_struct mapping @address
+ * @address: virtual address to look up
+ * @flags: flags modifying lookup behaviour
+ *
+ * @flags can have FOLL_ flags set, defined in <linux/mm.h>
+ *
+ * Returns the mapped (struct page *), %NULL if no mapping exists, or
+ * an error pointer if there is a mapping to something not represented
+ * by a page descriptor (see also vm_normal_page()).
  */
 struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
 			unsigned int flags)

Signature · 1 line

1.7.0.2

Andrea Arcangeli

2010-04-21 20:58 UTC #4 lore raw

Quoted 2 lines · Johannes Weiner

Check below that loop. If it returns non-null, the first check is whether it IS_ERR().

Indeed.

Quoted 3 lines

+ * Returns the mapped (struct page *), %NULL if no mapping exists, or
+ * an error pointer if there is a mapping to something not represented
+ * by a page descriptor (see also vm_normal_page()).

where exactly in vm_normal_page? Note I already checked vm_normal_page before sending the prev email and I didn't immediately see. I search return and they all return NULL except the return pfn_to_page(pfn), so is pfn_to_page that returns -EFAULT (the implementations I checked don't but there are plenty that I didn't check...).

Johannes Weiner

2010-04-21 21:10 UTC #5 lore raw

Quoted 10 lines · Andrea Arcangeli

Quoted 3 lines · Johannes Weiner
+ * Returns the mapped (struct page *), %NULL if no mapping exists, or
+ * an error pointer if there is a mapping to something not represented
+ * by a page descriptor (see also vm_normal_page()).
where exactly in vm_normal_page? Note I already checked vm_normal_page before sending the prev email and I didn't immediately see. I search return and they all return NULL except the return pfn_to_page(pfn), so is pfn_to_page that returns -EFAULT (the implementations I checked don't but there are plenty that I didn't check...).

It's not vm_normal_page() that returns -EFAULT. It is follow_page() that translates NULL from vm_normal_page() into -EFAULT.

Izik Eidus

Acked-by on v1 1/1

2010-04-21 23:44 UTC #6 lore raw

In reply to Dan Carpenter

On Wed, 21 Apr 2010 12:27:59 +0200
Dan Carpenter <[email protected]> wrote:

Hello

Quoted 9 lines

The follow_page() function can potentially return -EFAULT so I added checks for this.
Also I silenced an uninitialized variable warning on my version of gcc (version 4.3.2).
Signed-off-by Dan Carpenter <[email protected]>
---
I'm not very familiar with this code, so handle with care.

Acked-by Izik Eidus <[email protected]>

Thanks.

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