mm: call pte_unmap() against a proper pte (Re: [PATCH 7/9] swap_info: swap count continuations)

----Original Message----
From: [email protected]
Date: 16/10/2009 7:30
To: "Hugh Dickins"<[email protected]>
Cc: "Andrew Morton"<[email protected]>, "Nitin Gupta"<ngupta@vflare.

Subj: [PATCH] mm: call pte_unmap() against a proper pte (Re: [PATCH 7/9]

Hi.

Quoted 6 lines

(quoted diff) +1 -0

@@ -645,6 +648,7 @@ static int copy_pte_range(struct mm_stru
 	spinlock_t *src_ptl, *dst_ptl;
 	int progress = 0;
 	int rss[2];
+	swp_entry_t entry = (swp_entry_t){0};
 

Quoted 4 lines

(quoted diff) +0 -0

 again:
 	rss[1] = rss[0] = 0;

@@ -671,7 +675,10 @@ again:
 			progress++;

Review again: · line 684

Quoted 10 lines

(quoted diff) +4 -1

 			continue;
 		}
-		copy_one_pte(dst_mm, src_mm, dst_pte, src_pte, vma, addr, rss);
+		entry.val = copy_one_pte(dst_mm, src_mm, dst_pte, src_pte,
+							vma, addr, rss);
+		if (entry.val)
+			break;
 		progress += 8;
 	} while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
 

It isn't the fault of only this patch, but I think breaking the loop without

dst_pte(and src_pte) would be bad behavior because we do unmap_pte(dst_pte -

(current copy_pte_range() already does it though... and this is only

when we break the first loop, IIUC.)

Quoted 4 lines

(quoted diff) +0 -0

@@ -681,6 +688,12 @@ again:
 	add_mm_rss(dst_mm, rss[0], rss[1]);
 	pte_unmap_unlock(dst_pte - 1, dst_ptl);

Review again: · line 699

Quoted 10 lines

(quoted diff) +6 -0

 	cond_resched();
+
+	if (entry.val) {
+		if (add_swap_count_continuation(entry, GFP_KERNEL) < 0)
+			return -ENOMEM;
+		progress = 0;
+	}
 	if (addr != end)
 		goto again;
 	return 0;

I've searched other places where we break a similar loop and do pte_unmap(pte

Current copy_pte_range() and apply_to_pte_range() has the same problem.

How about a patch like this ?
===
From: Daisuke Nishimura <[email protected]>

There are some places where we do like:

	pte = pte_map();
	do {
		(do break in some conditions)
	} while (pte++, ...);
	pte_unmap(pte - 1);

But if the loop breaks at the first loop, pte_unmap() unmaps invalid pte.

This patch is a fix for this problem.

Signed-off-by Daisuke Nishimura <[email protected]>