By mistake the ITER_PIPE early-exit / warning from copy_from_iter() was
cargo-culted in _copy_to_iter_mcsafe() rather than a machine-check-safe
version of copy_to_iter_pipe().
Implement copy_pipe_to_iter_mcsafe() being careful to return the
indication of short copies due to a CPU exception.
Without this regression-fix all splice reads to dax-mode files fail.
Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
Cc: Al Viro <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Reported-by: Ross Zwisler <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Hi Ingo,
I'm submitting this fix back through the tip tree since the regression
originated through tip/x86/dax.
lib/iov_iter.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 7e43cd54c84c..d4c5de8d6fba 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -596,15 +596,44 @@ static unsigned long memcpy_mcsafe_to_page(struct page *page, size_t offset,
return ret;
}
+static size_t copy_pipe_to_iter_mcsafe(const void *addr, size_t bytes,
+ struct iov_iter *i)
+{
+ struct pipe_inode_info *pipe = i->pipe;
+ size_t n, off, xfer = 0;
+ int idx;
+
+ if (!sanity(i))
+ return 0;
+
+ bytes = n = push_pipe(i, bytes, &idx, &off);
+ if (unlikely(!n))
+ return 0;
+ for ( ; n; idx = next_idx(idx, pipe), off = 0) {
+ size_t chunk = min_t(size_t, n, PAGE_SIZE - off);
+ unsigned long rem;
+
+ rem = memcpy_mcsafe_to_page(pipe->bufs[idx].page, off, addr,
+ chunk);
+ i->idx = idx;
+ i->iov_offset = off + chunk - rem;
+ xfer += chunk - rem;
+ if (rem)
+ break;
+ n -= chunk;
+ addr += chunk;
+ }
+ i->count -= xfer;
+ return xfer;
+}
+
size_t _copy_to_iter_mcsafe(const void *addr, size_t bytes, struct iov_iter *i)
{
const char *from = addr;
unsigned long rem, curr_addr, s_addr = (unsigned long) addr;
- if (unlikely(i->type & ITER_PIPE)) {
- WARN_ON(1);
- return 0;
- }
+ if (unlikely(i->type & ITER_PIPE))
+ return copy_pipe_to_iter_mcsafe(addr, bytes, i);
if (iter_is_iovec(i))
might_fault();
iterate_and_advance(i, bytes, v,
On Sun, Jul 01, 2018 at 08:52:20AM -0700, Dan Williams wrote:
> By mistake the ITER_PIPE early-exit / warning from copy_from_iter() was
> cargo-culted in _copy_to_iter_mcsafe() rather than a machine-check-safe
> version of copy_to_iter_pipe().
>
> Implement copy_pipe_to_iter_mcsafe() being careful to return the
> indication of short copies due to a CPU exception.
>
> Without this regression-fix all splice reads to dax-mode files fail.
>
> Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
> Cc: Al Viro <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Tony Luck <[email protected]>
> Reported-by: Ross Zwisler <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Hi Ingo,
>
> I'm submitting this fix back through the tip tree since the regression
> originated through tip/x86/dax.
>
> lib/iov_iter.c | 37 +++++++++++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 4 deletions(-)
Hey Dan,
I retested the current linux/master with this patch applied, and XFS + DAX +
generic/323 still dies for me:
run fstests generic/323 at 2018-07-02 10:51:35
BUG: unable to handle kernel paging request at 00007f16dc001000
PGD 80000000bb71a067 P4D 80000000bb71a067 PUD bb71b067 PMD bb6e8067 PTE 0
Oops: 0002 [#1] PREEMPT SMP PTI
CPU: 1 PID: 1598 Comm: aio-last-ref-he Not tainted
4.18.0-rc3-00001-g5174f2f2b6e5 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014
RIP: 0010:__memcpy+0x12/0x20
Code: c3 e8 42 fb ff ff 48 8b 43 60 48 2b 43 50 88 43 4e 5b 5d c3 90 90 90
90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3
a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
RSP: 0018:ffffc90002783a60 EFLAGS: 00010246
RAX: 00007f16dc001000 RBX: ffff880151229000 RCX: 0000000000002000
RDX: 0000000000000000 RSI: ffff880151219000 RDI: 00007f16dc001000
RBP: ffffc90002783a68 R08: 0000004227a4083c R09: ffff880151219000
R10: ffffc90002783d40 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000010000 R14: ffffc90002783d18 R15: 0000000000010000
FS: 00007f16f1ec5700(0000) GS:ffff880114600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f16dc001000 CR3: 0000000035508000 CR4: 00000000000006e0
Call Trace:
? copyout_mcsafe+0x3e/0x60
_copy_to_iter_mcsafe+0x9e/0x4c0
? __lock_is_held+0x65/0xb0
pmem_copy_to_iter+0x17/0x20 [nd_pmem]
dax_copy_to_iter+0x49/0x70
dax_iomap_actor+0x1f8/0x280
? dax_iomap_rw+0x100/0x100
iomap_apply+0xb5/0x130
? dax_iomap_rw+0x100/0x100
dax_iomap_rw+0x95/0x100
? dax_iomap_rw+0x100/0x100
xfs_file_dax_read+0x83/0x1f0
xfs_file_read_iter+0xac/0xc0
aio_read+0x11f/0x1a0
? __might_fault+0x3e/0x90
io_submit_one+0x39d/0x5f0
? io_submit_one+0x39d/0x5f0
__x64_sys_io_submit+0xa1/0x280
do_syscall_64+0x65/0x220
? do_syscall_64+0x65/0x220
entry_SYSCALL_64_after_hwframe+0x49/0xbe
This failure looks identical to what I was hitting with the original bug
report.
- Ross
On Mon, Jul 2, 2018 at 9:58 AM, Ross Zwisler
<[email protected]> wrote:
> On Sun, Jul 01, 2018 at 08:52:20AM -0700, Dan Williams wrote:
>> By mistake the ITER_PIPE early-exit / warning from copy_from_iter() was
>> cargo-culted in _copy_to_iter_mcsafe() rather than a machine-check-safe
>> version of copy_to_iter_pipe().
>>
>> Implement copy_pipe_to_iter_mcsafe() being careful to return the
>> indication of short copies due to a CPU exception.
>>
>> Without this regression-fix all splice reads to dax-mode files fail.
>>
>> Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
>> Cc: Al Viro <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Andy Lutomirski <[email protected]>
>> Cc: Borislav Petkov <[email protected]>
>> Cc: Linus Torvalds <[email protected]>
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Tony Luck <[email protected]>
>> Reported-by: Ross Zwisler <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>> ---
>> Hi Ingo,
>>
>> I'm submitting this fix back through the tip tree since the regression
>> originated through tip/x86/dax.
>>
>> lib/iov_iter.c | 37 +++++++++++++++++++++++++++++++++----
>> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> Hey Dan,
>
> I retested the current linux/master with this patch applied, and XFS + DAX +
> generic/323 still dies for me:
>
> run fstests generic/323 at 2018-07-02 10:51:35
> BUG: unable to handle kernel paging request at 00007f16dc001000
> PGD 80000000bb71a067 P4D 80000000bb71a067 PUD bb71b067 PMD bb6e8067 PTE 0
> Oops: 0002 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 1598 Comm: aio-last-ref-he Not tainted
> 4.18.0-rc3-00001-g5174f2f2b6e5 #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014
> RIP: 0010:__memcpy+0x12/0x20
> Code: c3 e8 42 fb ff ff 48 8b 43 60 48 2b 43 50 88 43 4e 5b 5d c3 90 90 90
> 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3
> a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
> RSP: 0018:ffffc90002783a60 EFLAGS: 00010246
> RAX: 00007f16dc001000 RBX: ffff880151229000 RCX: 0000000000002000
> RDX: 0000000000000000 RSI: ffff880151219000 RDI: 00007f16dc001000
> RBP: ffffc90002783a68 R08: 0000004227a4083c R09: ffff880151219000
> R10: ffffc90002783d40 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000010000 R14: ffffc90002783d18 R15: 0000000000010000
> FS: 00007f16f1ec5700(0000) GS:ffff880114600000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f16dc001000 CR3: 0000000035508000 CR4: 00000000000006e0
> Call Trace:
> ? copyout_mcsafe+0x3e/0x60
> _copy_to_iter_mcsafe+0x9e/0x4c0
> ? __lock_is_held+0x65/0xb0
> pmem_copy_to_iter+0x17/0x20 [nd_pmem]
> dax_copy_to_iter+0x49/0x70
> dax_iomap_actor+0x1f8/0x280
> ? dax_iomap_rw+0x100/0x100
> iomap_apply+0xb5/0x130
> ? dax_iomap_rw+0x100/0x100
> dax_iomap_rw+0x95/0x100
> ? dax_iomap_rw+0x100/0x100
> xfs_file_dax_read+0x83/0x1f0
> xfs_file_read_iter+0xac/0xc0
> aio_read+0x11f/0x1a0
> ? __might_fault+0x3e/0x90
> io_submit_one+0x39d/0x5f0
> ? io_submit_one+0x39d/0x5f0
> __x64_sys_io_submit+0xa1/0x280
> do_syscall_64+0x65/0x220
> ? do_syscall_64+0x65/0x220
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> This failure looks identical to what I was hitting with the original bug
> report.
I see it now, my run was skipping generic/323.
All copy_to_user() implementations need to be prepared to handle faults
accessing userspace. The __memcpy_mcsafe() implementation handles both
mmu-faults on the user destination and machine-check-exceptions on the
source buffer. However, the memcpy_mcsafe() wrapper may silently
fallback to memcpy() depending on build options and cpu-capabilities.
Force copy_to_user_mcsafe() to always use __memcpy_mcsafe() when
available, and otherwise disable all of the copy_to_user_mcsafe()
infrastructure when __memcpy_mcsafe() is not available, i.e.
CONFIG_X86_MCE=n.
This fixes crashes of the form:
run fstests generic/323 at 2018-07-02 12:46:23
BUG: unable to handle kernel paging request at 00007f0d50001000
RIP: 0010:__memcpy+0x12/0x20
[..]
Call Trace:
copyout_mcsafe+0x3a/0x50
_copy_to_iter_mcsafe+0xa1/0x4a0
? dax_alive+0x30/0x50
dax_iomap_actor+0x1f9/0x280
? dax_iomap_rw+0x100/0x100
iomap_apply+0xba/0x130
? dax_iomap_rw+0x100/0x100
dax_iomap_rw+0x95/0x100
? dax_iomap_rw+0x100/0x100
xfs_file_dax_read+0x7b/0x1d0 [xfs]
xfs_file_read_iter+0xa7/0xc0 [xfs]
aio_read+0x11c/0x1a0
Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
Cc: Al Viro <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Reported-by: Ross Zwisler <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Hi Ingo,
Here is an additional copy_to_iter_mcsafe() fix to address the crash
reported by Ross. This now passes xfstests:generic/323 on my system.
arch/x86/Kconfig | 2 +-
arch/x86/include/asm/uaccess_64.h | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f1dbb4ee19d7..887d3a7bb646 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -63,7 +63,7 @@ config X86
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_REFCOUNT
select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64
- select ARCH_HAS_UACCESS_MCSAFE if X86_64
+ select ARCH_HAS_UACCESS_MCSAFE if X86_64 && X86_MCE
select ARCH_HAS_SET_MEMORY
select ARCH_HAS_SG_CHAIN
select ARCH_HAS_STRICT_KERNEL_RWX
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 62acb613114b..a9d637bc301d 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -52,7 +52,12 @@ copy_to_user_mcsafe(void *to, const void *from, unsigned len)
unsigned long ret;
__uaccess_begin();
- ret = memcpy_mcsafe(to, from, len);
+ /*
+ * Note, __memcpy_mcsafe() is explicitly used since it can
+ * handle exceptions / faults. memcpy_mcsafe() may fall back to
+ * memcpy() which lacks this handling.
+ */
+ ret = __memcpy_mcsafe(to, from, len);
__uaccess_end();
return ret;
}
* Dan Williams <[email protected]> wrote:
> Hi Ingo,
>
> Here is an additional copy_to_iter_mcsafe() fix to address the crash
> reported by Ross. This now passes xfstests:generic/323 on my system.
The lib/iov_iter fix would need an Acked-by from Al.
Thanks,
Ingo
On Mon, Jul 02, 2018 at 02:16:10PM -0700, Dan Williams wrote:
> All copy_to_user() implementations need to be prepared to handle faults
> accessing userspace. The __memcpy_mcsafe() implementation handles both
> mmu-faults on the user destination and machine-check-exceptions on the
> source buffer. However, the memcpy_mcsafe() wrapper may silently
> fallback to memcpy() depending on build options and cpu-capabilities.
>
> Force copy_to_user_mcsafe() to always use __memcpy_mcsafe() when
> available, and otherwise disable all of the copy_to_user_mcsafe()
> infrastructure when __memcpy_mcsafe() is not available, i.e.
> CONFIG_X86_MCE=n.
>
> This fixes crashes of the form:
> run fstests generic/323 at 2018-07-02 12:46:23
> BUG: unable to handle kernel paging request at 00007f0d50001000
> RIP: 0010:__memcpy+0x12/0x20
> [..]
> Call Trace:
> copyout_mcsafe+0x3a/0x50
> _copy_to_iter_mcsafe+0xa1/0x4a0
> ? dax_alive+0x30/0x50
> dax_iomap_actor+0x1f9/0x280
> ? dax_iomap_rw+0x100/0x100
> iomap_apply+0xba/0x130
> ? dax_iomap_rw+0x100/0x100
> dax_iomap_rw+0x95/0x100
> ? dax_iomap_rw+0x100/0x100
> xfs_file_dax_read+0x7b/0x1d0 [xfs]
> xfs_file_read_iter+0xa7/0xc0 [xfs]
> aio_read+0x11c/0x1a0
>
> Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
> Cc: Al Viro <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Tony Luck <[email protected]>
> Reported-by: Ross Zwisler <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Tested-by: Ross Zwisler <[email protected]>
On Sun, Jul 01, 2018 at 08:52:20AM -0700, Dan Williams wrote:
> By mistake the ITER_PIPE early-exit / warning from copy_from_iter() was
> cargo-culted in _copy_to_iter_mcsafe() rather than a machine-check-safe
> version of copy_to_iter_pipe().
>
> Implement copy_pipe_to_iter_mcsafe() being careful to return the
> indication of short copies due to a CPU exception.
>
> Without this regression-fix all splice reads to dax-mode files fail.
>
> Fixes: 8780356ef630 ("x86/asm/memcpy_mcsafe: Define copy_to_iter_mcsafe()")
> Cc: Al Viro <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Tony Luck <[email protected]>
> Reported-by: Ross Zwisler <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Tested-by: Ross Zwisler <[email protected]>
On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote:
>
> * Dan Williams <[email protected]> wrote:
>
> > Hi Ingo,
> >
> > Here is an additional copy_to_iter_mcsafe() fix to address the crash
> > reported by Ross. This now passes xfstests:generic/323 on my system.
>
> The lib/iov_iter fix would need an Acked-by from Al.
I can live with that; I would really like to see some documentation on
the copy_to_iter_mcsafe(), but that's a separate story. Incidentally,
are there any expectations of other callers appearing, or is that
(and copy_from_iter_flushcache()) YASingleConsumerAPI?
On Wed, Jul 4, 2018 at 3:38 PM, Al Viro <[email protected]> wrote:
> On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote:
>>
>> * Dan Williams <[email protected]> wrote:
>>
>> > Hi Ingo,
>> >
>> > Here is an additional copy_to_iter_mcsafe() fix to address the crash
>> > reported by Ross. This now passes xfstests:generic/323 on my system.
>>
>> The lib/iov_iter fix would need an Acked-by from Al.
>
> I can live with that; I would really like to see some documentation on
> the copy_to_iter_mcsafe(), but that's a separate story. Incidentally,
> are there any expectations of other callers appearing, or is that
> (and copy_from_iter_flushcache()) YASingleConsumerAPI?
The current cpu architectural detail preventing conversion of the
standard copy_to_iter() path to use the mcsafe flavor is that we can't
use REP MOV for fast copies and instead need to use a software loop so
that any exceptions are recoverable. When / if that is addressed, and
there is no performance difference between the two, it might make
sense to convert more users.
The _flushcache flavor, however, will likely stay limited to a single
consumer for the persistent memory use case.
* Dan Williams <[email protected]> wrote:
> On Wed, Jul 4, 2018 at 3:38 PM, Al Viro <[email protected]> wrote:
> > On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote:
> >>
> >> * Dan Williams <[email protected]> wrote:
> >>
> >> > Hi Ingo,
> >> >
> >> > Here is an additional copy_to_iter_mcsafe() fix to address the crash
> >> > reported by Ross. This now passes xfstests:generic/323 on my system.
> >>
> >> The lib/iov_iter fix would need an Acked-by from Al.
> >
> > I can live with that; I would really like to see some documentation on
> > the copy_to_iter_mcsafe(), but that's a separate story. Incidentally,
> > are there any expectations of other callers appearing, or is that
> > (and copy_from_iter_flushcache()) YASingleConsumerAPI?
>
> The current cpu architectural detail preventing conversion of the
> standard copy_to_iter() path to use the mcsafe flavor is that we can't
> use REP MOV for fast copies and instead need to use a software loop so
> that any exceptions are recoverable. When / if that is addressed, and
> there is no performance difference between the two, it might make
> sense to convert more users.
>
> The _flushcache flavor, however, will likely stay limited to a single
> consumer for the persistent memory use case.
Could you please add the API documentation Al asked for, and update the changlog
with the acks and tested-by's that people gave, and send out a v2 series?
Thanks!
Ingo