Re: [PATCH v2 1/2] kdump: vmcore: remove copy_to() and add copy_to_user_or_kernel()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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