Re: [PATCH v2 1/2] kdump: vmcore: remove copy_to() and add copy_to_user_or_kernel()
From: Christophe Leroy <hidden>
Date: 2021-12-11 10:32:39
Also in:
kexec, linux-arm-kernel, linux-fsdevel, linux-mips, linux-riscv, linux-sh, lkml
Le 11/12/2021 à 04:33, Tiezhu Yang a écrit :
In arch/*/kernel/crash_dump*.c, there exist many similar code about copy_oldmem_page(), remove copy_to() in fs/proc/vmcore.c and add copy_to_user_or_kernel() in lib/usercopy.c, then we can use copy_to_user_or_kernel() to simplify the related code.
It should be an inline function in uaccess.h, see below why.
quoted hunk ↗ jump to hunk
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- fs/proc/vmcore.c | 28 +++++++--------------------- include/linux/uaccess.h | 1 + lib/usercopy.c | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 21 deletions(-)diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c index 509f851..f67fd77 100644 --- a/fs/proc/vmcore.c +++ b/fs/proc/vmcore.c@@ -238,22 +238,8 @@ copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize, return copy_oldmem_page(pfn, buf, csize, offset, userbuf); } -/* - * Copy to either kernel or user space - */ -static int copy_to(void *target, void *src, size_t size, int userbuf) -{ - if (userbuf) { - if (copy_to_user((char __user *) target, src, size)) - return -EFAULT; - } else { - memcpy(target, 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 *dst, u64 start, size_t size, bool userbuf)
Changing int to bool in all the callers should be another patch. You can have copy_to_user_or_kernel() take a bool in the patch while still having all the callers using an int.
quoted hunk ↗ jump to hunk
{ struct vmcoredd_node *dump; u64 offset = 0;@@ -266,7 +252,7 @@ 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_user_or_kernel(dst, buf, tsz, userbuf)) { ret = -EFAULT; goto out_unlock; }@@ -330,7 +316,7 @@ static int vmcoredd_mmap_dumps(struct vm_area_struct *vma, unsigned long dst, * returned otherwise number of bytes read are returned. */ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos, - int userbuf) + bool userbuf) { ssize_t acc = 0, tmp; size_t tsz;@@ -347,7 +333,7 @@ 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_user_or_kernel(buffer, elfcorebuf + *fpos, tsz, userbuf)) return -EFAULT; buflen -= tsz; *fpos += tsz;@@ -395,7 +381,7 @@ 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_user_or_kernel(buffer, kaddr, tsz, userbuf)) return -EFAULT; buflen -= tsz;@@ -435,7 +421,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((__force char *) buffer, buflen, fpos, true); } /*@@ -461,7 +447,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(buf, PAGE_SIZE, &offset, false); if (rc < 0) { unlock_page(page); put_page(page);diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index ac03940..a25e682e 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h@@ -283,6 +283,7 @@ __copy_from_user_inatomic_nocache(void *to, const void __user *from, #endif /* ARCH_HAS_NOCACHE_UACCESS */ extern __must_check int check_zeroed_user(const void __user *from, size_t size); +extern __must_check int copy_to_user_or_kernel(void *target, void *src, size_t size, bool userbuf);
extern keyword is pointless for function prototypes, please don't add new ones.
quoted hunk ↗ jump to hunk
/** * copy_struct_from_user: copy a struct from userspacediff --git a/lib/usercopy.c b/lib/usercopy.c index 7413dd3..7431b1b 100644 --- a/lib/usercopy.c +++ b/lib/usercopy.c@@ -90,3 +90,18 @@ int check_zeroed_user(const void __user *from, size_t size) return -EFAULT; } EXPORT_SYMBOL(check_zeroed_user); + +/* + * Copy to either user or kernel space + */ +int copy_to_user_or_kernel(void *target, void *src, size_t size, bool userbuf) +{ + if (userbuf) { + if (copy_to_user((char __user *) target, src, size)) + return -EFAULT; + } else { + memcpy(target, src, size); + } + return 0; +} +EXPORT_SYMBOL(copy_to_user_or_kernel);
Ref my answer to Andrew, I don't think outlining this fonction is a
worth it. As shown in that mail, the size of the caller is increased by
4 instructions (which is in the noise) but also this new function is not
small. So I see no benefit in term of size, and I don't think there is
any benefit in terms of performance either.
In this patch that's the same. Before the patch, read_vmcore() has a
size of 0x338.
With this patch, read_vmcore() has a size of 0x340. So that's 2
instructions more, so no benefit either.
So I think this should remain an inline function like in your first
patch (but with the new name).
000001a4 <copy_to_user_or_kernel>:
1a4: 2c 06 00 00 cmpwi r6,0
1a8: 94 21 ff f0 stwu r1,-16(r1)
1ac: 41 82 00 50 beq 1fc <copy_to_user_or_kernel+0x58>
1b0: 2c 05 00 00 cmpwi r5,0
1b4: 41 80 00 7c blt 230 <copy_to_user_or_kernel+0x8c>
1b8: 3d 00 b0 00 lis r8,-20480
1bc: 7f 83 40 40 cmplw cr7,r3,r8
1c0: 41 9c 00 14 blt cr7,1d4 <copy_to_user_or_kernel+0x30>
1c4: 40 82 00 64 bne 228 <copy_to_user_or_kernel+0x84>
1c8: 38 60 00 00 li r3,0
1cc: 38 21 00 10 addi r1,r1,16
1d0: 4e 80 00 20 blr
1d4: 7d 23 40 50 subf r9,r3,r8
1d8: 7f 85 48 40 cmplw cr7,r5,r9
1dc: 7c 08 02 a6 mflr r0
1e0: 90 01 00 14 stw r0,20(r1)
1e4: 41 9d 00 38 bgt cr7,21c <copy_to_user_or_kernel+0x78>
1e8: 48 00 00 01 bl 1e8 <copy_to_user_or_kernel+0x44>
1e8: R_PPC_REL24 __copy_tofrom_user
1ec: 80 01 00 14 lwz r0,20(r1)
1f0: 2c 03 00 00 cmpwi r3,0
1f4: 7c 08 03 a6 mtlr r0
1f8: 4b ff ff cc b 1c4 <copy_to_user_or_kernel+0x20>
1fc: 7c 08 02 a6 mflr r0
200: 90 01 00 14 stw r0,20(r1)
204: 48 00 00 01 bl 204 <copy_to_user_or_kernel+0x60>
204: R_PPC_REL24 memcpy
208: 38 60 00 00 li r3,0
20c: 80 01 00 14 lwz r0,20(r1)
210: 38 21 00 10 addi r1,r1,16
214: 7c 08 03 a6 mtlr r0
218: 4e 80 00 20 blr
21c: 80 01 00 14 lwz r0,20(r1)
220: 7c 08 03 a6 mtlr r0
224: 4b ff ff a0 b 1c4 <copy_to_user_or_kernel+0x20>
228: 38 60 ff f2 li r3,-14
22c: 4b ff ff a0 b 1cc <copy_to_user_or_kernel+0x28>
230: 0f e0 00 00 twui r0,0
234: 7c 08 02 a6 mflr r0
238: 90 01 00 14 stw r0,20(r1)
Also note that checkpatch.pl provides the following on your patch:
CHECK: No space is necessary after a cast
#88: FILE: fs/proc/vmcore.c:424:
+ return __read_vmcore((__force char *) buffer, buflen, fpos, true);
CHECK: extern prototypes should be avoided in .h files
#109: FILE: include/linux/uaccess.h:286:
+extern __must_check int copy_to_user_or_kernel(void *target, void *src,
size_t size, bool userbuf);
CHECK: No space is necessary after a cast
#128: FILE: lib/usercopy.c:100:
+ if (copy_to_user((char __user *) target, src, size))
total: 0 errors, 0 warnings, 3 checks, 96 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
Commit 2c94767fa768 ("kdump: vmcore: remove copy_to() and add
copy_to_user_or_kernel()") has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Christophe