linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] s390/crash: support multi-segment iterators
@ 2022-07-18 13:32 Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 1/3] s390/crash: move copy_to_user_real() to crash_dump.c Alexander Gordeev
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alexander Gordeev @ 2022-07-18 13:32 UTC (permalink / raw)
  To: Matthew Wilcox, Alexander Egorenkov, Heiko Carstens
  Cc: Baoquan He, Christoph Hellwig, Vasily Gorbik, Al Viro,
	linux-kernel, linux-s390

Hi Matthew et al,

This series completes 5d8de293c224 ("vmcore: convert copy_oldmem_page() to
take an iov_iter") for s390.

@Matthew,
  Please, let me know if you are not okay with Suggested-by tag in patch 3.

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 (3):
  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   | 111 +++++++-------------------------
 arch/s390/mm/maccess.c          |  26 --------
 drivers/s390/char/zcore.c       |  59 +++++++----------
 6 files changed, 65 insertions(+), 153 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/3] s390/crash: move copy_to_user_real() to crash_dump.c
  2022-07-18 13:32 [PATCH v3 0/3] s390/crash: support multi-segment iterators Alexander Gordeev
@ 2022-07-18 13:32 ` Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real() Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 3/3] s390/crash: support multi-segment iterators Alexander Gordeev
  2 siblings, 0 replies; 7+ messages in thread
From: Alexander Gordeev @ 2022-07-18 13:32 UTC (permalink / raw)
  To: Matthew Wilcox, Alexander Egorenkov, Heiko Carstens
  Cc: Baoquan He, Christoph Hellwig, Vasily Gorbik, Al Viro,
	linux-kernel, linux-s390

Function copy_to_user_real() does not really belong to maccess.c.
It is only used for copying oldmem to user space, so let's move
it to the friends.

Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/include/asm/uaccess.h |  1 -
 arch/s390/kernel/crash_dump.c   | 26 ++++++++++++++++++++++++++
 arch/s390/mm/maccess.c          | 26 --------------------------
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index f4511e21d646..2a067315fe59 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -285,7 +285,6 @@ static inline unsigned long __must_check clear_user(void __user *to, unsigned lo
 	return __clear_user(to, n);
 }
 
-int copy_to_user_real(void __user *dest, unsigned long src, unsigned long count);
 void *s390_kernel_write(void *dst, const void *src, size_t size);
 
 int __noreturn __put_kernel_bad(void);
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 0efee5c49b1e..8d7332d4444c 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -173,6 +173,32 @@ int copy_oldmem_kernel(void *dst, unsigned long src, size_t count)
 	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)
+{
+	int offs = 0, size, rc;
+	char *buf;
+
+	buf = (char *)__get_free_page(GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+	rc = -EFAULT;
+	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;
+		offs += size;
+	}
+	rc = 0;
+out:
+	free_page((unsigned long)buf);
+	return rc;
+}
+
 /*
  * Copy memory of the old, dumped system to a user space virtual address
  */
diff --git a/arch/s390/mm/maccess.c b/arch/s390/mm/maccess.c
index 421efa46946b..d6d84e02f35a 100644
--- a/arch/s390/mm/maccess.c
+++ b/arch/s390/mm/maccess.c
@@ -171,32 +171,6 @@ void memcpy_absolute(void *dest, void *src, size_t count)
 	arch_local_irq_restore(flags);
 }
 
-/*
- * Copy memory from kernel (real) to user (virtual)
- */
-int copy_to_user_real(void __user *dest, unsigned long src, unsigned long count)
-{
-	int offs = 0, size, rc;
-	char *buf;
-
-	buf = (char *) __get_free_page(GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-	rc = -EFAULT;
-	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;
-		offs += size;
-	}
-	rc = 0;
-out:
-	free_page((unsigned long) buf);
-	return rc;
-}
-
 /*
  * Check if physical address is within prefix or zero page
  */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real()
  2022-07-18 13:32 [PATCH v3 0/3] s390/crash: support multi-segment iterators Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 1/3] s390/crash: move copy_to_user_real() to crash_dump.c Alexander Gordeev
@ 2022-07-18 13:32 ` Alexander Gordeev
  2022-07-18 13:48   ` Matthew Wilcox
  2022-07-18 13:32 ` [PATCH v3 3/3] s390/crash: support multi-segment iterators Alexander Gordeev
  2 siblings, 1 reply; 7+ messages in thread
