Hi Matthew et al,
This series completes 5d8de293c224 ("vmcore: convert copy_oldmem_page()
to take an iov_iter") for s390.
Changes since v3:
- concurrent access to HSA and oldmem swap buffers protected;
Changes since v2:
- Matthew Wilcox suggestion is adopted, with that...
- copy_to_iter() is used instead of custom implementation;
Changes since v1:
- number of bytes left to copy on fail fixed;
Thanks!
Alexander Gordeev (4):
s390/zcore: fix race when reading from hardware system area
s390/crash: move copy_to_user_real() to crash_dump.c
s390/crash: use static swap buffer for copy_to_user_real()
s390/crash: support multi-segment iterators
arch/s390/include/asm/os_info.h | 17 ++++-
arch/s390/include/asm/sclp.h | 4 +-
arch/s390/include/asm/uaccess.h | 1 -
arch/s390/kernel/crash_dump.c | 114 ++++++++------------------------
arch/s390/mm/maccess.c | 26 --------
drivers/s390/char/zcore.c | 55 ++++++++-------
6 files changed, 71 insertions(+), 146 deletions(-)
--
2.34.1
Currently a temporary page-size buffer is allocated for copying
oldmem to user space. That limits copy_to_user_real() operation
only to stages when virtual memory is available and still makes
it possible to fail while the system is being dumped.
Instead of reallocating single page on each copy_oldmem_page()
iteration use a statically allocated buffer.
This also paves the way for a further memcpy_real() rework where
no swap buffer is needed altogether.
Signed-off-by: Alexander Gordeev <[email protected]>
---
arch/s390/kernel/crash_dump.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 8d7332d4444c..1662f1d81abe 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -53,6 +53,8 @@ struct save_area {
};
static LIST_HEAD(dump_save_areas);
+static DEFINE_MUTEX(memcpy_real_mutex);
+static char memcpy_real_buf[PAGE_SIZE];
/*
* Allocate a save area
@@ -178,25 +180,21 @@ int copy_oldmem_kernel(void *dst, unsigned long src, size_t count)
*/
static int copy_to_user_real(void __user *dest, unsigned long src, unsigned long count)
{
- int offs = 0, size, rc;
- char *buf;
+ unsigned long offs = 0, size;
- buf = (char *)__get_free_page(GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
- rc = -EFAULT;
+ mutex_lock(&memcpy_real_mutex);
while (offs < count) {
size = min(PAGE_SIZE, count - offs);
- if (memcpy_real(buf, src + offs, size))
- goto out;
- if (copy_to_user(dest + offs, buf, size))
- goto out;
+ if (memcpy_real(memcpy_real_buf, src + offs, size))
+ break;
+ if (copy_to_user(dest + offs, memcpy_real_buf, size))
+ break;
offs += size;
}
- rc = 0;
-out:
- free_page((unsigned long)buf);
- return rc;
+ mutex_unlock(&memcpy_real_mutex);
+ if (offs < count)
+ return -EFAULT;
+ return 0;
}
/*
--
2.34.1
Make it possible to handle not only single-, but also multi-
segment iterators in copy_oldmem_iter() callback. Change the
semantics of called functions to match the iterator model -
instead of an error code the exact number of bytes copied is
returned.
The swap page used to copy data to user space is adopted for
kernel space too. That does not bring any performance impact.
Suggested-by: Matthew Wilcox <[email protected]>
Fixes: 49b11524d648 ("s390/crash: add missing iterator advance in copy_oldmem_page()")
Signed-off-by: Alexander Gordeev <[email protected]>
---
arch/s390/include/asm/os_info.h | 17 ++++-
arch/s390/include/asm/sclp.h | 4 +-
arch/s390/kernel/crash_dump.c | 128 ++++++--------------------------
drivers/s390/char/zcore.c | 58 ++++++---------
4 files changed, 63 insertions(+), 144 deletions(-)
diff --git a/arch/s390/include/asm/os_info.h b/arch/s390/include/asm/os_info.h
index 147a8d547ef9..85248d8fee0c 100644
--- a/arch/s390/include/asm/os_info.h
+++ b/arch/s390/include/asm/os_info.h
@@ -8,6 +8,8 @@
#ifndef _ASM_S390_OS_INFO_H
#define _ASM_S390_OS_INFO_H
+#include <linux/uio.h>
+
#define OS_INFO_VERSION_MAJOR 1
#define OS_INFO_VERSION_MINOR 1
#define OS_INFO_MAGIC 0x4f53494e464f535aULL /* OSINFOSZ */
@@ -39,7 +41,20 @@ u32 os_info_csum(struct os_info *os_info);
#ifdef CONFIG_CRASH_DUMP
void *os_info_old_entry(int nr, unsigned long *size);
-int copy_oldmem_kernel(void *dst, unsigned long src, size_t count);
+size_t copy_oldmem_iter(struct iov_iter *iter, unsigned long src, size_t count);
+
+static inline int copy_oldmem_kernel(void *dst, unsigned long src, size_t count)
+{
+ struct iov_iter iter;
+ struct kvec kvec;
+
+ kvec.iov_base = dst;
+ kvec.iov_len = count;
+ iov_iter_kvec(&iter, WRITE, &kvec, 1, count);
+ if (copy_oldmem_iter(&iter, src, count) < count)
+ return -EFAULT;
+ return 0;
+}
#else
static inline void *os_info_old_entry(int nr, unsigned long *size)
{
diff --git a/arch/s390/include/asm/sclp.h b/arch/s390/include/asm/sclp.h
index addefe8ccdba..9d4c7f71e070 100644
--- a/arch/s390/include/asm/sclp.h
+++ b/arch/s390/include/asm/sclp.h
@@ -17,6 +17,7 @@
#define EXT_SCCB_READ_CPU (3 * PAGE_SIZE)
#ifndef __ASSEMBLY__
+#include <linux/uio.h>
#include <asm/chpid.h>
#include <asm/cpu.h>
@@ -146,8 +147,7 @@ int sclp_pci_deconfigure(u32 fid);
int sclp_ap_configure(u32 apid);
int sclp_ap_deconfigure(u32 apid);
int sclp_pci_report(struct zpci_report_error_header *report, u32 fh, u32 fid);
-int memcpy_hsa_kernel(void *dest, unsigned long src, size_t count);
-int memcpy_hsa_user(void __user *dest, unsigned long src, size_t count);
+size_t memcpy_hsa_iter(struct iov_iter *iter, unsigned long src, size_t count);
void sclp_ocf_cpc_name_copy(char *dst);
static inline int sclp_get_core_info(struct sclp_core_info *info, int early)
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 1662f1d81abe..bad8f47fc5d6 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -116,102 +116,35 @@ void __init save_area_add_vxrs(struct save_area *sa, __vector128 *vxrs)
memcpy(sa->vxrs_high, vxrs + 16, 16 * sizeof(__vector128));
}
-/*
- * Return physical address for virtual address
- */
-static inline void *load_real_addr(void *addr)
-{
- unsigned long real_addr;
-
- asm volatile(
- " lra %0,0(%1)\n"
- " jz 0f\n"
- " la %0,0\n"
- "0:"
- : "=a" (real_addr) : "a" (addr) : "cc");
- return (void *)real_addr;
-}
-
-/*
- * Copy memory of the old, dumped system to a kernel space virtual address
- */
-int copy_oldmem_kernel(void *dst, unsigned long src, size_t count)
-{
- unsigned long len;
- void *ra;
- int rc;
-
- while (count) {
- if (!oldmem_data.start && src < sclp.hsa_size) {
- /* Copy from zfcp/nvme dump HSA area */
- len = min(count, sclp.hsa_size - src);
- rc = memcpy_hsa_kernel(dst, src, len);
- if (rc)
- return rc;
- } else {
- /* Check for swapped kdump oldmem areas */
- if (oldmem_data.start && src - oldmem_data.start < oldmem_data.size) {
- src -= oldmem_data.start;
- len = min(count, oldmem_data.size - src);
- } else if (oldmem_data.start && src < oldmem_data.size) {
- len = min(count, oldmem_data.size - src);
- src += oldmem_data.start;
- } else {
- len = count;
- }
- if (is_vmalloc_or_module_addr(dst)) {
- ra = load_real_addr(dst);
- len = min(PAGE_SIZE - offset_in_page(ra), len);
- } else {
- ra = dst;
- }
- if (memcpy_real(ra, src, len))
- return -EFAULT;
- }
- dst += len;
- src += len;
- count -= len;
- }
- return 0;
-}
-
-/*
- * Copy memory from kernel (real) to user (virtual)
- */
-static int copy_to_user_real(void __user *dest, unsigned long src, unsigned long count)
+static size_t copy_to_iter_real(struct iov_iter *iter, unsigned long src, size_t count)
{
- unsigned long offs = 0, size;
+ size_t len, copied, res = 0;
mutex_lock(&memcpy_real_mutex);
- while (offs < count) {
- size = min(PAGE_SIZE, count - offs);
- if (memcpy_real(memcpy_real_buf, src + offs, size))
+ while (count) {
+ len = min(PAGE_SIZE, count);
+ if (memcpy_real(memcpy_real_buf, src, len))
break;
- if (copy_to_user(dest + offs, memcpy_real_buf, size))
+ copied = copy_to_iter(memcpy_real_buf, len, iter);
+ count -= copied;
+ src += copied;
+ res += copied;
+ if (copied < len)
break;
- offs += size;
}
mutex_unlock(&memcpy_real_mutex);
- if (offs < count)
- return -EFAULT;
- return 0;
+ return res;
}
-/*
- * Copy memory of the old, dumped system to a user space virtual address
- */
-static int copy_oldmem_user(void __user *dst, unsigned long src, size_t count)
+size_t copy_oldmem_iter(struct iov_iter *iter, unsigned long src, size_t count)
{
- unsigned long len;
- int rc;
+ size_t len, copied, res = 0;
while (count) {
if (!oldmem_data.start && src < sclp.hsa_size) {
/* Copy from zfcp/nvme dump HSA area */
len = min(count, sclp.hsa_size - src);
- rc = memcpy_hsa_user(dst, src, len);
- if (rc)
- return rc;
+ copied = memcpy_hsa_iter(iter, src, len);
} else {
/* Check for swapped kdump oldmem areas */
if (oldmem_data.start && src - oldmem_data.start < oldmem_data.size) {
@@ -223,15 +156,15 @@ static int copy_oldmem_user(void __user *dst, unsigned long src, size_t count)
} else {
len = count;
}
- rc = copy_to_user_real(dst, src, len);
- if (rc)
- return rc;
+ copied = copy_to_iter_real(iter, src, len);
}
- dst += len;
- src += len;
- count -= len;
+ count -= copied;
+ src += copied;
+ res += copied;
+ if (copied < len)
+ break;
}
- return 0;
+ return res;
}
/*
@@ -241,26 +174,9 @@ ssize_t copy_oldmem_page(struct iov_iter *iter, unsigned long pfn, size_t csize,
unsigned long offset)
{
unsigned long src;
- int rc;
- if (!(iter_is_iovec(iter) || iov_iter_is_kvec(iter)))
- return -EINVAL;
- /* Multi-segment iterators are not supported */
- if (iter->nr_segs > 1)
- return -EINVAL;
- if (!csize)
- return 0;
src = pfn_to_phys(pfn) + offset;
-
- /* XXX: pass the iov_iter down to a common function */
- if (iter_is_iovec(iter))
- rc = copy_oldmem_user(iter->iov->iov_base, src, csize);
- else
- rc = copy_oldmem_kernel(iter->kvec->iov_base, src, csize);
- if (rc < 0)
- return rc;
- iov_iter_advance(iter, csize);
- return csize;
+ return copy_oldmem_iter(iter, src, csize);
}
/*
diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
index 92b32ce645b9..f6da215ccf9f 100644
--- a/drivers/s390/char/zcore.c
+++ b/drivers/s390/char/zcore.c
@@ -17,6 +17,7 @@
#include <linux/debugfs.h>
#include <linux/panic_notifier.h>
#include <linux/reboot.h>
+#include <linux/uio.h>
#include <asm/asm-offsets.h>
#include <asm/ipl.h>
@@ -54,38 +55,37 @@ static DEFINE_MUTEX(hsa_buf_mutex);
static char hsa_buf[PAGE_SIZE] __aligned(PAGE_SIZE);
/*
- * Copy memory from HSA to user memory (not reentrant):
+ * Copy memory from HSA to iterator (not reentrant):
*
- * @dest: User buffer where memory should be copied to
+ * @iter: Iterator where memory should be copied to
* @src: Start address within HSA where data should be copied
* @count: Size of buffer, which should be copied
*/
-int memcpy_hsa_user(void __user *dest, unsigned long src, size_t count)
+size_t memcpy_hsa_iter(struct iov_iter *iter, unsigned long src, size_t count)
{
- unsigned long offset, bytes;
+ size_t bytes, copied, res = 0;
+ unsigned long offset;
if (!hsa_available)
- return -ENODATA;
+ return 0;
mutex_lock(&hsa_buf_mutex);
while (count) {
if (sclp_sdias_copy(hsa_buf, src / PAGE_SIZE + 2, 1)) {
TRACE("sclp_sdias_copy() failed\n");
- mutex_unlock(&hsa_buf_mutex);
- return -EIO;
+ break;
}
offset = src % PAGE_SIZE;
bytes = min(PAGE_SIZE - offset, count);
- if (copy_to_user(dest, hsa_buf + offset, bytes)) {
- mutex_unlock(&hsa_buf_mutex);
- return -EFAULT;
- }
- src += bytes;
- dest += bytes;
- count -= bytes;
+ copied = copy_to_iter(hsa_buf + offset, bytes, iter);
+ count -= copied;
+ src += copied;
+ res += copied;
+ if (copied < bytes)
+ break;
}
mutex_unlock(&hsa_buf_mutex);
- return 0;
+ return res;
}
/*
@@ -95,28 +95,16 @@ int memcpy_hsa_user(void __user *dest, unsigned long src, size_t count)
* @src: Start address within HSA where data should be copied
* @count: Size of buffer, which should be copied
*/
-int memcpy_hsa_kernel(void *dest, unsigned long src, size_t count)
+static inline int memcpy_hsa_kernel(void *dst, unsigned long src, size_t count)
{
- unsigned long offset, bytes;
+ struct iov_iter iter;
+ struct kvec kvec;
- if (!hsa_available)
- return -ENODATA;
-
- mutex_lock(&hsa_buf_mutex);
- while (count) {
- if (sclp_sdias_copy(hsa_buf, src / PAGE_SIZE + 2, 1)) {
- TRACE("sclp_sdias_copy() failed\n");
- mutex_unlock(&hsa_buf_mutex);
- return -EIO;
- }
- offset = src % PAGE_SIZE;
- bytes = min(PAGE_SIZE - offset, count);
- memcpy(dest, hsa_buf + offset, bytes);
- src += bytes;
- dest += bytes;
- count -= bytes;
- }
- mutex_unlock(&hsa_buf_mutex);
+ kvec.iov_base = dst;
+ kvec.iov_len = count;
+ iov_iter_kvec(&iter, WRITE, &kvec, 1, count);
+ if (memcpy_hsa_iter(&iter, src, count) < count)
+ return -EIO;
return 0;
}
--
2.34.1
On Tue, Jul 19, 2022 at 07:16:32AM +0200, Alexander Gordeev wrote:
> Hi Matthew et al,
>
> This series completes 5d8de293c224 ("vmcore: convert copy_oldmem_page()
> to take an iov_iter") for s390.
>
> Changes since v3:
> - concurrent access to HSA and oldmem swap buffers protected;
>
> Changes since v2:
> - Matthew Wilcox suggestion is adopted, with that...
> - copy_to_iter() is used instead of custom implementation;
>
> Changes since v1:
> - number of bytes left to copy on fail fixed;
OK... Do you prefer it to go through s390 tree? The thing is, I've
stuff in iov_iter tree that conflicts with it; I'll gladly drop that
bit (vfs.git #fixes-s390) in favour of your series (and drop s390
bits from "new iov_iter flavour - ITER_UBUF" in #work.iov_iter - they
are not needed anymore).
I can put your series into replacement of #fixes-s390, or pull it
from whatever static branch you put it into - up to you.
Preferences?
On Tue, Jul 19, 2022 at 07:16:32AM +0200, Alexander Gordeev wrote:
> Hi Matthew et al,
>
> This series completes 5d8de293c224 ("vmcore: convert copy_oldmem_page()
> to take an iov_iter") for s390.
>
> Changes since v3:
> - concurrent access to HSA and oldmem swap buffers protected;
>
> Changes since v2:
> - Matthew Wilcox suggestion is adopted, with that...
> - copy_to_iter() is used instead of custom implementation;
>
> Changes since v1:
> - number of bytes left to copy on fail fixed;
>
> Thanks!
>
>
> Alexander Gordeev (4):
> s390/zcore: fix race when reading from hardware system area
> s390/crash: move copy_to_user_real() to crash_dump.c
> s390/crash: use static swap buffer for copy_to_user_real()
> s390/crash: support multi-segment iterators
>
> arch/s390/include/asm/os_info.h | 17 ++++-
> arch/s390/include/asm/sclp.h | 4 +-
> arch/s390/include/asm/uaccess.h | 1 -
> arch/s390/kernel/crash_dump.c | 114 ++++++++------------------------
> arch/s390/mm/maccess.c | 26 --------
> drivers/s390/char/zcore.c | 55 ++++++++-------
> 6 files changed, 71 insertions(+), 146 deletions(-)
FWIW,
Acked-by: Heiko Carstens <[email protected]>
HI Alexander,
Alexander Gordeev <[email protected]> writes:
> Hi Matthew et al,
>
> This series completes 5d8de293c224 ("vmcore: convert copy_oldmem_page()
> to take an iov_iter") for s390.
>
> Changes since v3:
> - concurrent access to HSA and oldmem swap buffers protected;
>
> Changes since v2:
> - Matthew Wilcox suggestion is adopted, with that...
> - copy_to_iter() is used instead of custom implementation;
>
> Changes since v1:
> - number of bytes left to copy on fail fixed;
>
> Thanks!
>
>
> Alexander Gordeev (4):
> s390/zcore: fix race when reading from hardware system area
> s390/crash: move copy_to_user_real() to crash_dump.c
> s390/crash: use static swap buffer for copy_to_user_real()
> s390/crash: support multi-segment iterators
>
> arch/s390/include/asm/os_info.h | 17 ++++-
> arch/s390/include/asm/sclp.h | 4 +-
> arch/s390/include/asm/uaccess.h | 1 -
> arch/s390/kernel/crash_dump.c | 114 ++++++++------------------------
> arch/s390/mm/maccess.c | 26 --------
> drivers/s390/char/zcore.c | 55 ++++++++-------
> 6 files changed, 71 insertions(+), 146 deletions(-)
>
> --
> 2.34.1
all our kdump + makedumpfile (with CP as core collector as well) work
with these patches applied.
Tested on LPAR and zVM.
With and w/o KASAN.
Tested-by: Alexander Egorenkov <[email protected]>
Regards
Alex
On Wed, Jul 20, 2022 at 04:17:20AM +0100, Al Viro wrote:
> OK... Do you prefer it to go through s390 tree? The thing is, I've
> stuff in iov_iter tree that conflicts with it; I'll gladly drop that
> bit (vfs.git #fixes-s390) in favour of your series (and drop s390
> bits from "new iov_iter flavour - ITER_UBUF" in #work.iov_iter - they
> are not needed anymore).
>
> I can put your series into replacement of #fixes-s390, or pull it
> from whatever static branch you put it into - up to you.
> Preferences?
Hi Al,
I am preparing a branch for you.
Will retest it and let you know once it is done.
Thanks!
On Wed, Jul 20, 2022 at 04:17:20AM +0100, Al Viro wrote:
> I can put your series into replacement of #fixes-s390, or pull it
> from whatever static branch you put it into - up to you.
> Preferences?
Hi Al,
Please find the changes since commit af2debd58bd769e38f538143f0d332e15d753396:
s390/crash: make copy_oldmem_page() return number of bytes copied
up to ebbc9570169147740aa39aee1d61b4cc5a631644:
s390/crash: support multi-segment iterators
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git vmcore-iov_iter
Please, note three (rather trivial) prereq commits to pull along:
f6749da17a34 s390/crash: fix incorrect number of bytes to copy to user space
86caa4b67895 s390/crash: remove redundant panic() on save area allocation failure
7190d84966b3 s390/mm: remove unused tprot() function
Thanks!
On Thu, Jul 21, 2022 at 10:46:16AM +0200, Alexander Gordeev wrote:
> On Wed, Jul 20, 2022 at 04:17:20AM +0100, Al Viro wrote:
> > I can put your series into replacement of #fixes-s390, or pull it
> > from whatever static branch you put it into - up to you.
> > Preferences?
>
> Hi Al,
>
> Please find the changes since commit af2debd58bd769e38f538143f0d332e15d753396:
>
> s390/crash: make copy_oldmem_page() return number of bytes copied
>
> up to ebbc9570169147740aa39aee1d61b4cc5a631644:
>
> s390/crash: support multi-segment iterators
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git vmcore-iov_iter
>
> Please, note three (rather trivial) prereq commits to pull along:
>
> f6749da17a34 s390/crash: fix incorrect number of bytes to copy to user space
> 86caa4b67895 s390/crash: remove redundant panic() on save area allocation failure
> 7190d84966b3 s390/mm: remove unused tprot() function
Pulled.