| Message ID | 153056565378.3420.295180898468362039.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Dan Williams <dan.j.williams@intel.com> 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 <viro@zeniv.linux.org.uk> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Tony Luck <tony.luck@intel.com> > Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote: > > * Dan Williams <dan.j.williams@intel.com> 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 <viro@zeniv.linux.org.uk> wrote: > On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote: >> >> * Dan Williams <dan.j.williams@intel.com> 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 <dan.j.williams@intel.com> wrote: > On Wed, Jul 4, 2018 at 3:38 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Jul 03, 2018 at 10:30:40AM +0200, Ingo Molnar wrote: > >> > >> * Dan Williams <dan.j.williams@intel.com> 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
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; }
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 <viro@zeniv.linux.org.uk> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Borislav Petkov <bp@alien8.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Tony Luck <tony.luck@intel.com> Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- 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(-)