| Message ID | 1639193588-7027-2-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kdump: simplify code | expand |
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. > > 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. > { > 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. > > /** > * copy_struct_from_user: copy a struct from userspace > diff --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
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) { 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); /** * copy_struct_from_user: copy a struct from userspace diff --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);
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. 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(-)