* [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching
2024-03-15 2:57 [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Benjamin Gray
@ 2024-03-15 2:57 ` Benjamin Gray
2024-03-15 3:17 ` Benjamin Gray
2024-03-15 6:36 ` Christophe Leroy
2024-03-15 2:57 ` [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks Benjamin Gray
2024-03-15 7:14 ` [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Christophe Leroy
2 siblings, 2 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-15 2:57 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Benjamin Gray
The patching page set up as a writable alias may be in quadrant 1
(userspace) if the temporary mm path is used. This causes sanitiser
failures if so. Sanitiser failures also occur on the non-mm path
because the plain memset family is instrumented, and KASAN treats the
patching window as poisoned.
Introduce locally defined patch_* variants of memset that perform an
uninstrumented lower level set, as well as detecting write errors like
the original single patch variant does.
copy_to_user() is not correct here, as the PTE makes it a proper kernel
page (the EEA is privileged access only, RW). It just happens to be in
quadrant 1 because that's the hardware's mechanism for using the current
PID vs PID 0 in translations. Importantly, it's incorrect to allow user
page accesses.
Now that the patching memsets are used, we also propagate a failure up
to the caller as the single patch variant does.
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
The patch_memcpy() can be optimised to 4 bytes at a time assuming the
same requirements as regular instruction patching are being followed
for the 'copy sequence of instructions' mode (i.e., they actually are
instructions following instruction alignment rules).
---
arch/powerpc/lib/code-patching.c | 42 +++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c6ab46156cda..c6633759b509 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
}
NOKPROBE_SYMBOL(patch_instruction);
+static int patch_memset64(u64 *addr, u64 val, size_t count)
+{
+ for (u64 *end = addr + count; addr < end; addr++)
+ __put_kernel_nofault(addr, &val, u64, failed);
+
+ return 0;
+
+failed:
+ return -EPERM;
+}
+
+static int patch_memset32(u32 *addr, u32 val, size_t count)
+{
+ for (u32 *end = addr + count; addr < end; addr++)
+ __put_kernel_nofault(addr, &val, u32, failed);
+
+ return 0;
+
+failed:
+ return -EPERM;
+}
+
+static int patch_memcpy(void *dst, void *src, size_t len)
+{
+ for (void *end = src + len; src < end; dst++, src++)
+ __put_kernel_nofault(dst, src, u8, failed);
+
+ return 0;
+
+failed:
+ return -EPERM;
+}
+
static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
{
unsigned long start = (unsigned long)patch_addr;
+ int err;
/* Repeat instruction */
if (repeat_instr) {
@@ -383,19 +417,19 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
if (ppc_inst_prefixed(instr)) {
u64 val = ppc_inst_as_ulong(instr);
- memset64((u64 *)patch_addr, val, len / 8);
+ err = patch_memset64((u64 *)patch_addr, val, len / 8);
} else {
u32 val = ppc_inst_val(instr);
- memset32(patch_addr, val, len / 4);
+ err = patch_memset32(patch_addr, val, len / 4);
}
} else {
- memcpy(patch_addr, code, len);
+ err = patch_memcpy(patch_addr, code, len);
}
smp_wmb(); /* smp write barrier */
flush_icache_range(start, start + len);
- return 0;
+ return err;
}
/*
--
2.44.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching
2024-03-15 2:57 ` [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching Benjamin Gray
@ 2024-03-15 3:17 ` Benjamin Gray
2024-03-15 6:36 ` Christophe Leroy
1 sibling, 0 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-15 3:17 UTC (permalink / raw)
To: linuxppc-dev, mpe
Also supersedes
https://lore.kernel.org/all/20240213043638.168048-1-bgray@linux.ibm.com/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching
2024-03-15 2:57 ` [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching Benjamin Gray
2024-03-15 3:17 ` Benjamin Gray
@ 2024-03-15 6:36 ` Christophe Leroy
2024-03-17 21:42 ` Benjamin Gray
1 sibling, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2024-03-15 6:36 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> The patching page set up as a writable alias may be in quadrant 1
> (userspace) if the temporary mm path is used. This causes sanitiser
> failures if so. Sanitiser failures also occur on the non-mm path
> because the plain memset family is instrumented, and KASAN treats the
> patching window as poisoned.
>
> Introduce locally defined patch_* variants of memset that perform an
> uninstrumented lower level set, as well as detecting write errors like
> the original single patch variant does.
>
> copy_to_user() is not correct here, as the PTE makes it a proper kernel
> page (the EEA is privileged access only, RW). It just happens to be in
> quadrant 1 because that's the hardware's mechanism for using the current
> PID vs PID 0 in translations. Importantly, it's incorrect to allow user
> page accesses.
>
> Now that the patching memsets are used, we also propagate a failure up
> to the caller as the single patch variant does.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
>
> ---
>
> The patch_memcpy() can be optimised to 4 bytes at a time assuming the
> same requirements as regular instruction patching are being followed
> for the 'copy sequence of instructions' mode (i.e., they actually are
> instructions following instruction alignment rules).
Why not use copy_to_kernel_nofault() ?
> ---
> arch/powerpc/lib/code-patching.c | 42 +++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index c6ab46156cda..c6633759b509 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
> }
> NOKPROBE_SYMBOL(patch_instruction);
>
> +static int patch_memset64(u64 *addr, u64 val, size_t count)
> +{
> + for (u64 *end = addr + count; addr < end; addr++)
> + __put_kernel_nofault(addr, &val, u64, failed);
> +
> + return 0;
> +
> +failed:
> + return -EPERM;
Is it correct ? Shouldn't it be -EFAULT ?
> +}
> +
> +static int patch_memset32(u32 *addr, u32 val, size_t count)
> +{
> + for (u32 *end = addr + count; addr < end; addr++)
> + __put_kernel_nofault(addr, &val, u32, failed);
> +
> + return 0;
> +
> +failed:
> + return -EPERM;
> +}
> +
> +static int patch_memcpy(void *dst, void *src, size_t len)
> +{
> + for (void *end = src + len; src < end; dst++, src++)
> + __put_kernel_nofault(dst, src, u8, failed);
> +
> + return 0;
> +
> +failed:
> + return -EPERM;
> +}
> +
> static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
> {
> unsigned long start = (unsigned long)patch_addr;
> + int err;
>
> /* Repeat instruction */
> if (repeat_instr) {
> @@ -383,19 +417,19 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
> if (ppc_inst_prefixed(instr)) {
> u64 val = ppc_inst_as_ulong(instr);
>
> - memset64((u64 *)patch_addr, val, len / 8);
> + err = patch_memset64((u64 *)patch_addr, val, len / 8);
> } else {
> u32 val = ppc_inst_val(instr);
>
> - memset32(patch_addr, val, len / 4);
> + err = patch_memset32(patch_addr, val, len / 4);
> }
> } else {
> - memcpy(patch_addr, code, len);
> + err = patch_memcpy(patch_addr, code, len);
Use copy_to_kernel_nofault() instead of open coding a new less optimised
version of it.
> }
>
> smp_wmb(); /* smp write barrier */
> flush_icache_range(start, start + len);
> - return 0;
> + return err;
> }
>
> /*
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching
2024-03-15 6:36 ` Christophe Leroy
@ 2024-03-17 21:42 ` Benjamin Gray
0 siblings, 0 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-17 21:42 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
On Fri, 2024-03-15 at 06:36 +0000, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > The patching page set up as a writable alias may be in quadrant 1
> > (userspace) if the temporary mm path is used. This causes sanitiser
> > failures if so. Sanitiser failures also occur on the non-mm path
> > because the plain memset family is instrumented, and KASAN treats
> > the
> > patching window as poisoned.
> >
> > Introduce locally defined patch_* variants of memset that perform
> > an
> > uninstrumented lower level set, as well as detecting write errors
> > like
> > the original single patch variant does.
> >
> > copy_to_user() is not correct here, as the PTE makes it a proper
> > kernel
> > page (the EEA is privileged access only, RW). It just happens to be
> > in
> > quadrant 1 because that's the hardware's mechanism for using the
> > current
> > PID vs PID 0 in translations. Importantly, it's incorrect to allow
> > user
> > page accesses.
> >
> > Now that the patching memsets are used, we also propagate a failure
> > up
> > to the caller as the single patch variant does.
> >
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> >
> > ---
> >
> > The patch_memcpy() can be optimised to 4 bytes at a time assuming
> > the
> > same requirements as regular instruction patching are being
> > followed
> > for the 'copy sequence of instructions' mode (i.e., they actually
> > are
> > instructions following instruction alignment rules).
>
> Why not use copy_to_kernel_nofault() ?
I had not come across copy_to_kernel_nofault(). It looks like the
optimised memcpy() I wanted, so thanks.
>
>
> > ---
> > arch/powerpc/lib/code-patching.c | 42
> > +++++++++++++++++++++++++++++---
> > 1 file changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index c6ab46156cda..c6633759b509 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t
> > instr)
> > }
> > NOKPROBE_SYMBOL(patch_instruction);
> >
> > +static int patch_memset64(u64 *addr, u64 val, size_t count)
> > +{
> > + for (u64 *end = addr + count; addr < end; addr++)
> > + __put_kernel_nofault(addr, &val, u64, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
>
> Is it correct ? Shouldn't it be -EFAULT ?
The single instruction patch returns EPERM, which was set this way to
align with ftrace's expectations. I think it's best to keep the
single/multi patching variants consistent with each other where
possible.
>
> > +}
> > +
> > +static int patch_memset32(u32 *addr, u32 val, size_t count)
> > +{
> > + for (u32 *end = addr + count; addr < end; addr++)
> > + __put_kernel_nofault(addr, &val, u32, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
> > +}
> > +
> > +static int patch_memcpy(void *dst, void *src, size_t len)
> > +{
> > + for (void *end = src + len; src < end; dst++, src++)
> > + __put_kernel_nofault(dst, src, u8, failed);
> > +
> > + return 0;
> > +
> > +failed:
> > + return -EPERM;
> > +}
> > +
> > static int __patch_instructions(u32 *patch_addr, u32 *code,
> > size_t len, bool repeat_instr)
> > {
> > unsigned long start = (unsigned long)patch_addr;
> > + int err;
> >
> > /* Repeat instruction */
> > if (repeat_instr) {
> > @@ -383,19 +417,19 @@ static int __patch_instructions(u32
> > *patch_addr, u32 *code, size_t len, bool rep
> > if (ppc_inst_prefixed(instr)) {
> > u64 val = ppc_inst_as_ulong(instr);
> >
> > - memset64((u64 *)patch_addr, val, len / 8);
> > + err = patch_memset64((u64 *)patch_addr,
> > val, len / 8);
> > } else {
> > u32 val = ppc_inst_val(instr);
> >
> > - memset32(patch_addr, val, len / 4);
> > + err = patch_memset32(patch_addr, val, len
> > / 4);
> > }
> > } else {
> > - memcpy(patch_addr, code, len);
> > + err = patch_memcpy(patch_addr, code, len);
>
> Use copy_to_kernel_nofault() instead of open coding a new less
> optimised
> version of it.
>
> > }
> >
> > smp_wmb(); /* smp write barrier */
> > flush_icache_range(start, start + len);
> > - return 0;
> > + return err;
> > }
> >
> > /*
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks
2024-03-15 2:57 [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Benjamin Gray
2024-03-15 2:57 ` [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching Benjamin Gray
@ 2024-03-15 2:57 ` Benjamin Gray
2024-03-15 6:39 ` Christophe Leroy
2024-03-15 7:14 ` [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Christophe Leroy
2 siblings, 1 reply; 13+ messages in thread
From: Benjamin Gray @ 2024-03-15 2:57 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Benjamin Gray
As we are patching instructions, we can assume the length is a multiple
of 4 and the destination address is aligned.
Atomicity of patching a prefixed instruction is not a concern, as the
original implementation doesn't provide it anyway.
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
arch/powerpc/lib/code-patching.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c6633759b509..ed450a32918c 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val, size_t count)
return -EPERM;
}
-static int patch_memcpy(void *dst, void *src, size_t len)
+static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
{
- for (void *end = src + len; src < end; dst++, src++)
- __put_kernel_nofault(dst, src, u8, failed);
+ for (u32 *end = src + count; src < end; dst++, src++)
+ __put_kernel_nofault(dst, src, u32, failed);
return 0;
@@ -424,7 +424,7 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
err = patch_memset32(patch_addr, val, len / 4);
}
} else {
- err = patch_memcpy(patch_addr, code, len);
+ err = patch_memcpy32(patch_addr, code, len / 4);
}
smp_wmb(); /* smp write barrier */
--
2.44.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks
2024-03-15 2:57 ` [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks Benjamin Gray
@ 2024-03-15 6:39 ` Christophe Leroy
2024-03-17 21:44 ` Benjamin Gray
0 siblings, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2024-03-15 6:39 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> As we are patching instructions, we can assume the length is a multiple
> of 4 and the destination address is aligned.
>
> Atomicity of patching a prefixed instruction is not a concern, as the
> original implementation doesn't provide it anyway.
This patch looks unnecessary.
copy_to_kernel_nofault() is what you want to use instead.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> arch/powerpc/lib/code-patching.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index c6633759b509..ed450a32918c 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val, size_t count)
> return -EPERM;
> }
>
> -static int patch_memcpy(void *dst, void *src, size_t len)
> +static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
> {
> - for (void *end = src + len; src < end; dst++, src++)
> - __put_kernel_nofault(dst, src, u8, failed);
> + for (u32 *end = src + count; src < end; dst++, src++)
> + __put_kernel_nofault(dst, src, u32, failed);
>
> return 0;
>
> @@ -424,7 +424,7 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
> err = patch_memset32(patch_addr, val, len / 4);
> }
> } else {
> - err = patch_memcpy(patch_addr, code, len);
> + err = patch_memcpy32(patch_addr, code, len / 4);
> }
>
> smp_wmb(); /* smp write barrier */
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks
2024-03-15 6:39 ` Christophe Leroy
@ 2024-03-17 21:44 ` Benjamin Gray
0 siblings, 0 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-17 21:44 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
On Fri, 2024-03-15 at 06:39 +0000, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > As we are patching instructions, we can assume the length is a
> > multiple
> > of 4 and the destination address is aligned.
> >
> > Atomicity of patching a prefixed instruction is not a concern, as
> > the
> > original implementation doesn't provide it anyway.
>
> This patch looks unnecessary.
>
> copy_to_kernel_nofault() is what you want to use instead.
Yeah, I would drop this patch when using copy_to_kernel_nofault()
>
> >
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > ---
> > arch/powerpc/lib/code-patching.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index c6633759b509..ed450a32918c 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val,
> > size_t count)
> > return -EPERM;
> > }
> >
> > -static int patch_memcpy(void *dst, void *src, size_t len)
> > +static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
> > {
> > - for (void *end = src + len; src < end; dst++, src++)
> > - __put_kernel_nofault(dst, src, u8, failed);
> > + for (u32 *end = src + count; src < end; dst++, src++)
> > + __put_kernel_nofault(dst, src, u32, failed);
> >
> > return 0;
> >
> > @@ -424,7 +424,7 @@ static int __patch_instructions(u32
> > *patch_addr, u32 *code, size_t len, bool rep
> > err = patch_memset32(patch_addr, val, len
> > / 4);
> > }
> > } else {
> > - err = patch_memcpy(patch_addr, code, len);
> > + err = patch_memcpy32(patch_addr, code, len / 4);
> > }
> >
> > smp_wmb(); /* smp write barrier */
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
2024-03-15 2:57 [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Benjamin Gray
2024-03-15 2:57 ` [PATCH v1 2/3] powerpc/code-patching: Use dedicated memory routines for patching Benjamin Gray
2024-03-15 2:57 ` [PATCH v1 3/3] powerpc/code-patching: Optimise patch_memcpy() to 4 byte chunks Benjamin Gray
@ 2024-03-15 7:14 ` Christophe Leroy
2024-03-17 21:38 ` Benjamin Gray
2 siblings, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2024-03-15 7:14 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> patch_instructions() introduces new behaviour with a couple of
> variations. Test each case of
>
> * a repeated 32-bit instruction,
> * a repeated 64-bit instruction (ppc64), and
> * a copied sequence of instructions
>
> for both on a single page and when it crosses a page boundary.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> arch/powerpc/lib/test-code-patching.c | 92 +++++++++++++++++++++++++++
> 1 file changed, 92 insertions(+)
>
> diff --git a/arch/powerpc/lib/test-code-patching.c b/arch/powerpc/lib/test-code-patching.c
> index c44823292f73..35a3756272df 100644
> --- a/arch/powerpc/lib/test-code-patching.c
> +++ b/arch/powerpc/lib/test-code-patching.c
> @@ -347,6 +347,97 @@ static void __init test_prefixed_patching(void)
> check(!memcmp(iptr, expected, sizeof(expected)));
> }
>
> +static void __init test_multi_instruction_patching(void)
> +{
> + u32 code[256];
Build failure:
CC arch/powerpc/lib/test-code-patching.o
arch/powerpc/lib/test-code-patching.c: In function
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
439 | }
| ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243:
arch/powerpc/lib/test-code-patching.o] Error 1
I have to avoid big arrays on the stack.
> + void *buf;
> + u32 *addr32;
> + u64 *addr64;
> + ppc_inst_t inst64 = ppc_inst_prefix(OP_PREFIX << 26 | 3UL << 24, PPC_RAW_TRAP());
> + u32 inst32 = PPC_RAW_NOP();
> +
> + buf = vzalloc(PAGE_SIZE * 8);
> + check(buf);
> + if (!buf)
> + return;
> +
> + /* Test single page 32-bit repeated instruction */
> + addr32 = buf + PAGE_SIZE;
> + check(!patch_instructions(addr32 + 1, &inst32, 12, true));
> +
> + check(addr32[0] == 0);
> + check(addr32[1] == inst32);
> + check(addr32[2] == inst32);
> + check(addr32[3] == inst32);
> + check(addr32[4] == 0);
> +
> + /* Test single page 64-bit repeated instruction */
> + if (IS_ENABLED(CONFIG_PPC64)) {
> + check(ppc_inst_prefixed(inst64));
> +
> + addr64 = buf + PAGE_SIZE * 2;
> + ppc_inst_write(code, inst64);
> + check(!patch_instructions((u32 *)(addr64 + 1), code, 24, true));
> +
> + check(addr64[0] == 0);
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[1]), inst64));
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[2]), inst64));
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[3]), inst64));
> + check(addr64[4] == 0);
> + }
> +
> + /* Test single page memcpy */
> + addr32 = buf + PAGE_SIZE * 3;
> +
> + for (int i = 0; i < ARRAY_SIZE(code); i++)
> + code[i] = i + 1;
> +
> + check(!patch_instructions(addr32 + 1, code, sizeof(code), false));
> +
> + check(addr32[0] == 0);
> + check(!memcmp(&addr32[1], code, sizeof(code)));
> + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> +
> + /* Test multipage 32-bit repeated instruction */
> + addr32 = buf + PAGE_SIZE * 4 - 8;
> + check(!patch_instructions(addr32 + 1, &inst32, 12, true));
> +
> + check(addr32[0] == 0);
> + check(addr32[1] == inst32);
> + check(addr32[2] == inst32);
> + check(addr32[3] == inst32);
> + check(addr32[4] == 0);
> +
> + /* Test multipage 64-bit repeated instruction */
> + if (IS_ENABLED(CONFIG_PPC64)) {
> + check(ppc_inst_prefixed(inst64));
> +
> + addr64 = buf + PAGE_SIZE * 5 - 8;
> + ppc_inst_write(code, inst64);
> + check(!patch_instructions((u32 *)(addr64 + 1), code, 24, true));
> +
> + check(addr64[0] == 0);
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[1]), inst64));
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[2]), inst64));
> + check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[3]), inst64));
> + check(addr64[4] == 0);
> + }
> +
> + /* Test multipage memcpy */
> + addr32 = buf + PAGE_SIZE * 6 - 12;
> +
> + for (int i = 0; i < ARRAY_SIZE(code); i++)
> + code[i] = i + 1;
> +
> + check(!patch_instructions(addr32 + 1, code, sizeof(code), false));
> +
> + check(addr32[0] == 0);
> + check(!memcmp(&addr32[1], code, sizeof(code)));
> + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> +
> + vfree(buf);
> +}
> +
> static int __init test_code_patching(void)
> {
> pr_info("Running code patching self-tests ...\n");
> @@ -356,6 +447,7 @@ static int __init test_code_patching(void)
> test_create_function_call();
> test_translate_branch();
> test_prefixed_patching();
> + test_multi_instruction_patching();
>
> return 0;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
2024-03-15 7:14 ` [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot Christophe Leroy
@ 2024-03-17 21:38 ` Benjamin Gray
2024-03-17 22:23 ` Benjamin Gray
2024-03-17 22:24 ` Benjamin Gray
0 siblings, 2 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-17 21:38 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:
>
>
> Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > patch_instructions() introduces new behaviour with a couple of
> > variations. Test each case of
> >
> > * a repeated 32-bit instruction,
> > * a repeated 64-bit instruction (ppc64), and
> > * a copied sequence of instructions
> >
> > for both on a single page and when it crosses a page boundary.
> >
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > ---
> > arch/powerpc/lib/test-code-patching.c | 92
> > +++++++++++++++++++++++++++
> > 1 file changed, 92 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/test-code-patching.c
> > b/arch/powerpc/lib/test-code-patching.c
> > index c44823292f73..35a3756272df 100644
> > --- a/arch/powerpc/lib/test-code-patching.c
> > +++ b/arch/powerpc/lib/test-code-patching.c
> > @@ -347,6 +347,97 @@ static void __init
> > test_prefixed_patching(void)
> > check(!memcmp(iptr, expected, sizeof(expected)));
> > }
> >
> > +static void __init test_multi_instruction_patching(void)
> > +{
> > + u32 code[256];
>
> Build failure:
>
> CC arch/powerpc/lib/test-code-patching.o
> arch/powerpc/lib/test-code-patching.c: In function
> 'test_multi_instruction_patching':
> arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 439 | }
> | ^
> cc1: all warnings being treated as errors
> make[4]: *** [scripts/Makefile.build:243:
> arch/powerpc/lib/test-code-patching.o] Error 1
>
>
> I have to avoid big arrays on the stack.
All good, I can do that.
I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?
>
>
> > + void *buf;
> > + u32 *addr32;
> > + u64 *addr64;
> > + ppc_inst_t inst64 = ppc_inst_prefix(OP_PREFIX << 26 | 3UL
> > << 24, PPC_RAW_TRAP());
> > + u32 inst32 = PPC_RAW_NOP();
> > +
> > + buf = vzalloc(PAGE_SIZE * 8);
> > + check(buf);
> > + if (!buf)
> > + return;
> > +
> > + /* Test single page 32-bit repeated instruction */
> > + addr32 = buf + PAGE_SIZE;
> > + check(!patch_instructions(addr32 + 1, &inst32, 12, true));
> > +
> > + check(addr32[0] == 0);
> > + check(addr32[1] == inst32);
> > + check(addr32[2] == inst32);
> > + check(addr32[3] == inst32);
> > + check(addr32[4] == 0);
> > +
> > + /* Test single page 64-bit repeated instruction */
> > + if (IS_ENABLED(CONFIG_PPC64)) {
> > + check(ppc_inst_prefixed(inst64));
> > +
> > + addr64 = buf + PAGE_SIZE * 2;
> > + ppc_inst_write(code, inst64);
> > + check(!patch_instructions((u32 *)(addr64 + 1),
> > code, 24, true));
> > +
> > + check(addr64[0] == 0);
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[1]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[2]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[3]), inst64));
> > + check(addr64[4] == 0);
> > + }
> > +
> > + /* Test single page memcpy */
> > + addr32 = buf + PAGE_SIZE * 3;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(code); i++)
> > + code[i] = i + 1;
> > +
> > + check(!patch_instructions(addr32 + 1, code, sizeof(code),
> > false));
> > +
> > + check(addr32[0] == 0);
> > + check(!memcmp(&addr32[1], code, sizeof(code)));
> > + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> > +
> > + /* Test multipage 32-bit repeated instruction */
> > + addr32 = buf + PAGE_SIZE * 4 - 8;
> > + check(!patch_instructions(addr32 + 1, &inst32, 12, true));
> > +
> > + check(addr32[0] == 0);
> > + check(addr32[1] == inst32);
> > + check(addr32[2] == inst32);
> > + check(addr32[3] == inst32);
> > + check(addr32[4] == 0);
> > +
> > + /* Test multipage 64-bit repeated instruction */
> > + if (IS_ENABLED(CONFIG_PPC64)) {
> > + check(ppc_inst_prefixed(inst64));
> > +
> > + addr64 = buf + PAGE_SIZE * 5 - 8;
> > + ppc_inst_write(code, inst64);
> > + check(!patch_instructions((u32 *)(addr64 + 1),
> > code, 24, true));
> > +
> > + check(addr64[0] == 0);
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[1]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[2]), inst64));
> > + check(ppc_inst_equal(ppc_inst_read((u32
> > *)&addr64[3]), inst64));
> > + check(addr64[4] == 0);
> > + }
> > +
> > + /* Test multipage memcpy */
> > + addr32 = buf + PAGE_SIZE * 6 - 12;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(code); i++)
> > + code[i] = i + 1;
> > +
> > + check(!patch_instructions(addr32 + 1, code, sizeof(code),
> > false));
> > +
> > + check(addr32[0] == 0);
> > + check(!memcmp(&addr32[1], code, sizeof(code)));
> > + check(addr32[ARRAY_SIZE(code) + 1] == 0);
> > +
> > + vfree(buf);
> > +}
> > +
> > static int __init test_code_patching(void)
> > {
> > pr_info("Running code patching self-tests ...\n");
> > @@ -356,6 +447,7 @@ static int __init test_code_patching(void)
> > test_create_function_call();
> > test_translate_branch();
> > test_prefixed_patching();
> > + test_multi_instruction_patching();
> >
> > return 0;
> > }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
2024-03-17 21:38 ` Benjamin Gray
@ 2024-03-17 22:23 ` Benjamin Gray
2024-03-18 2:25 ` Michael Ellerman
2024-03-17 22:24 ` Benjamin Gray
1 sibling, 1 reply; 13+ messages in thread
From: Benjamin Gray @ 2024-03-17 22:23 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
> On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:
> >
> >
> > Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
> > > patch_instructions() introduces new behaviour with a couple of
> > > variations. Test each case of
> > >
> > > * a repeated 32-bit instruction,
> > > * a repeated 64-bit instruction (ppc64), and
> > > * a copied sequence of instructions
> > >
> > > for both on a single page and when it crosses a page boundary.
> > >
> > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > > ---
> > > arch/powerpc/lib/test-code-patching.c | 92
> > > +++++++++++++++++++++++++++
> > > 1 file changed, 92 insertions(+)
> > >
> > > diff --git a/arch/powerpc/lib/test-code-patching.c
> > > b/arch/powerpc/lib/test-code-patching.c
> > > index c44823292f73..35a3756272df 100644
> > > --- a/arch/powerpc/lib/test-code-patching.c
> > > +++ b/arch/powerpc/lib/test-code-patching.c
> > > @@ -347,6 +347,97 @@ static void __init
> > > test_prefixed_patching(void)
> > > check(!memcmp(iptr, expected, sizeof(expected)));
> > > }
> > >
> > > +static void __init test_multi_instruction_patching(void)
> > > +{
> > > + u32 code[256];
> >
> > Build failure:
> >
> > CC arch/powerpc/lib/test-code-patching.o
> > arch/powerpc/lib/test-code-patching.c: In function
> > 'test_multi_instruction_patching':
> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
> > of
> > 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > 439 | }
> > | ^
> > cc1: all warnings being treated as errors
> > make[4]: *** [scripts/Makefile.build:243:
> > arch/powerpc/lib/test-code-patching.o] Error 1
> >
> >
> > I have to avoid big arrays on the stack.
>
> All good, I can do that.
>
> I do run my patches through a couple of 32-bit configs, but I didn't
> see this error. Is this a standard config I should be testing with?
Specifically pmac32_defconfig and ppc44x_defconfig
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
2024-03-17 22:23 ` Benjamin Gray
@ 2024-03-18 2:25 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2024-03-18 2:25 UTC (permalink / raw)
To: Benjamin Gray, Christophe Leroy, linuxppc-dev@lists.ozlabs.org
Benjamin Gray <bgray@linux.ibm.com> writes:
> On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
>> On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:
>> > Le 15/03/2024 à 03:57, Benjamin Gray a écrit :
>> > > diff --git a/arch/powerpc/lib/test-code-patching.c
>> > > b/arch/powerpc/lib/test-code-patching.c
>> > > index c44823292f73..35a3756272df 100644
>> > > --- a/arch/powerpc/lib/test-code-patching.c
>> > > +++ b/arch/powerpc/lib/test-code-patching.c
>> > > @@ -347,6 +347,97 @@ static void __init
>> > > test_prefixed_patching(void)
>> > > check(!memcmp(iptr, expected, sizeof(expected)));
>> > > }
>> > >
>> > > +static void __init test_multi_instruction_patching(void)
>> > > +{
>> > > + u32 code[256];
>> >
>> > Build failure:
>> >
>> > CC arch/powerpc/lib/test-code-patching.o
>> > arch/powerpc/lib/test-code-patching.c: In function
>> > 'test_multi_instruction_patching':
>> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
>> > of
>> > 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>> > 439 | }
>> > | ^
>> > cc1: all warnings being treated as errors
>> > make[4]: *** [scripts/Makefile.build:243:
>> > arch/powerpc/lib/test-code-patching.o] Error 1
>> >
>> >
>> > I have to avoid big arrays on the stack.
>>
>> All good, I can do that.
>>
>> I do run my patches through a couple of 32-bit configs, but I didn't
>> see this error. Is this a standard config I should be testing with?
>
> Specifically pmac32_defconfig and ppc44x_defconfig
Both of those have CONFIG_FRAME_WARN=1024, so should have caught this.
But neither have CONFIG_CODE_PATCHING_SELFTEST=y, so I suspect that's
why you didn't see it.
I recommend ppc32_allmodconfig.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot
2024-03-17 21:38 ` Benjamin Gray
2024-03-17 22:23 ` Benjamin Gray
@ 2024-03-17 22:24 ` Benjamin Gray
1 sibling, 0 replies; 13+ messages in thread
From: Benjamin Gray @ 2024-03-17 22:24 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
mjW5LaBPOdGiiDE1w95Ri9HRK27S2dRZpyib9L4mkfYWPAF41mTudjKmVpgtBLO//rO+zmF04OMB/4sWJhLfvhq1CXULDqw5dcuIAIYwf2ughOtyAPFK1ViDcMO5X1bVpNAFO5m4VBpZvFDQ0j0JfqfVBdL68uH05W1/8dMj76RaWj5m0rLM5slY1FQUPddSU+ic9vaZhlDepjU3ZyI8fmioofNGHaxJq6uNTytKdj87kwDV6PQ4hmuGtY56C7JCgjp053sRJ6sXqgKBWfe4ZOJH17mQm+fws93byLoZvvz4Z3im0Rb0MlFo/WirNyhu+TmTNLpnzFUZfenoKrqAkZLY8u1iCFquhgqA321P+sfYew66DtwQmaoi2GKmF89y2enXXzjLNKfLDKkuVoKxFSPeizYqrLi22R9iO8EGBYKACAWIQQ9K5v9I+L06Hi4yOJ5xrdpFsvehAUCYzuwkQIbAgCBCRB5xrdpFsvehHYgBBkRCAAdFiEESFUlaLYscsf4Dt5gaavCcpI6D/8FAmM7sJEACgkQaavCcpI6D/95UgEAqfSj0QhCrYfazQiLDKJstrz3oIKFjhB6+FYMZqt+K1MA/2ioFtHbypeeWbsqYYRhRyTjAKcvE1NZGtH/YWLgkViUidoBAN6gFX/P+VWB77/w8S/BnPmnJx45wmphlkCL8ckOyopFAQCj9eWamHCl2DSaASMSuoZed6C6Gm0OFtuZh/r8K485BQ==
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Date: Mon, 18 Mar 2024 08:55:02 +1100
MIME-Version: 1.0
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39)
X-Trend-IP-HD: ip=[9.192.253.14]helo={ozlabs.au.ibm.com}sender=(bgray@linux.ibm.com)recipient=<christophe.leroy@csgroup.eu;mpe@ellerman.id.au;linuxppc-dev@lists.ozlabs.org>
On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:
> On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:
> >=20
> >=20
> > Le 15/03/2024 =C3=A0 03:57, Benjamin Gray a =C3=A9crit=C2=A0:
> > > patch_instructions() introduces new behaviour with a couple of
> > > variations. Test each case of
> > >=20
> > > =C2=A0=C2=A0 * a repeated 32-bit instruction,
> > > =C2=A0=C2=A0 * a repeated 64-bit instruction (ppc64), and
> > > =C2=A0=C2=A0 * a copied sequence of instructions
> > >=20
> > > for both on a single page and when it crosses a page boundary.
> > >=20
> > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > > ---
> > > =C2=A0 arch/powerpc/lib/test-code-patching.c | 92
> > > +++++++++++++++++++++++++++
> > > =C2=A0 1 file changed, 92 insertions(+)
> > >=20
> > > diff --git a/arch/powerpc/lib/test-code-patching.c
> > > b/arch/powerpc/lib/test-code-patching.c
> > > index c44823292f73..35a3756272df 100644
> > > --- a/arch/powerpc/lib/test-code-patching.c
> > > +++ b/arch/powerpc/lib/test-code-patching.c
> > > @@ -347,6 +347,97 @@ static void __init
> > > test_prefixed_patching(void)
> > > =C2=A0=C2=A0 check(!memcmp(iptr, expected, sizeof(expected)));
> > > =C2=A0 }
> > > =C2=A0=20
> > > +static void __init test_multi_instruction_patching(void)
> > > +{
> > > + u32 code[256];
> >=20
> > Build failure:
> >=20
> > =C2=A0=C2=A0 CC=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 arch/powerpc/lib/test-cod=
e-patching.o
> > arch/powerpc/lib/test-code-patching.c: In function=20
> > 'test_multi_instruction_patching':
> > arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
> > of
> > 1040 bytes is larger than 1024 bytes [-Werror=3Dframe-larger-than=3D]
> > =C2=A0=C2=A0 439 | }
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | ^
> > cc1: all warnings being treated as errors
> > make[4]: *** [scripts/Makefile.build:243:=20
> > arch/powerpc/lib/test-code-patching.o] Error 1
> >=20
> >=20
> > I have to avoid big arrays on the stack.
>=20
> All good, I can do that.
>=20
> I do run my patches through a couple of 32-bit configs, but I didn't
> see this error. Is this a standard config I should be testing with?
>=20
Specifically I build pmac32_defconfig and ppc44x_defconfig
^ permalink raw reply [flat|nested] 13+ messages in thread