* [PATCH v2 2/2] kdump: crashdump: use copy_to_user_or_kernel() to simplify code
From: Tiezhu Yang @ 2021-12-11 3:33 UTC (permalink / raw)
To: Dave Young, Baoquan He, Vivek Goyal, Andrew Morton
Cc: linux-ia64, linux-sh, Xuefeng Li, x86, kexec, linux-mips,
linux-kernel, linux-fsdevel, linux-riscv, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1639193588-7027-1-git-send-email-yangtiezhu@loongson.cn>
Use copy_to_user_or_kernel() to simplify the related code about
copy_oldmem_page() in arch/*/kernel/crash_dump*.c files.
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/arm/kernel/crash_dump.c | 12 +++---------
arch/arm64/kernel/crash_dump.c | 12 +++---------
arch/ia64/kernel/crash_dump.c | 12 +++++-------
arch/mips/kernel/crash_dump.c | 11 +++--------
arch/powerpc/kernel/crash_dump.c | 11 ++++-------
arch/riscv/kernel/crash_dump.c | 11 +++--------
arch/sh/kernel/crash_dump.c | 11 +++--------
arch/x86/kernel/crash_dump_32.c | 11 +++--------
arch/x86/kernel/crash_dump_64.c | 15 +++++----------
fs/proc/vmcore.c | 4 ++--
include/linux/crash_dump.h | 8 ++++----
11 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/arch/arm/kernel/crash_dump.c b/arch/arm/kernel/crash_dump.c
index 53cb924..a27c5df 100644
--- a/arch/arm/kernel/crash_dump.c
+++ b/arch/arm/kernel/crash_dump.c
@@ -29,7 +29,7 @@
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
size_t csize, unsigned long offset,
- int userbuf)
+ bool userbuf)
{
void *vaddr;
@@ -40,14 +40,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
if (!vaddr)
return -ENOMEM;
- if (userbuf) {
- if (copy_to_user(buf, vaddr + offset, csize)) {
- iounmap(vaddr);
- return -EFAULT;
- }
- } else {
- memcpy(buf, vaddr + offset, csize);
- }
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
iounmap(vaddr);
return csize;
diff --git a/arch/arm64/kernel/crash_dump.c b/arch/arm64/kernel/crash_dump.c
index 58303a9..d22988f 100644
--- a/arch/arm64/kernel/crash_dump.c
+++ b/arch/arm64/kernel/crash_dump.c
@@ -27,7 +27,7 @@
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
size_t csize, unsigned long offset,
- int userbuf)
+ bool userbuf)
{
void *vaddr;
@@ -38,14 +38,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
if (!vaddr)
return -ENOMEM;
- if (userbuf) {
- if (copy_to_user((char __user *)buf, vaddr + offset, csize)) {
- memunmap(vaddr);
- return -EFAULT;
- }
- } else {
- memcpy(buf, vaddr + offset, csize);
- }
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
memunmap(vaddr);
diff --git a/arch/ia64/kernel/crash_dump.c b/arch/ia64/kernel/crash_dump.c
index 0ed3c3d..12128f8 100644
--- a/arch/ia64/kernel/crash_dump.c
+++ b/arch/ia64/kernel/crash_dump.c
@@ -33,19 +33,17 @@
*/
ssize_t
copy_oldmem_page(unsigned long pfn, char *buf,
- size_t csize, unsigned long offset, int userbuf)
+ size_t csize, unsigned long offset, bool userbuf)
{
void *vaddr;
if (!csize)
return 0;
+
vaddr = __va(pfn<<PAGE_SHIFT);
- if (userbuf) {
- if (copy_to_user(buf, (vaddr + offset), csize)) {
- return -EFAULT;
- }
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ return -EFAULT;
+
return csize;
}
diff --git a/arch/mips/kernel/crash_dump.c b/arch/mips/kernel/crash_dump.c
index 2e50f551..7670915 100644
--- a/arch/mips/kernel/crash_dump.c
+++ b/arch/mips/kernel/crash_dump.c
@@ -16,7 +16,7 @@
* in the current kernel.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
- size_t csize, unsigned long offset, int userbuf)
+ size_t csize, unsigned long offset, bool userbuf)
{
void *vaddr;
@@ -24,13 +24,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
return 0;
vaddr = kmap_local_pfn(pfn);
-
- if (!userbuf) {
- memcpy(buf, vaddr + offset, csize);
- } else {
- if (copy_to_user(buf, vaddr + offset, csize))
- csize = -EFAULT;
- }
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
kunmap_local(vaddr);
diff --git a/arch/powerpc/kernel/crash_dump.c b/arch/powerpc/kernel/crash_dump.c
index 5693e1c67..e2e9612 100644
--- a/arch/powerpc/kernel/crash_dump.c
+++ b/arch/powerpc/kernel/crash_dump.c
@@ -69,13 +69,10 @@ void __init setup_kdump_trampoline(void)
#endif /* CONFIG_NONSTATIC_KERNEL */
static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
- unsigned long offset, int userbuf)
+ unsigned long offset, bool userbuf)
{
- if (userbuf) {
- if (copy_to_user((char __user *)buf, (vaddr + offset), csize))
- return -EFAULT;
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ return -EFAULT;
return csize;
}
@@ -94,7 +91,7 @@ static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
* in the current kernel. We stitch up a pte, similar to kmap_atomic.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
- size_t csize, unsigned long offset, int userbuf)
+ size_t csize, unsigned long offset, bool userbuf)
{
void *vaddr;
phys_addr_t paddr;
diff --git a/arch/riscv/kernel/crash_dump.c b/arch/riscv/kernel/crash_dump.c
index 86cc0ad..4167437 100644
--- a/arch/riscv/kernel/crash_dump.c
+++ b/arch/riscv/kernel/crash_dump.c
@@ -22,7 +22,7 @@
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
size_t csize, unsigned long offset,
- int userbuf)
+ bool userbuf)
{
void *vaddr;
@@ -33,13 +33,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
if (!vaddr)
return -ENOMEM;
- if (userbuf) {
- if (copy_to_user((char __user *)buf, vaddr + offset, csize)) {
- memunmap(vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, vaddr + offset, csize);
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
memunmap(vaddr);
return csize;
diff --git a/arch/sh/kernel/crash_dump.c b/arch/sh/kernel/crash_dump.c
index 5b41b59..4bc071a 100644
--- a/arch/sh/kernel/crash_dump.c
+++ b/arch/sh/kernel/crash_dump.c
@@ -24,7 +24,7 @@
* in the current kernel. We stitch up a pte, similar to kmap_atomic.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
- size_t csize, unsigned long offset, int userbuf)
+ size_t csize, unsigned long offset, bool userbuf)
{
void __iomem *vaddr;
@@ -33,13 +33,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
vaddr = ioremap(pfn << PAGE_SHIFT, PAGE_SIZE);
- if (userbuf) {
- if (copy_to_user((void __user *)buf, (vaddr + offset), csize)) {
- iounmap(vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
iounmap(vaddr);
return csize;
diff --git a/arch/x86/kernel/crash_dump_32.c b/arch/x86/kernel/crash_dump_32.c
index 5fcac46..3eff124 100644
--- a/arch/x86/kernel/crash_dump_32.c
+++ b/arch/x86/kernel/crash_dump_32.c
@@ -43,7 +43,7 @@ static inline bool is_crashed_pfn_valid(unsigned long pfn)
* in the current kernel.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
- unsigned long offset, int userbuf)
+ unsigned long offset, bool userbuf)
{
void *vaddr;
@@ -54,13 +54,8 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
return -EFAULT;
vaddr = kmap_local_pfn(pfn);
-
- if (!userbuf) {
- memcpy(buf, vaddr + offset, csize);
- } else {
- if (copy_to_user(buf, vaddr + offset, csize))
- csize = -EFAULT;
- }
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
kunmap_local(vaddr);
diff --git a/arch/x86/kernel/crash_dump_64.c b/arch/x86/kernel/crash_dump_64.c
index a7f617a..e8fffdf 100644
--- a/arch/x86/kernel/crash_dump_64.c
+++ b/arch/x86/kernel/crash_dump_64.c
@@ -13,7 +13,7 @@
#include <linux/cc_platform.h>
static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
- unsigned long offset, int userbuf,
+ unsigned long offset, bool userbuf,
bool encrypted)
{
void *vaddr;
@@ -29,13 +29,8 @@ static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
if (!vaddr)
return -ENOMEM;
- if (userbuf) {
- if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
- iounmap((void __iomem *)vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, vaddr + offset, csize);
+ if (copy_to_user_or_kernel(buf, vaddr + offset, csize, userbuf))
+ csize = -EFAULT;
set_iounmap_nonlazy();
iounmap((void __iomem *)vaddr);
@@ -56,7 +51,7 @@ static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
* mapped in the current kernel. We stitch up a pte, similar to kmap_atomic.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
- unsigned long offset, int userbuf)
+ unsigned long offset, bool userbuf)
{
return __copy_oldmem_page(pfn, buf, csize, offset, userbuf, false);
}
@@ -67,7 +62,7 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
* machines.
*/
ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
- unsigned long offset, int userbuf)
+ unsigned long offset, bool userbuf)
{
return __copy_oldmem_page(pfn, buf, csize, offset, userbuf, true);
}
diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index f67fd77..bba52aa 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -133,7 +133,7 @@ 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,
+ u64 *ppos, bool userbuf,
bool encrypted)
{
unsigned long pfn, offset;
@@ -233,7 +233,7 @@ int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
*/
ssize_t __weak
copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
- unsigned long offset, int userbuf)
+ unsigned long offset, bool userbuf)
{
return copy_oldmem_page(pfn, buf, csize, offset, userbuf);
}
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 6208215..033448b 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -25,10 +25,10 @@ extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
unsigned long size, pgprot_t prot);
extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
- unsigned long, int);
+ unsigned long, bool);
extern ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
size_t csize, unsigned long offset,
- int userbuf);
+ bool userbuf);
void vmcore_cleanup(void);
@@ -136,11 +136,11 @@ static inline int vmcore_add_device_dump(struct vmcoredd_data *data)
#ifdef CONFIG_PROC_VMCORE
ssize_t read_from_oldmem(char *buf, size_t count,
- u64 *ppos, int userbuf,
+ u64 *ppos, bool userbuf,
bool encrypted);
#else
static inline ssize_t read_from_oldmem(char *buf, size_t count,
- u64 *ppos, int userbuf,
+ u64 *ppos, bool userbuf,
bool encrypted)
{
return -EOPNOTSUPP;
--
2.1.0
^ permalink raw reply related
* [PATCH v2 1/2] kdump: vmcore: remove copy_to() and add copy_to_user_or_kernel()
From: Tiezhu Yang @ 2021-12-11 3:33 UTC (permalink / raw)
To: Dave Young, Baoquan He, Vivek Goyal, Andrew Morton
Cc: linux-ia64, linux-sh, Xuefeng Li, x86, kexec, linux-mips,
linux-kernel, linux-fsdevel, linux-riscv, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1639193588-7027-1-git-send-email-yangtiezhu@loongson.cn>
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(-)
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);
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 1/2] kdump: vmcore: move copy_to() from vmcore.c to uaccess.h
From: Christophe Leroy @ 2021-12-11 7:37 UTC (permalink / raw)
To: Andrew Morton, Tiezhu Yang
Cc: linux-ia64@vger.kernel.org, Baoquan He, linux-sh@vger.kernel.org,
Dave Young, x86@kernel.org, kexec@lists.infradead.org,
linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-fsdevel@vger.kernel.org, linux-riscv@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org, Vivek Goyal
In-Reply-To: <20211210085903.e7820815e738d7dc6da06050@linux-foundation.org>
Le 10/12/2021 à 17:59, Andrew Morton a écrit :
> On Fri, 10 Dec 2021 21:36:00 +0800 Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>> In arch/*/kernel/crash_dump*.c, there exist similar code about
>> copy_oldmem_page(), move copy_to() from vmcore.c to uaccess.h,
>> and then we can use copy_to() to simplify the related code.
>>
>> ...
>>
>> --- a/fs/proc/vmcore.c
>> +++ b/fs/proc/vmcore.c
>> @@ -238,20 +238,6 @@ 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)
>> {
>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>> index ac03940..4a6c3e4 100644
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -201,6 +201,20 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>> return n;
>> }
>>
>> +/*
>> + * Copy to either kernel or user space
>> + */
>> +static inline 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;
>> +}
>> +
>
> Ordinarily I'd say "this is too large to be inlined". But the function
> has only a single callsite per architecture so inlining it won't cause
> bloat at present.
>
> But hopefully copy_to() will get additional callers in the future, in
> which case it shouldn't be inlined. So I'm thinking it would be best
> to start out with this as a regular non-inlined function, in
> lib/usercopy.c.
>
> Also, copy_to() is a very poor name for a globally-visible helper
> function. Better would be copy_to_user_or_kernel(), although that's
> perhaps a bit long.
>
> And the `userbuf' arg should have type bool, yes?
>
I think keeping it inlined is better.
copy_oldmem_page() is bigger with v2 (outlined) than with v1 (inlined),
see both below:
v1:
00000000 <copy_oldmem_page>:
0: 94 21 ff e0 stwu r1,-32(r1)
4: 93 e1 00 1c stw r31,28(r1)
8: 7c bf 2b 79 mr. r31,r5
c: 40 82 00 14 bne 20 <copy_oldmem_page+0x20>
10: 83 e1 00 1c lwz r31,28(r1)
14: 38 60 00 00 li r3,0
18: 38 21 00 20 addi r1,r1,32
1c: 4e 80 00 20 blr
20: 28 1f 10 00 cmplwi r31,4096
24: 93 61 00 0c stw r27,12(r1)
28: 7c 08 02 a6 mflr r0
2c: 93 81 00 10 stw r28,16(r1)
30: 93 a1 00 14 stw r29,20(r1)
34: 7c 9b 23 78 mr r27,r4
38: 90 01 00 24 stw r0,36(r1)
3c: 7c dd 33 78 mr r29,r6
40: 93 c1 00 18 stw r30,24(r1)
44: 7c fc 3b 78 mr r28,r7
48: 40 81 00 08 ble 50 <copy_oldmem_page+0x50>
4c: 3b e0 10 00 li r31,4096
50: 54 7e 60 26 rlwinm r30,r3,12,0,19
54: 7f c3 f3 78 mr r3,r30
58: 7f e4 fb 78 mr r4,r31
5c: 48 00 00 01 bl 5c <copy_oldmem_page+0x5c>
5c: R_PPC_REL24 memblock_is_region_memory
60: 2c 03 00 00 cmpwi r3,0
64: 41 82 00 30 beq 94 <copy_oldmem_page+0x94>
68: 2c 1c 00 00 cmpwi r28,0
6c: 3f de c0 00 addis r30,r30,-16384
70: 7f 63 db 78 mr r3,r27
74: 7f e5 fb 78 mr r5,r31
78: 7c 9e ea 14 add r4,r30,r29
7c: 41 82 00 7c beq f8 <copy_oldmem_page+0xf8>
80: 48 00 00 01 bl 80 <copy_oldmem_page+0x80>
80: R_PPC_REL24 _copy_to_user
84: 2c 03 00 00 cmpwi r3,0
88: 41 a2 00 48 beq d0 <copy_oldmem_page+0xd0>
8c: 3b e0 ff f2 li r31,-14
90: 48 00 00 40 b d0 <copy_oldmem_page+0xd0>
94: 7f c3 f3 78 mr r3,r30
98: 38 a0 05 91 li r5,1425
9c: 38 80 10 00 li r4,4096
a0: 48 00 00 01 bl a0 <copy_oldmem_page+0xa0>
a0: R_PPC_REL24 ioremap_prot
a4: 2c 1c 00 00 cmpwi r28,0
a8: 7c 7e 1b 78 mr r30,r3
ac: 7c 83 ea 14 add r4,r3,r29
b0: 7f e5 fb 78 mr r5,r31
b4: 7f 63 db 78 mr r3,r27
b8: 41 82 00 48 beq 100 <copy_oldmem_page+0x100>
bc: 48 00 00 01 bl bc <copy_oldmem_page+0xbc>
bc: R_PPC_REL24 _copy_to_user
c0: 2c 03 00 00 cmpwi r3,0
c4: 40 82 00 44 bne 108 <copy_oldmem_page+0x108>
c8: 7f c3 f3 78 mr r3,r30
cc: 48 00 00 01 bl cc <copy_oldmem_page+0xcc>
cc: R_PPC_REL24 iounmap
d0: 80 01 00 24 lwz r0,36(r1)
d4: 7f e3 fb 78 mr r3,r31
d8: 83 61 00 0c lwz r27,12(r1)
dc: 83 81 00 10 lwz r28,16(r1)
e0: 7c 08 03 a6 mtlr r0
e4: 83 a1 00 14 lwz r29,20(r1)
e8: 83 c1 00 18 lwz r30,24(r1)
ec: 83 e1 00 1c lwz r31,28(r1)
f0: 38 21 00 20 addi r1,r1,32
f4: 4e 80 00 20 blr
f8: 48 00 00 01 bl f8 <copy_oldmem_page+0xf8>
f8: R_PPC_REL24 memcpy
fc: 4b ff ff d4 b d0 <copy_oldmem_page+0xd0>
100: 48 00 00 01 bl 100 <copy_oldmem_page+0x100>
100: R_PPC_REL24 memcpy
104: 4b ff ff c4 b c8 <copy_oldmem_page+0xc8>
108: 3b e0 ff f2 li r31,-14
10c: 4b ff ff bc b c8 <copy_oldmem_page+0xc8>
v2:
00000000 <copy_oldmem_page>:
0: 94 21 ff e0 stwu r1,-32(r1)
4: 93 e1 00 1c stw r31,28(r1)
8: 7c bf 2b 79 mr. r31,r5
c: 93 c1 00 18 stw r30,24(r1)
10: 3b c0 00 00 li r30,0
14: 40 82 00 18 bne 2c <copy_oldmem_page+0x2c>
18: 7f c3 f3 78 mr r3,r30
1c: 83 e1 00 1c lwz r31,28(r1)
20: 83 c1 00 18 lwz r30,24(r1)
24: 38 21 00 20 addi r1,r1,32
28: 4e 80 00 20 blr
2c: 28 1f 10 00 cmplwi r31,4096
30: 93 61 00 0c stw r27,12(r1)
34: 7c 08 02 a6 mflr r0
38: 93 81 00 10 stw r28,16(r1)
3c: 93 a1 00 14 stw r29,20(r1)
40: 7c db 33 78 mr r27,r6
44: 90 01 00 24 stw r0,36(r1)
48: 7c 9d 23 78 mr r29,r4
4c: 7c fc 3b 78 mr r28,r7
50: 40 81 00 08 ble 58 <copy_oldmem_page+0x58>
54: 3b e0 10 00 li r31,4096
58: 54 7e 60 26 rlwinm r30,r3,12,0,19
5c: 7f c3 f3 78 mr r3,r30
60: 7f e4 fb 78 mr r4,r31
64: 48 00 00 01 bl 64 <copy_oldmem_page+0x64>
64: R_PPC_REL24 memblock_is_region_memory
68: 2c 03 00 00 cmpwi r3,0
6c: 41 82 00 54 beq c0 <copy_oldmem_page+0xc0>
70: 3f de c0 00 addis r30,r30,-16384
74: 7c 9e da 14 add r4,r30,r27
78: 7f 86 e3 78 mr r6,r28
7c: 7f a3 eb 78 mr r3,r29
80: 7f e5 fb 78 mr r5,r31
84: 48 00 00 01 bl 84 <copy_oldmem_page+0x84>
84: R_PPC_REL24 copy_to_user_or_kernel
88: 3b c0 ff f2 li r30,-14
8c: 2c 03 00 00 cmpwi r3,0
90: 40 82 00 08 bne 98 <copy_oldmem_page+0x98>
94: 7f fe fb 78 mr r30,r31
98: 80 01 00 24 lwz r0,36(r1)
9c: 83 61 00 0c lwz r27,12(r1)
a0: 83 81 00 10 lwz r28,16(r1)
a4: 7c 08 03 a6 mtlr r0
a8: 83 a1 00 14 lwz r29,20(r1)
ac: 7f c3 f3 78 mr r3,r30
b0: 83 e1 00 1c lwz r31,28(r1)
b4: 83 c1 00 18 lwz r30,24(r1)
b8: 38 21 00 20 addi r1,r1,32
bc: 4e 80 00 20 blr
c0: 7f c3 f3 78 mr r3,r30
c4: 93 41 00 08 stw r26,8(r1)
c8: 38 a0 05 91 li r5,1425
cc: 38 80 10 00 li r4,4096
d0: 48 00 00 01 bl d0 <copy_oldmem_page+0xd0>
d0: R_PPC_REL24 ioremap_prot
d4: 7f 86 e3 78 mr r6,r28
d8: 7c 83 da 14 add r4,r3,r27
dc: 7c 7a 1b 78 mr r26,r3
e0: 7f e5 fb 78 mr r5,r31
e4: 7f a3 eb 78 mr r3,r29
e8: 48 00 00 01 bl e8 <copy_oldmem_page+0xe8>
e8: R_PPC_REL24 copy_to_user_or_kernel
ec: 3b c0 ff f2 li r30,-14
f0: 2c 03 00 00 cmpwi r3,0
f4: 40 82 00 08 bne fc <copy_oldmem_page+0xfc>
f8: 7f fe fb 78 mr r30,r31
fc: 7f 43 d3 78 mr r3,r26
100: 48 00 00 01 bl 100 <copy_oldmem_page+0x100>
100: R_PPC_REL24 iounmap
104: 80 01 00 24 lwz r0,36(r1)
108: 83 41 00 08 lwz r26,8(r1)
10c: 83 61 00 0c lwz r27,12(r1)
110: 7c 08 03 a6 mtlr r0
114: 83 81 00 10 lwz r28,16(r1)
118: 83 a1 00 14 lwz r29,20(r1)
11c: 4b ff ff 90 b ac <copy_oldmem_page+0xac>
Christophe
^ permalink raw reply
* [PATCH] soc: fsl: qe: fix typo in a comment
From: Jason Wang @ 2021-12-11 9:08 UTC (permalink / raw)
To: leoyang.li
Cc: Jason Wang, linuxppc-dev, linux-kernel, linux-arm-kernel,
qiang.zhao
The double `is' in the comment in line 150 is repeated. Remove one
of them from the comment.
Signed-off-by: Jason Wang <wangborong@cdjrlc.com>
---
drivers/soc/fsl/qe/qe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 4d38c80f8be8..b3c226eb5292 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -147,7 +147,7 @@ EXPORT_SYMBOL(qe_issue_cmd);
* memory mapped space.
* The BRG clock is the QE clock divided by 2.
* It was set up long ago during the initial boot phase and is
- * is given to us.
+ * given to us.
* Baud rate clocks are zero-based in the driver code (as that maps
* to port numbers). Documentation uses 1-based numbering.
*/
@@ -421,7 +421,7 @@ static void qe_upload_microcode(const void *base,
for (i = 0; i < be32_to_cpu(ucode->count); i++)
iowrite32be(be32_to_cpu(code[i]), &qe_immr->iram.idata);
-
+
/* Set I-RAM Ready Register */
iowrite32be(QE_IRAM_READY, &qe_immr->iram.iready);
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] kdump: vmcore: remove copy_to() and add copy_to_user_or_kernel()
From: Christophe Leroy @ 2021-12-11 10:32 UTC (permalink / raw)
To: Tiezhu Yang, Dave Young, Baoquan He, Vivek Goyal, Andrew Morton
Cc: linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, x86@kernel.org,
kexec@lists.infradead.org, linux-mips@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-riscv@lists.infradead.org, Xuefeng Li,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1639193588-7027-2-git-send-email-yangtiezhu@loongson.cn>
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
^ permalink raw reply
* [next-20211210] Build break powerpc/kvm: unknown member wait
From: Sachin Sant @ 2021-12-11 10:50 UTC (permalink / raw)
To: kvm, linuxppc-dev; +Cc: seanjc, linux-next
next-20211210 ( commit ea922272cbe547) powerpc build fails due to following error:
arch/powerpc/kvm/book3s_hv.c: In function 'kvmhv_run_single_vcpu':
arch/powerpc/kvm/book3s_hv.c:4591:27: error: 'struct kvm_vcpu' has no member named 'wait'
prepare_to_rcuwait(&vcpu->wait);
^~
arch/powerpc/kvm/book3s_hv.c:4608:23: error: 'struct kvm_vcpu' has no member named 'wait'
finish_rcuwait(&vcpu->wait);
^~
commit 510958e997217: KVM: Force PPC to define its own rcuwait object
introduced the error.
Thanks
-Sachin
^ permalink raw reply
* Re: [patch V3 03/35] x86/apic/msi: Use PCI device MSI property
From: Greg Kroah-Hartman @ 2021-12-11 10:52 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, Jason Gunthorpe,
linux-pci, xen-devel, Kevin Tian, Arnd Bergmann, Alex Williamson,
Cedric Le Goater, Santosh Shilimkar, Bjorn Helgaas, Megha Dey,
Laurentiu Tudor, Juergen Gross, Tero Kristo, Robin Murphy, LKML,
Vinod Koul, Marc Zygnier, dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.372357371@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:47PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* [PATCH 1/2] powerpc/code-patching: add patch_memory() for writing RO text
From: Russell Currey @ 2021-12-11 12:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: joe.lawrence, jniethe5, Russell Currey, naveen.n.rao
powerpc allocates a text poke area of one page that is used by
patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
is enabled.
patch_instruction() is only designed for instructions,
so writing data using the text poke area can only happen 4 bytes
at a time - each with a page map/unmap, pte flush and syncs.
This patch introduces patch_memory(), implementing the same
interface as memcpy(), similar to x86's text_poke() and s390's
s390_kernel_write(). patch_memory() only needs to map the text
poke area once, unless the write would cross a page boundary.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
Sorry I took so long to post this.
Some discussion here: https://github.com/linuxppc/issues/issues/375
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 73 ++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 4ba834599c4d..604211d8380c 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -31,6 +31,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
int patch_branch(u32 *addr, unsigned long target, int flags);
int patch_instruction(u32 *addr, struct ppc_inst instr);
int raw_patch_instruction(u32 *addr, struct ppc_inst instr);
+void *patch_memory(void *dest, const void *src, size_t size);
static inline unsigned long patch_site_addr(s32 *site)
{
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c5ed98823835..3a566d756ccc 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -17,6 +17,7 @@
#include <asm/code-patching.h>
#include <asm/setup.h>
#include <asm/inst.h>
+#include <asm/cacheflush.h>
static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch_addr)
{
@@ -178,6 +179,73 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
return err;
}
+
+static int do_patch_memory(void *dest, const void *src, size_t size, unsigned long poke_addr)
+{
+ unsigned long patch_addr = poke_addr + offset_in_page(dest);
+
+ if (map_patch_area(dest, poke_addr)) {
+ pr_warn("failed to map %lx\n", poke_addr);
+ return -1;
+ }
+
+ memcpy((u8 *)patch_addr, src, size);
+
+ flush_icache_range(patch_addr, size);
+
+ if (unmap_patch_area(poke_addr)) {
+ pr_warn("failed to unmap %lx\n", poke_addr);
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ * patch_memory - write data using the text poke area
+ *
+ * @dest: destination address
+ * @src: source address
+ * @size: size in bytes
+ *
+ * like memcpy(), but using the text poke area. No atomicity guarantees.
+ * Do not use for instructions, use patch_instruction() instead.
+ * Handles crossing page boundaries, though you shouldn't need to.
+ *
+ * Return value:
+ * @dest
+ **/
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ size_t bytes_written, write_size;
+ unsigned long text_poke_addr;
+ unsigned long flags;
+
+ // If the poke area isn't set up, it's early boot and we can just memcpy.
+ if (!this_cpu_read(text_poke_area))
+ return memcpy(dest, src, size);
+
+ local_irq_save(flags);
+ text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
+
+ for (bytes_written = 0;
+ bytes_written < size;
+ bytes_written += write_size) {
+ // Write as much as possible without crossing a page boundary.
+ write_size = min(size - bytes_written,
+ PAGE_SIZE - offset_in_page(dest + bytes_written));
+
+ if (do_patch_memory(dest + bytes_written,
+ src + bytes_written,
+ write_size,
+ text_poke_addr))
+ break;
+ }
+
+ local_irq_restore(flags);
+
+ return dest;
+}
#else /* !CONFIG_STRICT_KERNEL_RWX */
static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
@@ -185,6 +253,11 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
return raw_patch_instruction(addr, instr);
}
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ return memcpy(dest, src, size);
+}
+
#endif /* CONFIG_STRICT_KERNEL_RWX */
int patch_instruction(u32 *addr, struct ppc_inst instr)
--
2.34.1
^ permalink raw reply related
* [PATCH 2/2] powerpc/module_64: Use patch_memory() to apply relocations to loaded modules
From: Russell Currey @ 2021-12-11 12:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: joe.lawrence, jniethe5, Russell Currey, naveen.n.rao
In-Reply-To: <20211211123111.21044-1-ruscur@russell.cc>
Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these
writes through the text poke area by using patch_memory().
Similar to x86 and s390 implementations, apply_relocate_add() now
chooses to use patch_memory() or memcpy() depending on if the module
is loaded or not. Without STRICT_KERNEL_RWX, patch_memory() is just
memcpy(), so there should be no performance impact.
While many relocation types may not be applied in a livepatch
context, comprehensively handling them prevents any issues in future,
with no performance penalty as the text poke area is only used when
necessary.
create_stub() and create_ftrace_stub() are modified to first write
to the stack so that the ppc64_stub_entry struct only takes one
write() to modify, saving several map/unmap/flush operations
when use of patch_memory() is necessary.
This patch also contains some trivial whitespace fixes.
Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX")
Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
Some discussion here:https://github.com/linuxppc/issues/issues/375
for-stable version using patch_instruction():
https://lore.kernel.org/linuxppc-dev/20211123081520.18843-1-ruscur@russell.cc/
arch/powerpc/kernel/module_64.c | 157 +++++++++++++++++++++-----------
1 file changed, 104 insertions(+), 53 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 6baa676e7cb6..2a146750fa6f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -350,11 +350,11 @@ static u32 stub_insns[] = {
*/
static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ void *(*write)(void *, const void *, size_t))
{
long reladdr;
-
- memcpy(entry->jump, stub_insns, sizeof(stub_insns));
+ struct ppc64_stub_entry tmp_entry;
/* Stub uses address relative to kernel toc (from the paca) */
reladdr = addr - kernel_toc_addr();
@@ -364,12 +364,20 @@ static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
return 0;
}
- entry->jump[1] |= PPC_HA(reladdr);
- entry->jump[2] |= PPC_LO(reladdr);
+ /*
+ * In case @entry is write-protected, make our changes on the stack
+ * so we can update the whole struct in one write().
+ */
+ memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
+ memcpy(&tmp_entry.jump, stub_insns, sizeof(stub_insns));
+ tmp_entry.jump[1] |= PPC_HA(reladdr);
+ tmp_entry.jump[2] |= PPC_LO(reladdr);
/* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
- entry->funcdata = func_desc(addr);
- entry->magic = STUB_MAGIC;
+ tmp_entry.funcdata = func_desc(addr);
+ tmp_entry.magic = STUB_MAGIC;
+
+ write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
return 1;
}
@@ -392,7 +400,8 @@ static bool is_mprofile_ftrace_call(const char *name)
#else
static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ void *(*write)(void *, const void *, size_t))
{
return 0;
}
@@ -419,14 +428,14 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
struct ppc64_stub_entry *entry,
unsigned long addr,
struct module *me,
- const char *name)
+ const char *name,
+ void *(*write)(void *, const void *, size_t))
{
long reladdr;
+ struct ppc64_stub_entry tmp_entry;
if (is_mprofile_ftrace_call(name))
- return create_ftrace_stub(entry, addr, me);
-
- memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ return create_ftrace_stub(entry, addr, me, write);
/* Stub uses address relative to r2. */
reladdr = (unsigned long)entry - my_r2(sechdrs, me);
@@ -437,10 +446,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
}
pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
- entry->jump[0] |= PPC_HA(reladdr);
- entry->jump[1] |= PPC_LO(reladdr);
- entry->funcdata = func_desc(addr);
- entry->magic = STUB_MAGIC;
+ /*
+ * In case @entry is write-protected, make our changes on the stack
+ * so we can update the whole struct in one write().
+ */
+ memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
+
+ memcpy(&tmp_entry.jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ tmp_entry.jump[0] |= PPC_HA(reladdr);
+ tmp_entry.jump[1] |= PPC_LO(reladdr);
+ tmp_entry.funcdata = func_desc(addr);
+ tmp_entry.magic = STUB_MAGIC;
+
+ write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
return 1;
}
@@ -450,7 +468,8 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
unsigned long addr,
struct module *me,
- const char *name)
+ const char *name,
+ void *(*write)(void *, const void *, size_t))
{
struct ppc64_stub_entry *stubs;
unsigned int i, num_stubs;
@@ -467,7 +486,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
return (unsigned long)&stubs[i];
}
- if (!create_stub(sechdrs, &stubs[i], addr, me, name))
+ if (!create_stub(sechdrs, &stubs[i], addr, me, name, write))
return 0;
return (unsigned long)&stubs[i];
@@ -496,15 +515,20 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
return 0;
}
/* ld r2,R2_STACK_OFFSET(r1) */
- *instruction = PPC_INST_LD_TOC;
+ if (me->state == MODULE_STATE_UNFORMED)
+ *instruction = PPC_INST_LD_TOC;
+ else
+ patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC));
+
return 1;
}
-int apply_relocate_add(Elf64_Shdr *sechdrs,
- const char *strtab,
- unsigned int symindex,
- unsigned int relsec,
- struct module *me)
+static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me,
+ void *(*write)(void *dest, const void *src, size_t len))
{
unsigned int i;
Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
@@ -544,16 +568,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
switch (ELF64_R_TYPE(rela[i].r_info)) {
case R_PPC64_ADDR32:
/* Simply set it */
- *(u32 *)location = value;
+ write(location, &value, 4);
break;
case R_PPC64_ADDR64:
/* Simply set it */
- *(unsigned long *)location = value;
+ write(location, &value, 8);
break;
case R_PPC64_TOC:
- *(unsigned long *)location = my_r2(sechdrs, me);
+ value = my_r2(sechdrs, me);
+ write(location, &value, 8);
break;
case R_PPC64_TOC16:
@@ -564,17 +589,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_LO:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_DS:
@@ -585,9 +610,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xfffc)
+ value = (*((uint16_t *) location) & ~0xfffc)
| (value & 0xfffc);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_LO_DS:
@@ -598,18 +623,18 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xfffc)
+ value = (*((uint16_t *) location) & ~0xfffc)
| (value & 0xfffc);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_HA:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
value = ((value + 0x8000) >> 16);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC_REL24:
@@ -618,14 +643,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me,
- strtab + sym->st_name);
+ strtab + sym->st_name, write);
if (!value)
return -ENOENT;
if (!restore_r2(strtab + sym->st_name,
(u32 *)location + 1, me))
return -ENOEXEC;
- } else
+ } else {
value += local_entry_offset(sym);
+ }
/* Convert value to relative */
value -= (unsigned long)location;
@@ -636,14 +662,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
}
/* Only replace bits 2 through 26 */
- *(uint32_t *)location
- = (*(uint32_t *)location & ~0x03fffffc)
+ value = (*(uint32_t *)location & ~0x03fffffc)
| (value & 0x03fffffc);
+ write(location, &value, 4);
break;
case R_PPC64_REL64:
/* 64 bits relative (used by features fixups) */
- *location = value - (unsigned long)location;
+ value -= (unsigned long)location;
+ write(location, &value, 8);
break;
case R_PPC64_REL32:
@@ -655,7 +682,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, (long int)value);
return -ENOEXEC;
}
- *(u32 *)location = value;
+ write(location, &value, 4);
break;
case R_PPC64_TOCSAVE:
@@ -676,7 +703,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
break;
/*
* Check for the large code model prolog sequence:
- * ld r2, ...(r12)
+ * ld r2, ...(r12)
* add r2, r2, r12
*/
if ((((uint32_t *)location)[0] & ~0xfffc) != PPC_RAW_LD(_R2, _R12, 0))
@@ -688,25 +715,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
* addis r2, r12, (.TOC.-func)@ha
* addi r2, r2, (.TOC.-func)@l
*/
- ((uint32_t *)location)[0] = PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value));
- ((uint32_t *)location)[1] = PPC_RAW_ADDI(_R2, _R2, PPC_LO(value));
+ patch_instruction(&((uint32_t *)location)[0],
+ ppc_inst(PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value))));
+ patch_instruction(&((uint32_t *)location)[1],
+ ppc_inst(PPC_RAW_ADDI(_R2, _R2, PPC_LO(value))));
break;
case R_PPC64_REL16_HA:
/* Subtract location pointer */
value -= (unsigned long)location;
value = ((value + 0x8000) >> 16);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_REL16_LO:
/* Subtract location pointer */
value -= (unsigned long)location;
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
default:
@@ -720,6 +749,20 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return 0;
}
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me)
+{
+ void *(*write)(void *, const void *, size_t) = memcpy;
+ bool early = me->state == MODULE_STATE_UNFORMED;
+
+ if (!early)
+ write = patch_memory;
+
+ return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, write);
+}
#ifdef CONFIG_DYNAMIC_FTRACE
int module_trampoline_target(struct module *mod, unsigned long addr,
unsigned long *target)
@@ -749,7 +792,7 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
if (copy_from_kernel_nofault(&funcdata, &stub->funcdata,
sizeof(funcdata))) {
pr_err("%s: fault reading funcdata for stub %lx for %s\n", __func__, addr, mod->name);
- return -EFAULT;
+ return -EFAULT;
}
*target = stub_func_addr(funcdata);
@@ -759,15 +802,23 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
{
+ void *(*write)(void *, const void *, size_t) = memcpy;
+ bool early = mod->state == MODULE_STATE_UNFORMED;
+
+ if (!early)
+ write = patch_memory;
+
mod->arch.tramp = stub_for_addr(sechdrs,
(unsigned long)ftrace_caller,
mod,
- "ftrace_caller");
+ "ftrace_caller",
+ write);
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
mod->arch.tramp_regs = stub_for_addr(sechdrs,
(unsigned long)ftrace_regs_caller,
mod,
- "ftrace_regs_caller");
+ "ftrace_regs_caller",
+ write);
if (!mod->arch.tramp_regs)
return -ENOENT;
#endif
--
2.34.1
^ permalink raw reply related
* Re: [patch V3 05/35] powerpc/cell/axon_msi: Use PCI device property
From: Arnd Bergmann @ 2021-12-11 15:21 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya,
open list:IOMMU DRIVERS, Peter Ujfalusi, Bjorn Helgaas, Linux ARM,
Jason Gunthorpe, linux-pci, xen-devel, Kevin Tian, Arnd Bergmann,
Robin Murphy, Alex Williamson, Cedric Le Goater,
Santosh Shilimkar, Bjorn Helgaas, Megha Dey, Laurentiu Tudor,
Juergen Gross, Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul,
Marc Zygnier, dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.493922179@linutronix.de>
On Fri, Dec 10, 2021 at 11:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [patch V3 07/35] device: Move MSI related data into a struct
From: Arnd Bergmann @ 2021-12-11 15:22 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya,
open list:IOMMU DRIVERS, Peter Ujfalusi, Bjorn Helgaas, Linux ARM,
Jason Gunthorpe, linux-pci, xen-devel, Kevin Tian, Arnd Bergmann,
Robin Murphy, Alex Williamson, Cedric Le Goater,
Santosh Shilimkar, Bjorn Helgaas, Megha Dey, Laurentiu Tudor,
Juergen Gross, Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul,
Marc Zygnier, dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.617178827@linutronix.de>
On Fri, Dec 10, 2021 at 11:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> The only unconditional part of MSI data in struct device is the irqdomain
> pointer. Everything else can be allocated on demand. Create a data
> structure and move the irqdomain pointer into it. The other MSI specific
> parts are going to be removed from struct device in later steps.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [patch V3 34/35] soc: ti: ti_sci_inta_msi: Get rid of ti_sci_inta_msi_get_virq()
From: Arnd Bergmann @ 2021-12-11 15:24 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Peter Ujfalusi, Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya,
open list:IOMMU DRIVERS, Bjorn Helgaas, Linux ARM,
Jason Gunthorpe, linux-pci, xen-devel, Kevin Tian, Arnd Bergmann,
Robin Murphy, Alex Williamson, Cedric Le Goater,
Santosh Shilimkar, Bjorn Helgaas, Megha Dey, Laurentiu Tudor,
Juergen Gross, Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul,
Marc Zygnier, dmaengine, linuxppc-dev
In-Reply-To: <20211210221815.269468319@linutronix.de>
On Fri, Dec 10, 2021 at 11:19 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Just use the core function msi_get_virq().
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: dmaengine@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [patch V3 12/35] soc: ti: ti_sci_inta_msi: Allocate MSI device data on first use
From: Arnd Bergmann @ 2021-12-11 15:22 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya,
open list:IOMMU DRIVERS, Peter Ujfalusi, Bjorn Helgaas, Linux ARM,
Jason Gunthorpe, linux-pci, xen-devel, Kevin Tian, Arnd Bergmann,
Robin Murphy, Alex Williamson, Cedric Le Goater,
Santosh Shilimkar, Bjorn Helgaas, Megha Dey, Laurentiu Tudor,
Juergen Gross, Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul,
Marc Zygnier, dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.928842960@linutronix.de>
On Fri, Dec 10, 2021 at 11:19 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Allocate the MSI device data on first invocation of the allocation function.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Tero Kristo <kristo@kernel.org>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH] PCI/AER: potential dereference of null pointer
From: Bjorn Helgaas @ 2021-12-11 17:54 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: linux-pci, linux-kernel, oohall, bhelgaas, Rajat Jain,
linuxppc-dev
In-Reply-To: <20211209094556.2085357-1-jiasheng@iscas.ac.cn>
[+cc Rajat, author of aer_stats:
db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices"
81aa5206f9a7 ("PCI/AER: Add sysfs attributes to provide AER stats and breakdown"]
On Thu, Dec 09, 2021 at 05:45:56PM +0800, Jiasheng Jiang wrote:
> he return value of kzalloc() needs to be checked.
> To avoid use of null pointer in case of the failure of alloc.
>
> Fixes: db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
> drivers/pci/pcie/aer.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index ec943cee5ecc..d04303edf468 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -376,6 +376,8 @@ void pci_aer_init(struct pci_dev *dev)
> return;
>
> dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> + if (!dev->aer_stats)
> + return;
Did you actually trip over a null pointer dereference, and if so,
where was it?
I think the intent here was that aer_stats is a non-essential feature,
and if we can't allocate space to keep the statistics, we can still
use the device without the stats.
I *think* all the users of dev->aer_stats check for NULL before
dereferencing it, but if you found a case that doesn't do that, we
should definitely fix it.
In a few cases (aer_stats_dev_attr, aer_stats_rootport_attr), the
check isn't obvious -- it happens in aer_stats_attrs_are_visible().
If aer_stats_attrs_are_visible() finds that aer_stats is NULL, those
sysfs attributes should not be visible, and the corresponding *_show()
functions should never be called.
> /*
> * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
> --
> 2.25.1
>
^ permalink raw reply
* RE: [PATCH v2 0/2] kdump: simplify code
From: David Laight @ 2021-12-11 17:53 UTC (permalink / raw)
To: 'Tiezhu Yang', Dave Young, Baoquan He, Vivek Goyal,
Andrew Morton
Cc: linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org, Xuefeng Li,
x86@kernel.org, kexec@lists.infradead.org,
linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-riscv@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1639193588-7027-1-git-send-email-yangtiezhu@loongson.cn>
From: Tiezhu Yang
> Sent: 11 December 2021 03:33
>
> v2:
> -- add copy_to_user_or_kernel() in lib/usercopy.c
> -- define userbuf as bool type
Instead of having a flag to indicate whether the buffer is user or kernel,
would it be better to have two separate buffer pointers.
One for a user space buffer, the other for a kernel space buffer.
Exactly one of the buffers should always be NULL.
That way the flag is never incorrectly set.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 0/2] kdump: simplify code
From: Christophe Leroy @ 2021-12-11 19:32 UTC (permalink / raw)
To: David Laight, 'Tiezhu Yang', Dave Young, Baoquan He,
Vivek Goyal, Andrew Morton
Cc: linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, x86@kernel.org,
kexec@lists.infradead.org, linux-mips@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-riscv@lists.infradead.org, Xuefeng Li,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <0c5cb37139af4f3e85cc2c5115d7d006@AcuMS.aculab.com>
Le 11/12/2021 à 18:53, David Laight a écrit :
> From: Tiezhu Yang
>> Sent: 11 December 2021 03:33
>>
>> v2:
>> -- add copy_to_user_or_kernel() in lib/usercopy.c
>> -- define userbuf as bool type
>
> Instead of having a flag to indicate whether the buffer is user or kernel,
> would it be better to have two separate buffer pointers.
> One for a user space buffer, the other for a kernel space buffer.
> Exactly one of the buffers should always be NULL.
>
> That way the flag is never incorrectly set.
>
It's a very good idea.
I was worried about the casts forcing the __user property away and back.
With that approach we will preserve the __user tags on user buffers and
enable sparse checking.
The only little drawback I see is that apparently GCC doesn't consider
the NULL value as a constant and therefore doesn't perform constant
folding on pointers. Not sure if this is a problem here.
Christophe
^ permalink raw reply
* [PATCH v2 1/2] powerpc/code-patching: add patch_memory() for writing RO text
From: Russell Currey @ 2021-12-12 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: joe.lawrence, jniethe5, Russell Currey, joel, naveen.n.rao
powerpc allocates a text poke area of one page that is used by
patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
is enabled.
patch_instruction() is only designed for instructions,
so writing data using the text poke area can only happen 4 bytes
at a time - each with a page map/unmap, pte flush and syncs.
This patch introduces patch_memory(), implementing the same
interface as memcpy(), similar to x86's text_poke() and s390's
s390_kernel_write(). patch_memory() only needs to map the text
poke area once, unless the write would cross a page boundary.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v2: Use min_t() instead of min(), fixing the 32-bit build as reported
by snowpatch.
Some discussion here: https://github.com/linuxppc/issues/issues/375
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 74 ++++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 4ba834599c4d..604211d8380c 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -31,6 +31,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
int patch_branch(u32 *addr, unsigned long target, int flags);
int patch_instruction(u32 *addr, struct ppc_inst instr);
int raw_patch_instruction(u32 *addr, struct ppc_inst instr);
+void *patch_memory(void *dest, const void *src, size_t size);
static inline unsigned long patch_site_addr(s32 *site)
{
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c5ed98823835..330602aa59f1 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -17,6 +17,7 @@
#include <asm/code-patching.h>
#include <asm/setup.h>
#include <asm/inst.h>
+#include <asm/cacheflush.h>
static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch_addr)
{
@@ -178,6 +179,74 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
return err;
}
+
+static int do_patch_memory(void *dest, const void *src, size_t size, unsigned long poke_addr)
+{
+ unsigned long patch_addr = poke_addr + offset_in_page(dest);
+
+ if (map_patch_area(dest, poke_addr)) {
+ pr_warn("failed to map %lx\n", poke_addr);
+ return -1;
+ }
+
+ memcpy((u8 *)patch_addr, src, size);
+
+ flush_icache_range(patch_addr, size);
+
+ if (unmap_patch_area(poke_addr)) {
+ pr_warn("failed to unmap %lx\n", poke_addr);
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ * patch_memory - write data using the text poke area
+ *
+ * @dest: destination address
+ * @src: source address
+ * @size: size in bytes
+ *
+ * like memcpy(), but using the text poke area. No atomicity guarantees.
+ * Do not use for instructions, use patch_instruction() instead.
+ * Handles crossing page boundaries, though you shouldn't need to.
+ *
+ * Return value:
+ * @dest
+ **/
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ size_t bytes_written, write_size;
+ unsigned long text_poke_addr;
+ unsigned long flags;
+
+ // If the poke area isn't set up, it's early boot and we can just memcpy.
+ if (!this_cpu_read(text_poke_area))
+ return memcpy(dest, src, size);
+
+ local_irq_save(flags);
+ text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
+
+ for (bytes_written = 0;
+ bytes_written < size;
+ bytes_written += write_size) {
+ // Write as much as possible without crossing a page boundary.
+ write_size = min_t(size_t,
+ size - bytes_written,
+ PAGE_SIZE - offset_in_page(dest + bytes_written));
+
+ if (do_patch_memory(dest + bytes_written,
+ src + bytes_written,
+ write_size,
+ text_poke_addr))
+ break;
+ }
+
+ local_irq_restore(flags);
+
+ return dest;
+}
#else /* !CONFIG_STRICT_KERNEL_RWX */
static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
@@ -185,6 +254,11 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
return raw_patch_instruction(addr, instr);
}
+void *patch_memory(void *dest, const void *src, size_t size)
+{
+ return memcpy(dest, src, size);
+}
+
#endif /* CONFIG_STRICT_KERNEL_RWX */
int patch_instruction(u32 *addr, struct ppc_inst instr)
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/2] powerpc/module_64: Use patch_memory() to apply relocations to loaded modules
From: Russell Currey @ 2021-12-12 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: joe.lawrence, jniethe5, Russell Currey, joel, naveen.n.rao
In-Reply-To: <20211212010357.16280-1-ruscur@russell.cc>
Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these
writes through the text poke area by using patch_memory().
Similar to x86 and s390 implementations, apply_relocate_add() now
chooses to use patch_memory() or memcpy() depending on if the module
is loaded or not. Without STRICT_KERNEL_RWX, patch_memory() is just
memcpy(), so there should be no performance impact.
While many relocation types may not be applied in a livepatch
context, comprehensively handling them prevents any issues in future,
with no performance penalty as the text poke area is only used when
necessary.
create_stub() and create_ftrace_stub() are modified to first write
to the stack so that the ppc64_stub_entry struct only takes one
write() to modify, saving several map/unmap/flush operations
when use of patch_memory() is necessary.
This patch also contains some trivial whitespace fixes.
Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX")
Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v2: No changes.
Some discussion here:https://github.com/linuxppc/issues/issues/375
for-stable version using patch_instruction():
https://lore.kernel.org/linuxppc-dev/20211123081520.18843-1-ruscur@russell.cc/
arch/powerpc/kernel/module_64.c | 157 +++++++++++++++++++++-----------
1 file changed, 104 insertions(+), 53 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 6baa676e7cb6..2a146750fa6f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -350,11 +350,11 @@ static u32 stub_insns[] = {
*/
static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ void *(*write)(void *, const void *, size_t))
{
long reladdr;
-
- memcpy(entry->jump, stub_insns, sizeof(stub_insns));
+ struct ppc64_stub_entry tmp_entry;
/* Stub uses address relative to kernel toc (from the paca) */
reladdr = addr - kernel_toc_addr();
@@ -364,12 +364,20 @@ static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
return 0;
}
- entry->jump[1] |= PPC_HA(reladdr);
- entry->jump[2] |= PPC_LO(reladdr);
+ /*
+ * In case @entry is write-protected, make our changes on the stack
+ * so we can update the whole struct in one write().
+ */
+ memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
+ memcpy(&tmp_entry.jump, stub_insns, sizeof(stub_insns));
+ tmp_entry.jump[1] |= PPC_HA(reladdr);
+ tmp_entry.jump[2] |= PPC_LO(reladdr);
/* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
- entry->funcdata = func_desc(addr);
- entry->magic = STUB_MAGIC;
+ tmp_entry.funcdata = func_desc(addr);
+ tmp_entry.magic = STUB_MAGIC;
+
+ write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
return 1;
}
@@ -392,7 +400,8 @@ static bool is_mprofile_ftrace_call(const char *name)
#else
static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ void *(*write)(void *, const void *, size_t))
{
return 0;
}
@@ -419,14 +428,14 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
struct ppc64_stub_entry *entry,
unsigned long addr,
struct module *me,
- const char *name)
+ const char *name,
+ void *(*write)(void *, const void *, size_t))
{
long reladdr;
+ struct ppc64_stub_entry tmp_entry;
if (is_mprofile_ftrace_call(name))
- return create_ftrace_stub(entry, addr, me);
-
- memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ return create_ftrace_stub(entry, addr, me, write);
/* Stub uses address relative to r2. */
reladdr = (unsigned long)entry - my_r2(sechdrs, me);
@@ -437,10 +446,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
}
pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
- entry->jump[0] |= PPC_HA(reladdr);
- entry->jump[1] |= PPC_LO(reladdr);
- entry->funcdata = func_desc(addr);
- entry->magic = STUB_MAGIC;
+ /*
+ * In case @entry is write-protected, make our changes on the stack
+ * so we can update the whole struct in one write().
+ */
+ memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
+
+ memcpy(&tmp_entry.jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ tmp_entry.jump[0] |= PPC_HA(reladdr);
+ tmp_entry.jump[1] |= PPC_LO(reladdr);
+ tmp_entry.funcdata = func_desc(addr);
+ tmp_entry.magic = STUB_MAGIC;
+
+ write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
return 1;
}
@@ -450,7 +468,8 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
unsigned long addr,
struct module *me,
- const char *name)
+ const char *name,
+ void *(*write)(void *, const void *, size_t))
{
struct ppc64_stub_entry *stubs;
unsigned int i, num_stubs;
@@ -467,7 +486,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
return (unsigned long)&stubs[i];
}
- if (!create_stub(sechdrs, &stubs[i], addr, me, name))
+ if (!create_stub(sechdrs, &stubs[i], addr, me, name, write))
return 0;
return (unsigned long)&stubs[i];
@@ -496,15 +515,20 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
return 0;
}
/* ld r2,R2_STACK_OFFSET(r1) */
- *instruction = PPC_INST_LD_TOC;
+ if (me->state == MODULE_STATE_UNFORMED)
+ *instruction = PPC_INST_LD_TOC;
+ else
+ patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC));
+
return 1;
}
-int apply_relocate_add(Elf64_Shdr *sechdrs,
- const char *strtab,
- unsigned int symindex,
- unsigned int relsec,
- struct module *me)
+static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me,
+ void *(*write)(void *dest, const void *src, size_t len))
{
unsigned int i;
Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
@@ -544,16 +568,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
switch (ELF64_R_TYPE(rela[i].r_info)) {
case R_PPC64_ADDR32:
/* Simply set it */
- *(u32 *)location = value;
+ write(location, &value, 4);
break;
case R_PPC64_ADDR64:
/* Simply set it */
- *(unsigned long *)location = value;
+ write(location, &value, 8);
break;
case R_PPC64_TOC:
- *(unsigned long *)location = my_r2(sechdrs, me);
+ value = my_r2(sechdrs, me);
+ write(location, &value, 8);
break;
case R_PPC64_TOC16:
@@ -564,17 +589,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_LO:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_DS:
@@ -585,9 +610,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xfffc)
+ value = (*((uint16_t *) location) & ~0xfffc)
| (value & 0xfffc);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_LO_DS:
@@ -598,18 +623,18 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, value);
return -ENOEXEC;
}
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xfffc)
+ value = (*((uint16_t *) location) & ~0xfffc)
| (value & 0xfffc);
+ write(location, &value, 2);
break;
case R_PPC64_TOC16_HA:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
value = ((value + 0x8000) >> 16);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC_REL24:
@@ -618,14 +643,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me,
- strtab + sym->st_name);
+ strtab + sym->st_name, write);
if (!value)
return -ENOENT;
if (!restore_r2(strtab + sym->st_name,
(u32 *)location + 1, me))
return -ENOEXEC;
- } else
+ } else {
value += local_entry_offset(sym);
+ }
/* Convert value to relative */
value -= (unsigned long)location;
@@ -636,14 +662,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
}
/* Only replace bits 2 through 26 */
- *(uint32_t *)location
- = (*(uint32_t *)location & ~0x03fffffc)
+ value = (*(uint32_t *)location & ~0x03fffffc)
| (value & 0x03fffffc);
+ write(location, &value, 4);
break;
case R_PPC64_REL64:
/* 64 bits relative (used by features fixups) */
- *location = value - (unsigned long)location;
+ value -= (unsigned long)location;
+ write(location, &value, 8);
break;
case R_PPC64_REL32:
@@ -655,7 +682,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->name, (long int)value);
return -ENOEXEC;
}
- *(u32 *)location = value;
+ write(location, &value, 4);
break;
case R_PPC64_TOCSAVE:
@@ -676,7 +703,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
break;
/*
* Check for the large code model prolog sequence:
- * ld r2, ...(r12)
+ * ld r2, ...(r12)
* add r2, r2, r12
*/
if ((((uint32_t *)location)[0] & ~0xfffc) != PPC_RAW_LD(_R2, _R12, 0))
@@ -688,25 +715,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
* addis r2, r12, (.TOC.-func)@ha
* addi r2, r2, (.TOC.-func)@l
*/
- ((uint32_t *)location)[0] = PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value));
- ((uint32_t *)location)[1] = PPC_RAW_ADDI(_R2, _R2, PPC_LO(value));
+ patch_instruction(&((uint32_t *)location)[0],
+ ppc_inst(PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value))));
+ patch_instruction(&((uint32_t *)location)[1],
+ ppc_inst(PPC_RAW_ADDI(_R2, _R2, PPC_LO(value))));
break;
case R_PPC64_REL16_HA:
/* Subtract location pointer */
value -= (unsigned long)location;
value = ((value + 0x8000) >> 16);
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
case R_PPC64_REL16_LO:
/* Subtract location pointer */
value -= (unsigned long)location;
- *((uint16_t *) location)
- = (*((uint16_t *) location) & ~0xffff)
+ value = (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
+ write(location, &value, 2);
break;
default:
@@ -720,6 +749,20 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return 0;
}
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me)
+{
+ void *(*write)(void *, const void *, size_t) = memcpy;
+ bool early = me->state == MODULE_STATE_UNFORMED;
+
+ if (!early)
+ write = patch_memory;
+
+ return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, write);
+}
#ifdef CONFIG_DYNAMIC_FTRACE
int module_trampoline_target(struct module *mod, unsigned long addr,
unsigned long *target)
@@ -749,7 +792,7 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
if (copy_from_kernel_nofault(&funcdata, &stub->funcdata,
sizeof(funcdata))) {
pr_err("%s: fault reading funcdata for stub %lx for %s\n", __func__, addr, mod->name);
- return -EFAULT;
+ return -EFAULT;
}
*target = stub_func_addr(funcdata);
@@ -759,15 +802,23 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
{
+ void *(*write)(void *, const void *, size_t) = memcpy;
+ bool early = mod->state == MODULE_STATE_UNFORMED;
+
+ if (!early)
+ write = patch_memory;
+
mod->arch.tramp = stub_for_addr(sechdrs,
(unsigned long)ftrace_caller,
mod,
- "ftrace_caller");
+ "ftrace_caller",
+ write);
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
mod->arch.tramp_regs = stub_for_addr(sechdrs,
(unsigned long)ftrace_regs_caller,
mod,
- "ftrace_regs_caller");
+ "ftrace_regs_caller",
+ write);
if (!mod->arch.tramp_regs)
return -ENOENT;
#endif
--
2.34.1
^ permalink raw reply related
* [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
From: Stefan Berger @ 2021-12-12 1:28 UTC (permalink / raw)
To: jarkko, peterhuewe, linux-integrity
Cc: Korrapati.Likhitha, pavrampu, linux-kernel, jgg,
linux-security-module, gcwilson, linuxppc-dev, Stefan Berger
Fix the following crash on kexec by checking chip->ops for a NULL pointer
in tpm_chip_start() and returning an error code if this is the case.
BUG: Kernel NULL pointer dereference on read at 0x00000060
Faulting instruction address: 0xc00000000099a06c
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c00000000099a06c] tpm_chip_start+0x2c/0x140
LR [c00000000099a808] tpm_chip_unregister+0x108/0x170
Call Trace:
[c0000000188bfa00] [c000000002b03930] fw_devlink_strict+0x0/0x8 (unreliable)
[c0000000188bfa30] [c00000000099a808] tpm_chip_unregister+0x108/0x170
[c0000000188bfa70] [c0000000009a3874] tpm_ibmvtpm_remove+0x34/0x130
[c0000000188bfae0] [c000000000110dbc] vio_bus_remove+0x5c/0xb0
[c0000000188bfb20] [c0000000009bc154] device_shutdown+0x1d4/0x3a8
[c0000000188bfbc0] [c000000000196e14] kernel_restart_prepare+0x54/0x70
The referenced patch below introduced a function to shut down the VIO bus.
The bus shutdown now calls tpm_del_char_device (via tpm_chip_unregister)
after a call to tpm_class_shutdown, which already set chip->ops to NULL.
The crash occurrs when tpm_del_char_device calls tpm_chip_start with the
chip->ops NULL pointer.
Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and vio_bus")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
drivers/char/tpm/tpm-chip.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb7e109..cca1bde296ee 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -101,6 +101,9 @@ int tpm_chip_start(struct tpm_chip *chip)
{
int ret;
+ if (!chip->ops)
+ return -EINVAL;
+
tpm_clk_enable(chip);
if (chip->locality == -1) {
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] powerpc/code-patching: add patch_memory() for writing RO text
From: Christophe Leroy @ 2021-12-12 9:08 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev@lists.ozlabs.org
Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com,
joe.lawrence@redhat.com, joel@jms.id.au
In-Reply-To: <20211212010357.16280-1-ruscur@russell.cc>
Le 12/12/2021 à 02:03, Russell Currey a écrit :
> powerpc allocates a text poke area of one page that is used by
> patch_instruction() to modify read-only text when STRICT_KERNEL_RWX
> is enabled.
>
> patch_instruction() is only designed for instructions,
> so writing data using the text poke area can only happen 4 bytes
> at a time - each with a page map/unmap, pte flush and syncs.
>
> This patch introduces patch_memory(), implementing the same
> interface as memcpy(), similar to x86's text_poke() and s390's
> s390_kernel_write(). patch_memory() only needs to map the text
> poke area once, unless the write would cross a page boundary.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v2: Use min_t() instead of min(), fixing the 32-bit build as reported
> by snowpatch.
>
> Some discussion here: https://github.com/linuxppc/issues/issues/375
>
> arch/powerpc/include/asm/code-patching.h | 1 +
> arch/powerpc/lib/code-patching.c | 74 ++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 4ba834599c4d..604211d8380c 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -31,6 +31,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
> int patch_branch(u32 *addr, unsigned long target, int flags);
> int patch_instruction(u32 *addr, struct ppc_inst instr);
> int raw_patch_instruction(u32 *addr, struct ppc_inst instr);
> +void *patch_memory(void *dest, const void *src, size_t size);
>
> static inline unsigned long patch_site_addr(s32 *site)
> {
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index c5ed98823835..330602aa59f1 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -17,6 +17,7 @@
> #include <asm/code-patching.h>
> #include <asm/setup.h>
> #include <asm/inst.h>
> +#include <asm/cacheflush.h>
>
> static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch_addr)
> {
> @@ -178,6 +179,74 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
>
> return err;
> }
> +
> +static int do_patch_memory(void *dest, const void *src, size_t size, unsigned long poke_addr)
> +{
> + unsigned long patch_addr = poke_addr + offset_in_page(dest);
> +
> + if (map_patch_area(dest, poke_addr)) {
> + pr_warn("failed to map %lx\n", poke_addr);
It isn't worth a warning here. If that happens before slab is available,
it will panic in early_alloc_pgtable().
If it happens after, you will already get a pile of messages dumping the
memory state etc ...
During the last few years, pr_ messages have been removed from most
places where ENOMEM is returned.
> + return -1;
> + }
I have a series reworking error handling at
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=274823&state=*
Especially this one handles map_patch_area() :
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/85259d894069e47f915ea580b169e1adbeec7a61.1638446239.git.christophe.leroy@csgroup.eu/
Would be good if you could rebase your series on top of it.
> +
> + memcpy((u8 *)patch_addr, src, size);
Shouldn't we use copy_to_kernel_nofault(), so that we survive from a
fault just like patch_instruction() ?
> +
> + flush_icache_range(patch_addr, size);
> +
> + if (unmap_patch_area(poke_addr)) {
> + pr_warn("failed to unmap %lx\n", poke_addr);
> + return -1;
> + }
I have changed unmap_page_area() to a void in
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/299804b117fae35c786c827536c91f25352e279b.1638446239.git.christophe.leroy@csgroup.eu/
> +
> + return 0;
> +}
> +
> +/**
> + * patch_memory - write data using the text poke area
> + *
> + * @dest: destination address
> + * @src: source address
> + * @size: size in bytes
> + *
> + * like memcpy(), but using the text poke area. No atomicity guarantees.
> + * Do not use for instructions, use patch_instruction() instead.
> + * Handles crossing page boundaries, though you shouldn't need to.
> + *
> + * Return value:
> + * @dest
> + **/
> +void *patch_memory(void *dest, const void *src, size_t size)
> +{
> + size_t bytes_written, write_size;
> + unsigned long text_poke_addr;
> + unsigned long flags;
> +
> + // If the poke area isn't set up, it's early boot and we can just memcpy.
> + if (!this_cpu_read(text_poke_area))
> + return memcpy(dest, src, size);
> +
> + local_irq_save(flags);
Do we want to do such potentially big copies with interrupts disabled ?
> + text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> +
> + for (bytes_written = 0;
> + bytes_written < size;
> + bytes_written += write_size) {
I recommend you to read
https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=coding%20style#naming
As explained there, local variable names should be short. Using long
names is non-productive.
You could just call it "written", it would allow you to keep the for()
on a single line, that would be a lot more readable.
> + // Write as much as possible without crossing a page boundary.
> + write_size = min_t(size_t,
> + size - bytes_written,
> + PAGE_SIZE - offset_in_page(dest + bytes_written));
Reduce the size of you variable names and keep it on a single line.
> +
> + if (do_patch_memory(dest + bytes_written,
> + src + bytes_written,
> + write_size,
> + text_poke_addr))
Same, keep a single line as much as possible.
> + break;
> + }
> +
> + local_irq_restore(flags);
> +
> + return dest;
Maybe it would be better to return ERR_PTR() of the error returned by
do_page_memory().
> +}
> #else /* !CONFIG_STRICT_KERNEL_RWX */
>
> static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> @@ -185,6 +254,11 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> return raw_patch_instruction(addr, instr);
> }
>
> +void *patch_memory(void *dest, const void *src, size_t size)
> +{
> + return memcpy(dest, src, size);
> +}
> +
> #endif /* CONFIG_STRICT_KERNEL_RWX */
>
> int patch_instruction(u32 *addr, struct ppc_inst instr)
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/module_64: Use patch_memory() to apply relocations to loaded modules
From: Christophe Leroy @ 2021-12-12 10:41 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev@lists.ozlabs.org
Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com,
joe.lawrence@redhat.com, joel@jms.id.au
In-Reply-To: <20211212010357.16280-2-ruscur@russell.cc>
Le 12/12/2021 à 02:03, Russell Currey a écrit :
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these
> writes through the text poke area by using patch_memory().
>
> Similar to x86 and s390 implementations, apply_relocate_add() now
> chooses to use patch_memory() or memcpy() depending on if the module
> is loaded or not. Without STRICT_KERNEL_RWX, patch_memory() is just
> memcpy(), so there should be no performance impact.
>
> While many relocation types may not be applied in a livepatch
> context, comprehensively handling them prevents any issues in future,
> with no performance penalty as the text poke area is only used when
> necessary.
>
> create_stub() and create_ftrace_stub() are modified to first write
> to the stack so that the ppc64_stub_entry struct only takes one
> write() to modify, saving several map/unmap/flush operations
> when use of patch_memory() is necessary.
>
> This patch also contains some trivial whitespace fixes.
>
> Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX")
> Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v2: No changes.
>
> Some discussion here:https://github.com/linuxppc/issues/issues/375
> for-stable version using patch_instruction():
> https://lore.kernel.org/linuxppc-dev/20211123081520.18843-1-ruscur@russell.cc/
>
> arch/powerpc/kernel/module_64.c | 157 +++++++++++++++++++++-----------
> 1 file changed, 104 insertions(+), 53 deletions(-)
>
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 6baa676e7cb6..2a146750fa6f 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -350,11 +350,11 @@ static u32 stub_insns[] = {
> */
> static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
> unsigned long addr,
> - struct module *me)
> + struct module *me,
> + void *(*write)(void *, const void *, size_t))
I really dislike this write() parameter to the function.
I think it would be better to define a static sub-function that takes
write()'s parameters plus the 'struct module *me' and have it call
either patch_memory() or memcpy() based on me->state.
> {
> long reladdr;
> -
> - memcpy(entry->jump, stub_insns, sizeof(stub_insns));
> + struct ppc64_stub_entry tmp_entry;
>
> /* Stub uses address relative to kernel toc (from the paca) */
> reladdr = addr - kernel_toc_addr();
> @@ -364,12 +364,20 @@ static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
> return 0;
> }
>
> - entry->jump[1] |= PPC_HA(reladdr);
> - entry->jump[2] |= PPC_LO(reladdr);
> + /*
> + * In case @entry is write-protected, make our changes on the stack
> + * so we can update the whole struct in one write().
> + */
> + memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
That copy seems unnecessary, entry is a struct with three fields and you
are setting all three field below.
>
> + memcpy(&tmp_entry.jump, stub_insns, sizeof(stub_insns));
> + tmp_entry.jump[1] |= PPC_HA(reladdr);
> + tmp_entry.jump[2] |= PPC_LO(reladdr);
> /* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
> - entry->funcdata = func_desc(addr);
> - entry->magic = STUB_MAGIC;
> + tmp_entry.funcdata = func_desc(addr);
> + tmp_entry.magic = STUB_MAGIC;
> +
> + write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
>
> return 1;
> }
> @@ -392,7 +400,8 @@ static bool is_mprofile_ftrace_call(const char *name)
> #else
> static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
> unsigned long addr,
> - struct module *me)
> + struct module *me,
> + void *(*write)(void *, const void *, size_t))
> {
> return 0;
> }
> @@ -419,14 +428,14 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
> struct ppc64_stub_entry *entry,
> unsigned long addr,
> struct module *me,
> - const char *name)
> + const char *name,
> + void *(*write)(void *, const void *, size_t))
> {
> long reladdr;
> + struct ppc64_stub_entry tmp_entry;
>
> if (is_mprofile_ftrace_call(name))
> - return create_ftrace_stub(entry, addr, me);
> -
> - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> + return create_ftrace_stub(entry, addr, me, write);
>
> /* Stub uses address relative to r2. */
> reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> @@ -437,10 +446,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
> }
> pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
>
> - entry->jump[0] |= PPC_HA(reladdr);
> - entry->jump[1] |= PPC_LO(reladdr);
> - entry->funcdata = func_desc(addr);
> - entry->magic = STUB_MAGIC;
> + /*
> + * In case @entry is write-protected, make our changes on the stack
> + * so we can update the whole struct in one write().
> + */
> + memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
> +
> + memcpy(&tmp_entry.jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> + tmp_entry.jump[0] |= PPC_HA(reladdr);
> + tmp_entry.jump[1] |= PPC_LO(reladdr);
> + tmp_entry.funcdata = func_desc(addr);
> + tmp_entry.magic = STUB_MAGIC;
> +
> + write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
>
> return 1;
> }
> @@ -450,7 +468,8 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
> static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> unsigned long addr,
> struct module *me,
> - const char *name)
> + const char *name,
> + void *(*write)(void *, const void *, size_t))
> {
> struct ppc64_stub_entry *stubs;
> unsigned int i, num_stubs;
> @@ -467,7 +486,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> return (unsigned long)&stubs[i];
> }
>
> - if (!create_stub(sechdrs, &stubs[i], addr, me, name))
> + if (!create_stub(sechdrs, &stubs[i], addr, me, name, write))
> return 0;
>
> return (unsigned long)&stubs[i];
> @@ -496,15 +515,20 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
> return 0;
> }
> /* ld r2,R2_STACK_OFFSET(r1) */
> - *instruction = PPC_INST_LD_TOC;
> + if (me->state == MODULE_STATE_UNFORMED)
> + *instruction = PPC_INST_LD_TOC;
> + else
> + patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC));
> +
Would be better if that hunk was following the same approach as other
places.
> return 1;
> }
>
> -int apply_relocate_add(Elf64_Shdr *sechdrs,
> - const char *strtab,
> - unsigned int symindex,
> - unsigned int relsec,
> - struct module *me)
> +static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me,
> + void *(*write)(void *dest, const void *src, size_t len))
> {
> unsigned int i;
> Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> @@ -544,16 +568,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> switch (ELF64_R_TYPE(rela[i].r_info)) {
> case R_PPC64_ADDR32:
> /* Simply set it */
> - *(u32 *)location = value;
> + write(location, &value, 4);
> break;
>
> case R_PPC64_ADDR64:
> /* Simply set it */
> - *(unsigned long *)location = value;
> + write(location, &value, 8);
> break;
>
> case R_PPC64_TOC:
> - *(unsigned long *)location = my_r2(sechdrs, me);
> + value = my_r2(sechdrs, me);
> + write(location, &value, 8);
> break;
>
> case R_PPC64_TOC16:
> @@ -564,17 +589,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> me->name, value);
> return -ENOEXEC;
> }
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xffff)
> + value = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> + write(location, &value, 2);
> break;
>
> case R_PPC64_TOC16_LO:
> /* Subtract TOC pointer */
> value -= my_r2(sechdrs, me);
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xffff)
> + value = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> + write(location, &value, 2);
> break;
>
> case R_PPC64_TOC16_DS:
> @@ -585,9 +610,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> me->name, value);
> return -ENOEXEC;
> }
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xfffc)
> + value = (*((uint16_t *) location) & ~0xfffc)
> | (value & 0xfffc);
> + write(location, &value, 2);
> break;
>
> case R_PPC64_TOC16_LO_DS:
> @@ -598,18 +623,18 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> me->name, value);
> return -ENOEXEC;
> }
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xfffc)
> + value = (*((uint16_t *) location) & ~0xfffc)
> | (value & 0xfffc);
> + write(location, &value, 2);
> break;
>
> case R_PPC64_TOC16_HA:
> /* Subtract TOC pointer */
> value -= my_r2(sechdrs, me);
> value = ((value + 0x8000) >> 16);
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xffff)
> + value = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> + write(location, &value, 2);
> break;
>
> case R_PPC_REL24:
> @@ -618,14 +643,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> sym->st_shndx == SHN_LIVEPATCH) {
> /* External: go via stub */
> value = stub_for_addr(sechdrs, value, me,
> - strtab + sym->st_name);
> + strtab + sym->st_name, write);
> if (!value)
> return -ENOENT;
> if (!restore_r2(strtab + sym->st_name,
> (u32 *)location + 1, me))
> return -ENOEXEC;
> - } else
> + } else {
> value += local_entry_offset(sym);
> + }
>
> /* Convert value to relative */
> value -= (unsigned long)location;
> @@ -636,14 +662,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> }
>
> /* Only replace bits 2 through 26 */
> - *(uint32_t *)location
> - = (*(uint32_t *)location & ~0x03fffffc)
> + value = (*(uint32_t *)location & ~0x03fffffc)
> | (value & 0x03fffffc);
> + write(location, &value, 4);
> break;
>
> case R_PPC64_REL64:
> /* 64 bits relative (used by features fixups) */
> - *location = value - (unsigned long)location;
> + value -= (unsigned long)location;
> + write(location, &value, 8);
> break;
>
> case R_PPC64_REL32:
> @@ -655,7 +682,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> me->name, (long int)value);
> return -ENOEXEC;
> }
> - *(u32 *)location = value;
> + write(location, &value, 4);
> break;
>
> case R_PPC64_TOCSAVE:
> @@ -676,7 +703,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> break;
> /*
> * Check for the large code model prolog sequence:
> - * ld r2, ...(r12)
> + * ld r2, ...(r12)
> * add r2, r2, r12
> */
> if ((((uint32_t *)location)[0] & ~0xfffc) != PPC_RAW_LD(_R2, _R12, 0))
> @@ -688,25 +715,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> * addis r2, r12, (.TOC.-func)@ha
> * addi r2, r2, (.TOC.-func)@l
> */
> - ((uint32_t *)location)[0] = PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value));
> - ((uint32_t *)location)[1] = PPC_RAW_ADDI(_R2, _R2, PPC_LO(value));
> + patch_instruction(&((uint32_t *)location)[0],
> + ppc_inst(PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value))));
> + patch_instruction(&((uint32_t *)location)[1],
> + ppc_inst(PPC_RAW_ADDI(_R2, _R2, PPC_LO(value))));
Shouldn't you do like restore_r2() ?
> break;
>
> case R_PPC64_REL16_HA:
> /* Subtract location pointer */
> value -= (unsigned long)location;
> value = ((value + 0x8000) >> 16);
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xffff)
> + value = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> + write(location, &value, 2);
> break;
>
> case R_PPC64_REL16_LO:
> /* Subtract location pointer */
> value -= (unsigned long)location;
> - *((uint16_t *) location)
> - = (*((uint16_t *) location) & ~0xffff)
> + value = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> + write(location, &value, 2);
> break;
>
> default:
> @@ -720,6 +749,20 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> return 0;
> }
>
> +int apply_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me)
> +{
> + void *(*write)(void *, const void *, size_t) = memcpy;
> + bool early = me->state == MODULE_STATE_UNFORMED;
> +
> + if (!early)
> + write = patch_memory;
> +
> + return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, write);
> +}
I really dislike this stuff with the write() function as a parameter. We
have 'me', it should be enough for the callee to know what to do, see my
first comment at the top of this email.
> #ifdef CONFIG_DYNAMIC_FTRACE
> int module_trampoline_target(struct module *mod, unsigned long addr,
> unsigned long *target)
> @@ -749,7 +792,7 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
> if (copy_from_kernel_nofault(&funcdata, &stub->funcdata,
> sizeof(funcdata))) {
> pr_err("%s: fault reading funcdata for stub %lx for %s\n", __func__, addr, mod->name);
> - return -EFAULT;
> + return -EFAULT;
> }
>
> *target = stub_func_addr(funcdata);
> @@ -759,15 +802,23 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
>
> int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
> {
> + void *(*write)(void *, const void *, size_t) = memcpy;
> + bool early = mod->state == MODULE_STATE_UNFORMED;
> +
> + if (!early)
> + write = patch_memory;
> +
> mod->arch.tramp = stub_for_addr(sechdrs,
> (unsigned long)ftrace_caller,
> mod,
> - "ftrace_caller");
> + "ftrace_caller",
> + write);
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> mod->arch.tramp_regs = stub_for_addr(sechdrs,
> (unsigned long)ftrace_regs_caller,
> mod,
> - "ftrace_regs_caller");
> + "ftrace_regs_caller",
> + write);
> if (!mod->arch.tramp_regs)
> return -ENOENT;
> #endif
>
Christophe
^ permalink raw reply
* Re: Fail to boot 5.15 on mpc8347 with either debug_pagealloc or nobats
From: Maxime Bizon @ 2021-12-12 11:21 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <56402c2e-fe02-e697-7856-80830ea56e66@csgroup.eu>
On Tuesday 07 Dec 2021 à 07:11:40 (+0100), Christophe Leroy wrote:
Hello Christophe,
With all your recent patches, I was able to boot a kernel with every
CONFIG_DEBUG enabled.
After modprobing an empty module (probe just return 0), I get this new
one:
[ 15.351649] BUG: spinlock recursion on CPU#0, kworker/0:2/217
[ 15.357540] lock: init_mm+0x3c/0x420, .magic: dead4ead, .owner: kworker/0:2/217, .owner_cpu: 0
[ 15.366563] CPU: 0 PID: 217 Comm: kworker/0:2 Not tainted 5.15.0+ #523
[ 15.373350] Workqueue: events do_free_init
[ 15.377615] Call Trace:
[ 15.380232] [e4105ac0] [800946a4] do_raw_spin_lock+0xf8/0x120 (unreliable)
[ 15.387340] [e4105ae0] [8001f4ec] change_page_attr+0x40/0x1d4
[ 15.393413] [e4105b10] [801424e0] __apply_to_page_range+0x164/0x310
[ 15.400009] [e4105b60] [80169620] free_pcp_prepare+0x1e4/0x4a0
[ 15.406045] [e4105ba0] [8016c5a0] free_unref_page+0x40/0x2b8
[ 15.411979] [e4105be0] [8018724c] kasan_depopulate_vmalloc_pte+0x6c/0x94
[ 15.418989] [e4105c00] [801424e0] __apply_to_page_range+0x164/0x310
[ 15.425451] [e4105c50] [80187834] kasan_release_vmalloc+0xbc/0x134
[ 15.431898] [e4105c70] [8015f7a8] __purge_vmap_area_lazy+0x4e4/0xdd8
[ 15.438560] [e4105d30] [80160d10] _vm_unmap_aliases.part.0+0x17c/0x24c
[ 15.445283] [e4105d60] [801642d0] __vunmap+0x2f0/0x5c8
[ 15.450684] [e4105db0] [800e32d0] do_free_init+0x68/0x94
[ 15.456181] [e4105dd0] [8005d094] process_one_work+0x4bc/0x7b8
[ 15.462283] [e4105e90] [8005d614] worker_thread+0x284/0x6e8
[ 15.468227] [e4105f00] [8006aaec] kthread+0x1f0/0x210
[ 15.473489] [e4105f40] [80017148] ret_from_kernel_thread+0x14/0x1c
--
Maxime
^ permalink raw reply
* Re: [PATCH v2 0/2] kdump: simplify code
From: Matthew Wilcox @ 2021-12-12 11:47 UTC (permalink / raw)
To: David Laight
Cc: linux-ia64@vger.kernel.org, Baoquan He, linux-sh@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, x86@kernel.org,
kexec@lists.infradead.org, linux-mips@vger.kernel.org,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Xuefeng Li,
linux-fsdevel@vger.kernel.org, Andrew Morton, Dave Young,
'Tiezhu Yang', Vivek Goyal
In-Reply-To: <0c5cb37139af4f3e85cc2c5115d7d006@AcuMS.aculab.com>
On Sat, Dec 11, 2021 at 05:53:46PM +0000, David Laight wrote:
> From: Tiezhu Yang
> > Sent: 11 December 2021 03:33
> >
> > v2:
> > -- add copy_to_user_or_kernel() in lib/usercopy.c
> > -- define userbuf as bool type
>
> Instead of having a flag to indicate whether the buffer is user or kernel,
> would it be better to have two separate buffer pointers.
> One for a user space buffer, the other for a kernel space buffer.
> Exactly one of the buffers should always be NULL.
No. You should be using an iov_iter instead. See
https://lore.kernel.org/all/Ya4bdB0UBJCZhUSo@casper.infradead.org/
for a start on this.
^ permalink raw reply
* Re: [RFC PATCH v2 0/7] Use pageblock_order for cma and alloc_contig_range alignment.
From: Eric Ren @ 2021-12-10 7:30 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand, linux-mm
Cc: Mel Gorman, Robin Murphy, linux-kernel, iommu, virtualization,
linuxppc-dev, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20211209230414.2766515-1-zi.yan@sent.com>
Hi Zi Yan,
On 2021/12/10 07:04, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> Hi all,
>
> This patchset tries to remove the MAX_ORDER - 1 alignment requirement for CMA
> and alloc_contig_range(). It prepares for my upcoming changes to make MAX_ORDER
> adjustable at boot time[1].
>
> The MAX_ORDER - 1 alignment requirement comes from that alloc_contig_range()
> isolates pageblocks to remove free memory from buddy allocator but isolating
> only a subset of pageblocks within a page spanning across multiple pageblocks
> causes free page accounting issues. Isolated page might not be put into the
> right free list, since the code assumes the migratetype of the first pageblock
> as the whole free page migratetype. This is based on the discussion at [2].
>
> To remove the requirement, this patchset:
> 1. still isolates pageblocks at MAX_ORDER - 1 granularity;
Then, unplug fails if either pageblock of the MAX_ORDER - 1 page has
unmovable page, right?
Thanks,
Eric
> 2. but saves the pageblock migratetypes outside the specified range of
> alloc_contig_range() and restores them after all pages within the range
> become free after __alloc_contig_migrate_range();
> 3. splits free pages spanning multiple pageblocks at the beginning and the end
> of the range and puts the split pages to the right migratetype free lists
> based on the pageblock migratetypes;
> 4. returns pages not in the range as it did before this patch.
>
> Isolation needs to happen at MAX_ORDER - 1 granularity, because otherwise
> 1) extra code is needed to detect pages (free, PageHuge, THP, or PageCompound)
> to make sure all pageblocks belonging to a single page are isolated together
> and later pageblocks outside the range need to have their migratetypes restored;
> or 2) extra logic will need to be added during page free time to split a free
> page with multi-migratetype pageblocks.
>
> Two optimizations might come later:
> 1. only check unmovable pages within the range instead of MAX_ORDER - 1 aligned
> range during isolation to increase successful rate of alloc_contig_range().
> 2. make MIGRATE_ISOLATE a separate bit to avoid saving and restoring existing
> migratetypes before and after isolation respectively.
>
> Feel free to give comments and suggestions. Thanks.
>
>
> [1] https://lore.kernel.org/linux-mm/20210805190253.2795604-1-zi.yan@sent.com/
> [2] https://lore.kernel.org/linux-mm/d19fb078-cb9b-f60f-e310-fdeea1b947d2@redhat.com/
>
>
> Zi Yan (7):
> mm: page_alloc: avoid merging non-fallbackable pageblocks with others.
> mm: compaction: handle non-lru compound pages properly in
> isolate_migratepages_block().
> mm: migrate: allocate the right size of non hugetlb or THP compound
> pages.
> mm: make alloc_contig_range work at pageblock granularity
> mm: cma: use pageblock_order as the single alignment
> drivers: virtio_mem: use pageblock size as the minimum virtio_mem
> size.
> arch: powerpc: adjust fadump alignment to be pageblock aligned.
>
> arch/powerpc/include/asm/fadump-internal.h | 4 +-
> drivers/virtio/virtio_mem.c | 6 +-
> include/linux/mmzone.h | 11 +-
> kernel/dma/contiguous.c | 2 +-
> mm/cma.c | 6 +-
> mm/compaction.c | 10 +-
> mm/migrate.c | 8 +-
> mm/page_alloc.c | 203 +++++++++++++++++----
> 8 files changed, 196 insertions(+), 54 deletions(-)
>
^ permalink raw reply
* Re: [RFC PATCH v2 1/7] mm: page_alloc: avoid merging non-fallbackable pageblocks with others.
From: Eric Ren @ 2021-12-10 7:43 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand, linux-mm
Cc: Mel Gorman, Robin Murphy, linux-kernel, iommu, virtualization,
linuxppc-dev, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20211209230414.2766515-2-zi.yan@sent.com>
Hi,
On 2021/12/10 07:04, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> This is done in addition to MIGRATE_ISOLATE pageblock merge avoidance.
> It prepares for the upcoming removal of the MAX_ORDER-1 alignment
> requirement for CMA and alloc_contig_range().
>
> MIGRARTE_HIGHATOMIC should not merge with other migratetypes like
> MIGRATE_ISOLATE and MIGRARTE_CMA[1], so this commit prevents that too.
> Also add MIGRARTE_HIGHATOMIC to fallbacks array for completeness.
>
> [1] https://lore.kernel.org/linux-mm/20211130100853.GP3366@techsingularity.net/
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> include/linux/mmzone.h | 6 ++++++
> mm/page_alloc.c | 28 ++++++++++++++++++----------
> 2 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 58e744b78c2c..b925431b0123 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -83,6 +83,12 @@ static inline bool is_migrate_movable(int mt)
> return is_migrate_cma(mt) || mt == MIGRATE_MOVABLE;
> }
>
> +/* See fallbacks[MIGRATE_TYPES][3] in page_alloc.c */
> +static inline bool migratetype_has_fallback(int mt)
> +{
> + return mt < MIGRATE_PCPTYPES;
> +}
> +
I would suggest spliting the patch into 2 parts. The first part: no
functioning change, just introduce migratetype_has_fallback()
and replace where it applys to.
> #define for_each_migratetype_order(order, type) \
> for (order = 0; order < MAX_ORDER; order++) \
> for (type = 0; type < MIGRATE_TYPES; type++)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index edfd6c81af82..107a5f186d3b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1041,6 +1041,12 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn,
> return page_is_buddy(higher_page, higher_buddy, order + 1);
> }
>
> +static inline bool has_non_fallback_pageblock(struct zone *zone)
> +{
> + return has_isolate_pageblock(zone) || zone_cma_pages(zone) != 0 ||
> + zone->nr_reserved_highatomic != 0;
Make zone->nr_reserved_highatomic != 0 a helper as zone_cma_pages()?
> +}
> +
> /*
> * Freeing function for a buddy system allocator.
> *
> @@ -1116,14 +1122,15 @@ static inline void __free_one_page(struct page *page,
> }
> if (order < MAX_ORDER - 1) {
> /* If we are here, it means order is >= pageblock_order.
> - * We want to prevent merge between freepages on isolate
> - * pageblock and normal pageblock. Without this, pageblock
> - * isolation could cause incorrect freepage or CMA accounting.
> + * We want to prevent merge between freepages on pageblock
> + * without fallbacks and normal pageblock. Without this,
> + * pageblock isolation could cause incorrect freepage or CMA
> + * accounting or HIGHATOMIC accounting.
> *
> * We don't want to hit this code for the more frequent
> * low-order merging.
> */
> - if (unlikely(has_isolate_pageblock(zone))) {
> + if (unlikely(has_non_fallback_pageblock(zone))) {
I'm not familiar with the code details, just wondering if this change
would has side effects on cma
pageblock merging as it the condition stronger?
Thanks,
Eric
> int buddy_mt;
>
> buddy_pfn = __find_buddy_pfn(pfn, order);
> @@ -1131,8 +1138,8 @@ static inline void __free_one_page(struct page *page,
> buddy_mt = get_pageblock_migratetype(buddy);
>
> if (migratetype != buddy_mt
> - && (is_migrate_isolate(migratetype) ||
> - is_migrate_isolate(buddy_mt)))
> + && (!migratetype_has_fallback(migratetype) ||
> + !migratetype_has_fallback(buddy_mt)))
> goto done_merging;
> }
> max_order = order + 1;
> @@ -2483,6 +2490,7 @@ static int fallbacks[MIGRATE_TYPES][3] = {
> [MIGRATE_UNMOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE, MIGRATE_TYPES },
> [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_TYPES },
> [MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE, MIGRATE_MOVABLE, MIGRATE_TYPES },
> + [MIGRATE_HIGHATOMIC] = { MIGRATE_TYPES }, /* Never used */
> #ifdef CONFIG_CMA
> [MIGRATE_CMA] = { MIGRATE_TYPES }, /* Never used */
> #endif
> @@ -2794,8 +2802,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
>
> /* Yoink! */
> mt = get_pageblock_migratetype(page);
> - if (!is_migrate_highatomic(mt) && !is_migrate_isolate(mt)
> - && !is_migrate_cma(mt)) {
> + /* Only reserve normal pageblock */
> + if (migratetype_has_fallback(mt)) {
> zone->nr_reserved_highatomic += pageblock_nr_pages;
> set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
> move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
> @@ -3544,8 +3552,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
> struct page *endpage = page + (1 << order) - 1;
> for (; page < endpage; page += pageblock_nr_pages) {
> int mt = get_pageblock_migratetype(page);
> - if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)
> - && !is_migrate_highatomic(mt))
> + /* Only change normal pageblock */
> + if (migratetype_has_fallback(mt))
> set_pageblock_migratetype(page,
> MIGRATE_MOVABLE);
> }
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox