pagewalk: don't pte_unmap(NULL) in walk_pte_range()

PATCH In review Roel Kluin 3 participants 5 messages Apr 9, 2008 – Apr 10, 2008

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Roel Kluin

author

2008-04-09 10:08 UTC #0 lore raw

PATCH 1/1 pagewalk: don't pte_unmap(NULL) in walk_pte_range()

This is right isn't it?
---
Don't pte_unmap a NULL pointer, but the previous.

Signed-off-by Roel Kluin <[email protected]>

---

mm/pagewalk.c +1 -1

index 1cf1417..6615f0b 100644
@@ -15,7 +15,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		       break;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 
-	pte_unmap(pte);
+	pte_unmap(pte - 1);
 	return err;
 }

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

Johannes Weiner

2008-04-09 13:30 UTC #1 lore raw

Hi,

Quoted 3 lines · Roel Kluin

This is right isn't it?
---
Don't pte_unmap a NULL pointer, but the previous.

Which NULL pointer?

Quoted 7 lines

Signed-off-by Roel Kluin <[email protected]>
---
mm/pagewalk.c +0 -0
index 1cf1417..6615f0b 100644

mm/pagewalk.c static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, · line 20

Quoted 7 lines

mm/pagewalk.c +1 -1

@@ -15,7 +15,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		       break;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 
-	pte_unmap(pte);
+	pte_unmap(pte - 1);
 	return err;
 }

This does not make any sense to me.

Hannes

Roel Kluin

author

2008-04-10 11:42 UTC #2 lore raw

Quoted 20 lines · Johannes Weiner

Hi,
Quoted 3 lines · Roel Kluin
This is right isn't it?
---
Don't pte_unmap a NULL pointer, but the previous.
Which NULL pointer?
Quoted 10 lines
Signed-off-by Roel Kluin <[email protected]>
---
mm/pagewalk.c +0 -0
index 1cf1417..6615f0b 100644
@@ -15,7 +15,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		       break;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 

Quoted 6 lines

mm/pagewalk.c static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, · line 20
Quoted 4 lines
mm/pagewalk.c +1 -1
-	pte_unmap(pte);
+	pte_unmap(pte - 1);
 	return err;
 }
This does not make any sense to me.

you are right, please ignore.

Quoted 1 line

Hannes

thanks,

Roel

Andreas Schwab

2008-04-10 12:09 UTC #3 lore raw

In reply to Johannes Weiner

Quoted 10 lines · Johannes Weiner

Quoted 10 lines

Signed-off-by Roel Kluin <[email protected]>

---

mm/pagewalk.c +0 -0

index 1cf1417..6615f0b 100644
@@ -15,7 +15,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		       break;
 	} while (pte++, addr += PAGE_SIZE, addr != end);

Quoted 6 lines

mm/pagewalk.c static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, · line 20
Quoted 4 lines
mm/pagewalk.c +1 -1
-	pte_unmap(pte);
+	pte_unmap(pte - 1);
 	return err;
 }
This does not make any sense to me.

There is something fishy here. If the loop ends because addr == end then pte has been incremented past the pmd page for addr, no?

Andreas.

Signature · 10 lines

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstrasse 5, 90409 Nurnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

Johannes Weiner

2008-04-10 14:01 UTC #4 lore raw

PATCH 1/1 Stay below upper pmd boundary on pte range walk

Hi,

Quoted 19 lines · Andreas Schwab

Quoted 10 lines · Johannes Weiner
Quoted 10 lines
Signed-off-by Roel Kluin <[email protected]>
---
mm/pagewalk.c +0 -0
index 1cf1417..6615f0b 100644
@@ -15,7 +15,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		       break;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 
Quoted 6 lines
mm/pagewalk.c static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, · line 20
Quoted 4 lines
mm/pagewalk.c +1 -1
-	pte_unmap(pte);
+	pte_unmap(pte - 1);
 	return err;
 }
This does not make any sense to me.

Quoted 2 lines

There is something fishy here. If the loop ends because addr == end then pte has been incremented past the pmd page for addr, no?

Whoops, yes. But Roel's fix breaks if the break is taken in the first iteration of the loop, because the pte is then out of the lower bounds of the pmd page. Please see attached fix.

Hannes

---

After the loop in walk_pte_range() pte might point to the first address after the pmd it walks. The pte_unmap() is then applied to something bad.

Spotted by Roel Kluin and Andreas Schwab.

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

---

A bug is unlikely, though. kunmap_atomic() looks up the kmap entry by map-type instead of the address the pte points. So the worst thing I could find with a quick grep was that a wrong TLB entry is being flushed. Still, the code is wrong :)

mm/pagewalk.c +1 -1

index 1cf1417..cf3c004 100644
@@ -13,7 +13,7 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 		err = walk->pte_entry(pte, addr, addr + PAGE_SIZE, private);
 		if (err)
 		       break;
-	} while (pte++, addr += PAGE_SIZE, addr != end);
+	} while (addr += PAGE_SIZE, addr != end && pte++);
 
 	pte_unmap(pte);
 	return err;