diff mbox series

[RFC,01/14] fs/proc/vmcore: Update read_from_oldmem() for user pointer

Message ID 20211203104231.17597-2-amit.kachhap@arm.com (mailing list archive)
State New, archived
Headers show
Series fs/proc/vmcore: Remove unnecessary user pointer conversions | expand

Commit Message

Amit Daniel Kachhap Dec. 3, 2021, 10:42 a.m. UTC
The exported interface read_from_oldmem() passes user pointer
without __user annotation and does unnecessary user/kernel pointer
conversions during the pointer propagation.

Hence it is modified to have a new parameter for user pointer.

Also a helper macro read_from_oldmem_to_kernel is added for clear
readability from callsite when calling read_from_oldmem() for copying
to kernel buffer.

There are several internal functions used here such as read_vmcore(),
__read_vmcore(), copy_to() and vmcoredd_copy_dumps() which are
re-structured to avoid the unnecessary user/kernel pointer conversions.

x86_64 crash dump is also modified to use this new interface.

No functionality change intended.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Young <dyoung@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: x86 <x86@kernel.org>
Cc: kexec <kexec@lists.infradead.org>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---
 arch/x86/kernel/crash_dump_64.c |  4 +-
 fs/proc/vmcore.c                | 88 +++++++++++++++++++--------------
 include/linux/crash_dump.h      | 12 +++--
 3 files changed, 60 insertions(+), 44 deletions(-)

Comments

Christoph Hellwig Dec. 6, 2021, 2:04 p.m. UTC | #1
On Fri, Dec 03, 2021 at 04:12:18PM +0530, Amit Daniel Kachhap wrote:
> +	return read_from_oldmem_to_kernel(buf, count, ppos,
> +					  cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));

Overly long line.

> +ssize_t read_from_oldmem(char __user *ubuf, char *kbuf, size_t count,
> +			 u64 *ppos, bool encrypted)
>  {
>  	unsigned long pfn, offset;
>  	size_t nr_bytes;
> @@ -156,19 +163,27 @@ ssize_t read_from_oldmem(char *buf, size_t count,
>  		/* If pfn is not ram, return zeros for sparse dump files */
>  		if (!pfn_is_ram(pfn)) {
>  			tmp = 0;
> -			if (!userbuf)
> -				memset(buf, 0, nr_bytes);
> -			else if (clear_user(buf, nr_bytes))
> +			if (kbuf)
> +				memset(kbuf, 0, nr_bytes);
> +			else if (clear_user(ubuf, nr_bytes))
>  				tmp = -EFAULT;

This looks like a huge mess.  What speak against using an iov_iter
here?
Matthew Wilcox (Oracle) Dec. 6, 2021, 2:17 p.m. UTC | #2
On Mon, Dec 06, 2021 at 03:04:51PM +0100, Christoph Hellwig wrote:
> This looks like a huge mess.  What speak against using an iov_iter
> here?

I coincidentally made a start on this last night.  Happy to stop.
What do you think to adding a generic copy_pfn_to_iter()?  Not sure
which APIs to use to implement it ... some architectures have weird
requirements about which APIs can be used for what kinds of PFNs.

diff --git a/arch/arm/kernel/crash_dump.c b/arch/arm/kernel/crash_dump.c
index 53cb92435392..ed0d806a4d14 100644
--- a/arch/arm/kernel/crash_dump.c
+++ b/arch/arm/kernel/crash_dump.c
@@ -16,39 +16,28 @@
 #include <linux/io.h>
 
 /**
- * copy_oldmem_page() - copy one page from old kernel memory
- * @pfn: page frame number to be copied
- * @buf: buffer where the copied page is placed
- * @csize: number of bytes to copy
- * @offset: offset in bytes into the page
- * @userbuf: if set, @buf is int he user address space
+ * copy_pfn_to_iter() - copy one page from old kernel memory.
+ * @iter: Where to copy the page to.
+ * @pfn: Page frame number to be copied.
+ * @offset: Offset in bytes into the page.
+ * @len: Number of bytes to copy.
  *
- * This function copies one page from old kernel memory into buffer pointed by
- * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
- * copied or negative error in case of failure.
+ * This function copies (part of) one page from old kernel memory into
+ * memory described by @iter.
+ *
+ * Return: Number of bytes copied or negative error in case of failure.
  */
-ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
-			 size_t csize, unsigned long offset,
-			 int userbuf)
+ssize_t copy_pfn_to_iter(struct iov_iter *iter, unsigned long pfn,
+			 size_t offset, size_t len)
 {
 	void *vaddr;
 
-	if (!csize)
-		return 0;
-
 	vaddr = ioremap(__pfn_to_phys(pfn), PAGE_SIZE);
 	if (!vaddr)
 		return -ENOMEM;
 
-	if (userbuf) {
-		if (copy_to_user(buf, vaddr + offset, csize)) {
-			iounmap(vaddr);
-			return -EFAULT;
-		}
-	} else {
-		memcpy(buf, vaddr + offset, csize);
-	}
+	len = copy_to_iter(vadd + offset, len, iter);
 
 	iounmap(vaddr);
-	return csize;
+	return len;
 }
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 620821549b23..2dd3a692bcb7 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -24,8 +24,8 @@ extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
 				  unsigned long from, unsigned long pfn,
 				  unsigned long size, pgprot_t prot);
 
-extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
-						unsigned long, int);
+ssize_t copy_pfn_to_iter(struct iov_iter *i, unsigned long pfn, size_t offset,
+		size_t len);
 extern ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
 					  size_t csize, unsigned long offset,
 					  int userbuf);
Christoph Hellwig Dec. 6, 2021, 2:54 p.m. UTC | #3
On Mon, Dec 06, 2021 at 02:17:24PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 06, 2021 at 03:04:51PM +0100, Christoph Hellwig wrote:
> > This looks like a huge mess.  What speak against using an iov_iter
> > here?
> 
> I coincidentally made a start on this last night.  Happy to stop.

Don't stop!

> What do you think to adding a generic copy_pfn_to_iter()?  Not sure
> which APIs to use to implement it ... some architectures have weird
> requirements about which APIs can be used for what kinds of PFNs.

Hmm.  I though kmap_local_pfn(_prot) is all we need?
Matthew Wilcox (Oracle) Dec. 6, 2021, 3:07 p.m. UTC | #4
On Mon, Dec 06, 2021 at 03:54:22PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 06, 2021 at 02:17:24PM +0000, Matthew Wilcox wrote:
> > On Mon, Dec 06, 2021 at 03:04:51PM +0100, Christoph Hellwig wrote:
> > > This looks like a huge mess.  What speak against using an iov_iter
> > > here?
> > 
> > I coincidentally made a start on this last night.  Happy to stop.
> 
> Don't stop!
> 
> > What do you think to adding a generic copy_pfn_to_iter()?  Not sure
> > which APIs to use to implement it ... some architectures have weird
> > requirements about which APIs can be used for what kinds of PFNs.
> 
> Hmm.  I though kmap_local_pfn(_prot) is all we need?

