Messages in this thread | | | Date | Tue, 15 Apr 2008 17:36:30 +0200 | From | Mikael Pettersson <> | Subject | Re: [PATCH] mm: Fix possible off-by-one in walk_pte_range() |
| |
Johannes Weiner writes: > 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 <hannes@saeurebad.de> > CC: Roel Kluin <12o3l@tiscali.nl> > CC: Andreas Schwab <schwab@suse.de> > CC: Matt Mackall <mpm@selenic.com> > CC: Andrew Morton <akpm@linux-foundation.org> > --- > > 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 :) > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c > index 1cf1417..cf3c004 100644 > --- a/mm/pagewalk.c > +++ b/mm/pagewalk.c > @@ -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++);
Instead of obfuscating the code by putting "&& pte++" in the condition (it will always be true in valid C), you should IMO rewrite the do-while as a for loop + break, like this:
for (;;) { // same body as before addr += PAGE_SIZE; if (addr == end) break; pte++; }
This makes the ordering constraints very clear.
| |