[PATCH] Fix buf in zeromap_pud_range() losing virtual address

[PATCH] Fix buf in zeromap_pud_range() losing virtual address

[Benjamin Herrenschmidt]

Hi !

This patch fixes a nasty bug that took us almost a week to track down on
ppc64, introduced by the 4L page table changes, and resulting in random
memory corruption. All archs that rely on a PTE page's struct page to
contain the mm & address (in mapping/index) will be affected.

zeromap_pud_range() is one of these page tables walking functions that
split the address into a base and an offset. It forgets to add back the
"base" when calling the lower level zeromap_pmd_range(), thus passing a
bogus virtual address. Most archs won't care, unless they do the above,
since the lower level can allocate a PTE page.

Kudo's to Michael Ellerman too who spent that week running tests after
tests to track it down, since the only way we managed to get it to show
up was after about 1 to 2h of LTP runs ...

(Note: We are in _urgent_ need to consolidate all those page table
walking functions, they all do things in a subtely different way, with
different checks (sometimes redudant) and inconsitent with each other,
even within a given set of them. Hopefully, Nick has some work in
progress there).
 
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/mm/memory.c
===================================================================
--- linux-work.orig/mm/memory.c	2005-02-16 14:51:37.000000000 +1100
+++ linux-work/mm/memory.c	2005-02-17 18:11:15.000000000 +1100
@@ -1041,7 +1041,8 @@
 		error = -ENOMEM;
 		if (!pmd)
 			break;
-		error = zeromap_pmd_range(mm, pmd, address, end - address, prot);
+		error = zeromap_pmd_range(mm, pmd, base + address,
+					  end - address, prot);
 		if (error)
 			break;
 		address = (address + PUD_SIZE) & PUD_MASK;

Re: [PATCH] Fix buf in zeromap_pud_range() losing virtual address

[Andi Kleen]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>
> zeromap_pud_range() is one of these page tables walking functions that
> split the address into a base and an offset. It forgets to add back the
> "base" when calling the lower level zeromap_pmd_range(), thus passing a
> bogus virtual address. Most archs won't care, unless they do the above,
> since the lower level can allocate a PTE page.

Hmm, there might be even more cases of this. I remember pondering
it when I did the original 4 level work (sometimes we discard higher level 
virtual address bits during walking)

> (Note: We are in _urgent_ need to consolidate all those page table
> walking functions, they all do things in a subtely different way, with
> different checks (sometimes redudant) and inconsitent with each other,
> even within a given set of them. Hopefully, Nick has some work in
> progress there).

I have. But it will just make them more similar, not completely consolidate
them into an iterator, because that's too hard/ugly to do efficiently.

-Andi

Re: [PATCH] Fix buf in zeromap_pud_range() losing virtual address

[Benjamin Herrenschmidt]

On Thu, 2005-02-17 at 09:33 +0100, Andi Kleen wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> >
> > zeromap_pud_range() is one of these page tables walking functions that
> > split the address into a base and an offset. It forgets to add back the
> > "base" when calling the lower level zeromap_pmd_range(), thus passing a
> > bogus virtual address. Most archs won't care, unless they do the above,
> > since the lower level can allocate a PTE page.
> 
> Hmm, there might be even more cases of this. I remember pondering
> it when I did the original 4 level work (sometimes we discard higher level 
> virtual address bits during walking)

I think I went through all of them, but I'll do again just in case ...

> > (Note: We are in _urgent_ need to consolidate all those page table
> > walking functions, they all do things in a subtely different way, with
> > different checks (sometimes redudant) and inconsitent with each other,
> > even within a given set of them. Hopefully, Nick has some work in
> > progress there).
> 
> I have. But it will just make them more similar, not completely consolidate
> them into an iterator, because that's too hard/ugly to do efficiently.

Hrm... I'm pretty sure half of the ones we have now are not fully
efficient, and they all do the exact same thing until they get all the
way down. The only real variation is wether to allocate on the way. Oh
well, nick has some bits too, I'll have a look at what he has already
done.

Ben.