In the !HIGHMEM case, that calls pfn_to_page(), and I think the
point of this path is that we don't have a struct page for this pfn.
Amit Daniel Kachhap Dec. 7, 2021, 7:11 a.m. UTC | #5
On 12/6/21 7:34 PM, Christoph Hellwig wrote:
> On Fri, Dec 03, 2021 at 04:12:18PM +0530, Amit Daniel Kachhap wrote:
>> +	return read_from_oldmem_to_kernel(buf, count, ppos,
>> +					  cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
> 
> Overly long line.
> 
>> +ssize_t read_from_oldmem(char __user *ubuf, char *kbuf, size_t count,
>> +			 u64 *ppos, bool encrypted)
>>   {
>>   	unsigned long pfn, offset;
>>   	size_t nr_bytes;
>> @@ -156,19 +163,27 @@ ssize_t read_from_oldmem(char *buf, size_t count,
>>   		/* If pfn is not ram, return zeros for sparse dump files */
>>   		if (!pfn_is_ram(pfn)) {
>>   			tmp = 0;
>> -			if (!userbuf)
>> -				memset(buf, 0, nr_bytes);
>> -			else if (clear_user(buf, nr_bytes))
>> +			if (kbuf)
>> +				memset(kbuf, 0, nr_bytes);
>> +			else if (clear_user(ubuf, nr_bytes))
>>   				tmp = -EFAULT;
> 
> This looks like a huge mess.  What speak against using an iov_iter
> here?

iov_iter seems to be a reasonable way. As a start I thought of adding
minimal changes.

>
Christoph Hellwig Dec. 7, 2021, 11:15 a.m. UTC | #6
On Mon, Dec 06, 2021 at 03:07:15PM +0000, Matthew Wilcox wrote:
> > > What do you think to adding a generic copy_pfn_to_iter()?  Not sure
> > > which APIs to use to implement it ... some architectures have weird
> > > requirements about which APIs can be used for what kinds of PFNs.
> > 
> > Hmm.  I though kmap_local_pfn(_prot) is all we need?
> 
> In the !HIGHMEM case, that calls pfn_to_page(), and I think the
> point of this path is that we don't have a struct page for this pfn.

Indeed.  But to me this suggest that the !highmem stub is broken and
we should probably fix it rather than adding yet another interface.
diff mbox series

Patch

diff --git a/arch/x86/kernel/crash_dump_64.c b/arch/x86/kernel/crash_dump_64.c
index a7f617a3981d..6433513ef43a 100644
--- a/arch/x86/kernel/crash_dump_64.c
+++ b/arch/x86/kernel/crash_dump_64.c
@@ -74,6 +74,6 @@  ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
 
 ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0,
-				cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
+	return read_from_oldmem_to_kernel(buf, count, ppos,
+					  cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
 }
diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 509f85148fee..39b4353bd37c 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -70,6 +70,14 @@  static bool vmcore_cb_unstable;
 /* Whether the vmcore has been opened once. */
 static bool vmcore_opened;
 
+#define ptr_add(utarget, ktarget, size)		\
+({						\
+	if (utarget)				\
+		utarget += size;		\
+	else					\
+		ktarget += size;		\
+})
+
 void register_vmcore_cb(struct vmcore_cb *cb)
 {
 	down_write(&vmcore_cb_rwsem);
@@ -132,9 +140,8 @@  static int open_vmcore(struct inode *inode, struct file *file)
 }
 
 /* Reads a page from the oldmem device from given offset. */
-ssize_t read_from_oldmem(char *buf, size_t count,
-			 u64 *ppos, int userbuf,
-			 bool encrypted)
+ssize_t read_from_oldmem(char __user *ubuf, char *kbuf, size_t count,
+			 u64 *ppos, bool encrypted)
 {
 	unsigned long pfn, offset;
 	size_t nr_bytes;
@@ -156,19 +163,27 @@  ssize_t read_from_oldmem(char *buf, size_t count,
 		/* If pfn is not ram, return zeros for sparse dump files */
 		if (!pfn_is_ram(pfn)) {
 			tmp = 0;
-			if (!userbuf)
-				memset(buf, 0, nr_bytes);
-			else if (clear_user(buf, nr_bytes))
+			if (kbuf)
+				memset(kbuf, 0, nr_bytes);
+			else if (clear_user(ubuf, nr_bytes))
 				tmp = -EFAULT;
 		} else {
-			if (encrypted)
-				tmp = copy_oldmem_page_encrypted(pfn, buf,
+			if (encrypted && ubuf)
+				tmp = copy_oldmem_page_encrypted(pfn,
+								 (__force char *)ubuf,
+								 nr_bytes,
+								 offset, 1);
+			else if (encrypted && kbuf)
+				tmp = copy_oldmem_page_encrypted(pfn,
+								 kbuf,
 								 nr_bytes,
-								 offset,
-								 userbuf);
+								 offset, 0);
+			else if (ubuf)
+				tmp = copy_oldmem_page(pfn, (__force char *)ubuf,
+						       nr_bytes, offset, 1);
 			else
-				tmp = copy_oldmem_page(pfn, buf, nr_bytes,
-						       offset, userbuf);
+				tmp = copy_oldmem_page(pfn, kbuf, nr_bytes,
+						       offset, 0);
 		}
 		if (tmp < 0) {
 			up_read(&vmcore_cb_rwsem);
@@ -177,7 +192,7 @@  ssize_t read_from_oldmem(char *buf, size_t count,
 
 		*ppos += nr_bytes;
 		count -= nr_bytes;
-		buf += nr_bytes;
+		ptr_add(ubuf, kbuf, nr_bytes);
 		read += nr_bytes;
 		++pfn;
 		offset = 0;
@@ -206,7 +221,7 @@  void __weak elfcorehdr_free(unsigned long long addr)
  */
 ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0, false);
+	return read_from_oldmem_to_kernel(buf, count, ppos, false);
 }
 
 /*
@@ -214,7 +229,7 @@  ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
  */
 ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0, cc_platform_has(CC_ATTR_MEM_ENCRYPT));
+	return read_from_oldmem_to_kernel(buf, count, ppos, cc_platform_has(CC_ATTR_MEM_ENCRYPT));
 }
 
 /*
@@ -239,21 +254,21 @@  copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
 }
 
 /*
- * Copy to either kernel or user space
+ * Copy to either kernel or user space buffer
  */
-static int copy_to(void *target, void *src, size_t size, int userbuf)
+static int copy_to(void __user *utarget, void *ktarget, void *src, size_t size)
 {
-	if (userbuf) {
-		if (copy_to_user((char __user *) target, src, size))
+	if (utarget) {
+		if (copy_to_user(utarget, src, size))
 			return -EFAULT;
 	} else {
-		memcpy(target, src, size);
+		memcpy(ktarget, src, size);
 	}
 	return 0;
 }
 
 #ifdef CONFIG_PROC_VMCORE_DEVICE_DUMP
-static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, int userbuf)
+static int vmcoredd_copy_dumps(void __user *udst, void *kdst, u64 start, size_t size)
 {
 	struct vmcoredd_node *dump;
 	u64 offset = 0;
@@ -266,15 +281,14 @@  static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, int userbuf)
 		if (start < offset + dump->size) {
 			tsz = min(offset + (u64)dump->size - start, (u64)size);
 			buf = dump->buf + start - offset;
-			if (copy_to(dst, buf, tsz, userbuf)) {
+			if (copy_to(udst, kdst, buf, tsz)) {
 				ret = -EFAULT;
 				goto out_unlock;
 			}
 
 			size -= tsz;
 			start += tsz;
-			dst += tsz;
-
+			ptr_add(udst, kdst, tsz);
 			/* Leave now if buffer filled already */
 			if (!size)
 				goto out_unlock;
@@ -329,8 +343,8 @@  static int vmcoredd_mmap_dumps(struct vm_area_struct *vma, unsigned long dst,
 /* Read from the ELF header and then the crash dump. On error, negative value is
  * returned otherwise number of bytes read are returned.
  */
-static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
-			     int userbuf)
+static ssize_t __read_vmcore(char __user *ubuffer, char *kbuffer, size_t buflen,
+			     loff_t *fpos)
 {
 	ssize_t acc = 0, tmp;
 	size_t tsz;
@@ -347,11 +361,11 @@  static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 	/* Read ELF core header */
 	if (*fpos < elfcorebuf_sz) {
 		tsz = min(elfcorebuf_sz - (size_t)*fpos, buflen);
-		if (copy_to(buffer, elfcorebuf + *fpos, tsz, userbuf))
+		if (copy_to(ubuffer, kbuffer, elfcorebuf + *fpos, tsz))
 			return -EFAULT;
 		buflen -= tsz;
 		*fpos += tsz;
-		buffer += tsz;
+		ptr_add(ubuffer, kbuffer, tsz);
 		acc += tsz;
 
 		/* leave now if filled buffer already */
@@ -378,12 +392,12 @@  static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 			tsz = min(elfcorebuf_sz + vmcoredd_orig_sz -
 				  (size_t)*fpos, buflen);
 			start = *fpos - elfcorebuf_sz;
-			if (vmcoredd_copy_dumps(buffer, start, tsz, userbuf))
+			if (vmcoredd_copy_dumps(ubuffer, kbuffer, start, tsz))
 				return -EFAULT;
 
 			buflen -= tsz;
 			*fpos += tsz;
-			buffer += tsz;
+			ptr_add(ubuffer, kbuffer, tsz);
 			acc += tsz;
 
 			/* leave now if filled buffer already */
@@ -395,12 +409,12 @@  static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 		/* Read remaining elf notes */
 		tsz = min(elfcorebuf_sz + elfnotes_sz - (size_t)*fpos, buflen);
 		kaddr = elfnotes_buf + *fpos - elfcorebuf_sz - vmcoredd_orig_sz;
-		if (copy_to(buffer, kaddr, tsz, userbuf))
+		if (copy_to(ubuffer, kbuffer, kaddr, tsz))
 			return -EFAULT;
 
 		buflen -= tsz;
 		*fpos += tsz;
-		buffer += tsz;
+		ptr_add(ubuffer, kbuffer, tsz);
 		acc += tsz;
 
 		/* leave now if filled buffer already */
@@ -414,13 +428,13 @@  static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 					    m->offset + m->size - *fpos,
 					    buflen);
 			start = m->paddr + *fpos - m->offset;
-			tmp = read_from_oldmem(buffer, tsz, &start,
-					       userbuf, cc_platform_has(CC_ATTR_MEM_ENCRYPT));
+			tmp = read_from_oldmem(ubuffer, kbuffer, tsz, &start,
+					       cc_platform_has(CC_ATTR_MEM_ENCRYPT));
 			if (tmp < 0)
 				return tmp;
 			buflen -= tsz;
 			*fpos += tsz;
-			buffer += tsz;
+			ptr_add(ubuffer, kbuffer, tsz);
 			acc += tsz;
 
 			/* leave now if filled buffer already */
@@ -435,7 +449,7 @@  static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 static ssize_t read_vmcore(struct file *file, char __user *buffer,
 			   size_t buflen, loff_t *fpos)
 {
-	return __read_vmcore((__force char *) buffer, buflen, fpos, 1);
+	return __read_vmcore(buffer, NULL, buflen, fpos);
 }
 
 /*
@@ -461,7 +475,7 @@  static vm_fault_t mmap_vmcore_fault(struct vm_fault *vmf)
 	if (!PageUptodate(page)) {
 		offset = (loff_t) index << PAGE_SHIFT;
 		buf = __va((page_to_pfn(page) << PAGE_SHIFT));
-		rc = __read_vmcore(buf, PAGE_SIZE, &offset, 0);
+		rc = __read_vmcore(NULL, buf, PAGE_SIZE, &offset);
 		if (rc < 0) {
 			unlock_page(page);
 			put_page(page);
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 620821549b23..eb0ed423ccc8 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -135,16 +135,18 @@  static inline int vmcore_add_device_dump(struct vmcoredd_data *data)
 #endif /* CONFIG_PROC_VMCORE_DEVICE_DUMP */
 
 #ifdef CONFIG_PROC_VMCORE
-ssize_t read_from_oldmem(char *buf, size_t count,
-			 u64 *ppos, int userbuf,
-			 bool encrypted);
+ssize_t read_from_oldmem(char __user *ubuf, char *kbuf, size_t count,
+			 u64 *ppos, bool encrypted);
 #else
-static inline ssize_t read_from_oldmem(char *buf, size_t count,
-				       u64 *ppos, int userbuf,
+static inline ssize_t read_from_oldmem(char __user *ubuf, char *kbuf,
+				       size_t count, u64 *ppos,
 				       bool encrypted)
 {
 	return -EOPNOTSUPP;
 }
 #endif /* CONFIG_PROC_VMCORE */
 
+#define read_from_oldmem_to_kernel(buf, count, ppos, encrypted)	\
+	read_from_oldmem(NULL, buf, count, ppos, encrypted)
+
 #endif /* LINUX_CRASHDUMP_H */