On Wed, 2013-08-28 at 08:18 -0700, Matt Craighead wrote:
> > I'm curious why we haven't seen it before
>
> I agree, it's slightly mysterious. We're hitting the bug on a 32-bit ARM system with 1GB or 2GB of memory; surely that's not very far off the beaten path.
>
>
> > Or maybe memmove is an architecture-specific implementation that happens to handle left-to-right overlapping copies correctly on common architectures?
>
> It's architecture specific, e.g.: http://lxr.linux.no/linux+v3.8/arch/arm/lib/memmove.S
>
> In this particular case, it decides that the memory is non-overlapping (by comparing the virtual addresses), so it incorrectly dispatches to memcpy(). At that point all bets are off in terms of the direction, chunk size, etc. of the copy.
>
> I guess I'd typically expect memcpy() to stride through memory forwards rather than backwards though. And since this function mandates "from < to", it seems like this would usually fail.
>
>
> For x86: http://lxr.linux.no/linux+v3.8/arch/x86/lib/memcpy_32.c
>
> In this case, it looks like it doesn't dispatch to memcpy(). It just determines forward/backwards and copies accordingly. So it would work as long as the "from < to" property was preserved.
>
> If I'm reading the code correctly, kmap_atomic appears to grow downward on x86:
> http://lxr.linux.no/linux+v3.8/arch/x86/mm/highmem_32.c
> http://lxr.linux.no/linux+v3.8/arch/x86/include/asm/fixmap.h
> etc.
>
> Since xdr.c calls kmap_atomic on *pgto first, then *pgfrom, the "from < to" property will therefore be preserved.
>
>
> Therefore, I suspect you might be able to reproduce the bug on (32-bit) x86 by doing any of the following:
> - swapping the order of those kmap_atomic/kunmap_atomic calls
> - modifying kmap_atomic to grow in the opposite direction
> - modifying memmove() to dispatch to memcpy() when the virtual regions are non-overlapping
>
> It's still possible that there is something else funny about how memory gets allocated in our setup that makes the bug more likely, but empirically, the bug didn't seem hard to hit. We didn't need to copy very much data/very many files over NFS before we typically got corruption, and the bug happened on a variety of specific platforms. It was (unsurprisingly) easier to reproduce with 2GB than with 1GB though.
Can you please test that the attached patch fixes the corruption?
Thanks
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
From e9250a18b0a215ec5d970bded81fde01ee8b97aa Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
Date: Wed, 28 Aug 2013 13:35:13 -0400
Subject: [PATCH] SUNRPC: Fix memory corruption issue on 32-bit highmem systems
Some architectures, such as ARM-32 do not return the same base address
when you call kmap_atomic() twice on the same page.
This causes problems for the memmove() call in the XDR helper routine
"_shift_data_right_pages()", since it defeats the detection of
overlapping memory ranges, and has been seen to corrupt memory.
The fix is to distinguish between the case where we're doing an
inter-page copy or not. In the former case of we know that the memory
ranges cannot possibly overlap, so we can additionally micro-optimise
by replacing memmove() with memcpy().
Reported-by: Mark Young <MYoung@xxxxxxxxxx>
Reported-by: Matt Craighead <mcraighead@xxxxxxxxxx>
Cc: Bruce Fields <bfields@xxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---
net/sunrpc/xdr.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 75edcfa..1504bb1 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -207,10 +207,13 @@ _shift_data_right_pages(struct page **pages, size_t pgto_base,
pgfrom_base -= copy;
vto = kmap_atomic(*pgto);
- vfrom = kmap_atomic(*pgfrom);
- memmove(vto + pgto_base, vfrom + pgfrom_base, copy);
+ if (*pgto != *pgfrom) {
+ vfrom = kmap_atomic(*pgfrom);
+ memcpy(vto + pgto_base, vfrom + pgfrom_base, copy);
+ kunmap_atomic(vfrom);
+ } else
+ memmove(vto + pgto_base, vto + pgfrom_base, copy);
flush_dcache_page(*pgto);
- kunmap_atomic(vfrom);
kunmap_atomic(vto);
} while ((len -= copy) != 0);
--
1.8.3.1