From: Alexander Gordeev @ 2022-07-18 13:32 UTC (permalink / raw)
  To: Matthew Wilcox, Alexander Egorenkov, Heiko Carstens
  Cc: Baoquan He, Christoph Hellwig, Vasily Gorbik, Al Viro,
	linux-kernel, linux-s390

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 <agordeev@linux.ibm.com>
---
 arch/s390/kernel/crash_dump.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 8d7332d4444c..fd9fe93e48b6 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -53,6 +53,7 @@ struct save_area {
 };
 
 static LIST_HEAD(dump_save_areas);
+static char memcpy_real_buf[PAGE_SIZE];
 
 /*
  * Allocate a save area
@@ -179,23 +180,18 @@ 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;
 
-	buf = (char *)__get_free_page(GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
 	rc = -EFAULT;
 	while (offs < count) {
 		size = min(PAGE_SIZE, count - offs);
-		if (memcpy_real(buf, src + offs, size))
+		if (memcpy_real(memcpy_real_buf, src + offs, size))
 			goto out;
-		if (copy_to_user(dest + offs, buf, size))
+		if (copy_to_user(dest + offs, memcpy_real_buf, size))
 			goto out;
 		offs += size;
 	}
 	rc = 0;
 out:
-	free_page((unsigned long)buf);
 	return rc;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 3/3] s390/crash: support multi-segment iterators
  2022-07-18 13:32 [PATCH v3 0/3] s390/crash: support multi-segment iterators Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 1/3] s390/crash: move copy_to_user_real() to crash_dump.c Alexander Gordeev
  2022-07-18 13:32 ` [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real() Alexander Gordeev
@ 2022-07-18 13:32 ` Alexander Gordeev
  2 siblings, 0 replies; 7+ messages in thread
From: Alexander Gordeev @ 2022-07-18 13:32 UTC (permalink / raw)
  To: Matthew Wilcox, Alexander Egorenkov, Heiko Carstens
  Cc: Baoquan He, Christoph Hellwig, Vasily Gorbik, Al Viro,
	linux-kernel, linux-s390

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 <willy@infradead.org>
Fixes: 49b11524d648 ("s390/crash: add missing iterator advance in copy_oldmem_page()")
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/include/asm/os_info.h |  17 ++++-
 arch/s390/include/asm/sclp.h    |   4 +-
 arch/s390/kernel/crash_dump.c   | 131 ++++++--------------------------
 drivers/s390/char/zcore.c       |  59 ++++++--------
 4 files changed, 64 insertions(+), 147 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 fd9fe93e48b6..8f918441a9de 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -115,101 +115,33 @@ 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)
+static size_t copy_to_iter_real(struct iov_iter *iter, unsigned long src, size_t count)
 {
-	unsigned long len;
-	void *ra;
-	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_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)
-{
-	int offs = 0, size, rc;
-
-	rc = -EFAULT;
-	while (offs < count) {
-		size = min(PAGE_SIZE, count - offs);
-		if (memcpy_real(memcpy_real_buf, src + offs, size))
-			goto out;
-		if (copy_to_user(dest + offs, memcpy_real_buf, size))
-			goto out;
-		offs += size;
+		len = min(PAGE_SIZE, count);
+		if (memcpy_real(memcpy_real_buf, src, len))
+			break;
+		copied = copy_to_iter(memcpy_real_buf, len, iter);
+		count -= copied;
+		src += copied;
+		res += copied;
+		if (copied < len)
+			break;
 	}
-	rc = 0;
-out:
-	return rc;
+	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) {
@@ -221,15 +153,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;
 }
 
 /*
@@ -239,26 +171,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 516783ba950f..3846bf892b2f 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>
@@ -53,61 +54,47 @@ static struct ipl_parameter_block *zcore_ipl_block;
 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;
 
 	while (count) {
 		if (sclp_sdias_copy(hsa_buf, src / PAGE_SIZE + 2, 1)) {
 			TRACE("sclp_sdias_copy() failed\n");
-			return -EIO;
+			break;
 		}
 		offset = src % PAGE_SIZE;
 		bytes = min(PAGE_SIZE - offset, count);
-		if (copy_to_user(dest, hsa_buf + offset, bytes))
-			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;
 	}
-	return 0;
+	return res;
 }
 
-/*
- * Copy memory from HSA to kernel memory (not reentrant):
- *
- * @dest:  Kernel or user buffer 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_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;
-
-	while (count) {
-		if (sclp_sdias_copy(hsa_buf, src / PAGE_SIZE + 2, 1)) {
-			TRACE("sclp_sdias_copy() failed\n");
-			return -EIO;
-		}
-		offset = src % PAGE_SIZE;
-		bytes = min(PAGE_SIZE - offset, count);
-		memcpy(dest, hsa_buf + offset, bytes);
-		src += bytes;
-		dest += bytes;
-		count -= bytes;
-	}
+	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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real()
  2022-07-18 13:32 ` [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real() Alexander Gordeev
@ 2022-07-18 13:48   ` Matthew Wilcox
  2022-07-18 14:51     ` Alexander Gordeev
  0 siblings, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2022-07-18 13:48 UTC (permalink / raw)
  To: Alexander Gordeev
  Cc: Alexander Egorenkov, Heiko Carstens, Baoquan He,
	Christoph Hellwig, Vasily Gorbik, Al Viro, linux-kernel,
	linux-s390

On Mon, Jul 18, 2022 at 03:32:40PM +0200, Alexander Gordeev wrote:
> +++ b/arch/s390/kernel/crash_dump.c
> @@ -53,6 +53,7 @@ struct save_area {
>  };
>  
>  static LIST_HEAD(dump_save_areas);

I'd suggest you need a mutex here so that simultaneous calls to
copy_to_user_real() don't corrupt each others data.

> +static char memcpy_real_buf[PAGE_SIZE];
>  
>  /*
>   * Allocate a save area
> @@ -179,23 +180,18 @@ 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;
>  
> -	buf = (char *)__get_free_page(GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
>  	rc = -EFAULT;
>  	while (offs < count) {
>  		size = min(PAGE_SIZE, count - offs);
> -		if (memcpy_real(buf, src + offs, size))
> +		if (memcpy_real(memcpy_real_buf, src + offs, size))
>  			goto out;
> -		if (copy_to_user(dest + offs, buf, size))
> +		if (copy_to_user(dest + offs, memcpy_real_buf, size))
>  			goto out;
>  		offs += size;
>  	}
>  	rc = 0;
>  out:
> -	free_page((unsigned long)buf);
>  	return rc;
>  }
>  
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real()
  2022-07-18 13:48   ` Matthew Wilcox
@ 2022-07-18 14:51     ` Alexander Gordeev
  2022-07-18 16:02       ` Matthew Wilcox
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Gordeev @ 2022-07-18 14:51 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Alexander Egorenkov, Heiko Carstens, Baoquan He,
	Christoph Hellwig, Vasily Gorbik, Al Viro, linux-kernel,
	linux-s390

On Mon, Jul 18, 2022 at 02:48:21PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 18, 2022 at 03:32:40PM +0200, Alexander Gordeev wrote:
> > +++ b/arch/s390/kernel/crash_dump.c
> > @@ -53,6 +53,7 @@ struct save_area {
> >  };
> >  
> >  static LIST_HEAD(dump_save_areas);
> 
> I'd suggest you need a mutex here so that simultaneous calls to
> copy_to_user_real() don't corrupt each others data.

We stop all (but one) CPUs before calling into the capture kernel -
one that calls these functions. Similarily to racy hsa_buf[] access
from memcpy_hsa_iter() this should not hit.

As you noticed last time, it is a pre-existing race and I was
actually going to address it in a separate fix - if the problem
really exists.

> > +static char memcpy_real_buf[PAGE_SIZE];
> >  
> >  /*
> >   * Allocate a save area
> > @@ -179,23 +180,18 @@ 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;
> >  
> > -	buf = (char *)__get_free_page(GFP_KERNEL);
> > -	if (!buf)
> > -		return -ENOMEM;
> >  	rc = -EFAULT;
> >  	while (offs < count) {
> >  		size = min(PAGE_SIZE, count - offs);
> > -		if (memcpy_real(buf, src + offs, size))
> > +		if (memcpy_real(memcpy_real_buf, src + offs, size))
> >  			goto out;
> > -		if (copy_to_user(dest + offs, buf, size))
> > +		if (copy_to_user(dest + offs, memcpy_real_buf, size))
> >  			goto out;
> >  		offs += size;
> >  	}
> >  	rc = 0;
> >  out:
> > -	free_page((unsigned long)buf);
> >  	return rc;
> >  }

Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real()
  2022-07-18 14:51     ` Alexander Gordeev
@ 2022-07-18 16:02       ` Matthew Wilcox
  0 siblings, 0 replies; 7+ messages in thread
From: Matthew Wilcox @ 2022-07-18 16:02 UTC (permalink / raw)
  To: Alexander Gordeev
  Cc: Alexander Egorenkov, Heiko Carstens, Baoquan He,
	Christoph Hellwig, Vasily Gorbik, Al Viro, linux-kernel,
	linux-s390

On Mon, Jul 18, 2022 at 04:51:59PM +0200, Alexander Gordeev wrote:
> On Mon, Jul 18, 2022 at 02:48:21PM +0100, Matthew Wilcox wrote:
> > On Mon, Jul 18, 2022 at 03:32:40PM +0200, Alexander Gordeev wrote:
> > > +++ b/arch/s390/kernel/crash_dump.c
> > > @@ -53,6 +53,7 @@ struct save_area {
> > >  };
> > >  
> > >  static LIST_HEAD(dump_save_areas);
> > 
> > I'd suggest you need a mutex here so that simultaneous calls to
> > copy_to_user_real() don't corrupt each others data.
> 
> We stop all (but one) CPUs before calling into the capture kernel -
> one that calls these functions. Similarily to racy hsa_buf[] access
> from memcpy_hsa_iter() this should not hit.

Could you show me how that works when two processes read from
/proc/vmcore at the same time?

> As you noticed last time, it is a pre-existing race and I was
> actually going to address it in a separate fix - if the problem
> really exists.
> 
> > > +static char memcpy_real_buf[PAGE_SIZE];
> > >  
> > >  /*
> > >   * Allocate a save area
> > > @@ -179,23 +180,18 @@ 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;
> > >  
> > > -	buf = (char *)__get_free_page(GFP_KERNEL);
> > > -	if (!buf)
> > > -		return -ENOMEM;
> > >  	rc = -EFAULT;
> > >  	while (offs < count) {
> > >  		size = min(PAGE_SIZE, count - offs);
> > > -		if (memcpy_real(buf, src + offs, size))
> > > +		if (memcpy_real(memcpy_real_buf, src + offs, size))
> > >  			goto out;
> > > -		if (copy_to_user(dest + offs, buf, size))
> > > +		if (copy_to_user(dest + offs, memcpy_real_buf, size))
> > >  			goto out;
> > >  		offs += size;
> > >  	}
> > >  	rc = 0;
> > >  out:
> > > -	free_page((unsigned long)buf);
> > >  	return rc;
> > >  }
> 
> Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-07-18 16:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-18 13:32 [PATCH v3 0/3] s390/crash: support multi-segment iterators Alexander Gordeev
2022-07-18 13:32 ` [PATCH v3 1/3] s390/crash: move copy_to_user_real() to crash_dump.c Alexander Gordeev
2022-07-18 13:32 ` [PATCH v3 2/3] s390/crash: use static swap buffer for copy_to_user_real() Alexander Gordeev
2022-07-18 13:48   ` Matthew Wilcox
2022-07-18 14:51     ` Alexander Gordeev
2022-07-18 16:02       ` Matthew Wilcox
2022-07-18 13:32 ` [PATCH v3 3/3] s390/crash: support multi-segment iterators Alexander Gordeev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).