* Re: [PATCH?] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2019-09-17 12:12 [PATCH?] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too Oleg Nesterov
@ 2019-09-17 14:09 ` kbuild test robot
2019-09-17 14:37 ` [PATCH? v2] " Oleg Nesterov
2019-09-19 7:52 ` [PATCH?] " Michael Ellerman
2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2019-09-17 14:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: kbuild-all, Benjamin Herrenschmidt, Madhavan Srinivasan,
Michael Ellerman, Paul Mackerras, Jan Kratochvil, linuxppc-dev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3847 bytes --]
Hi Oleg,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190916]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oleg-Nesterov/powerpc-Hard-wire-PT_SOFTE-value-to-1-in-gpr_get-too/20190917-201613
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/powerpc/kernel/ptrace.c: In function 'gpr_get':
>> arch/powerpc/kernel/ptrace.c:375:58: error: 'msr' undeclared (first use in this function)
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
^~~
arch/powerpc/kernel/ptrace.c:375:58: note: each undeclared identifier is reported only once for each function it appears in
vim +/msr +375 arch/powerpc/kernel/ptrace.c
336
337 static int gpr_get(struct task_struct *target, const struct user_regset *regset,
338 unsigned int pos, unsigned int count,
339 void *kbuf, void __user *ubuf)
340 {
341 int i, ret;
342
343 if (target->thread.regs == NULL)
344 return -EIO;
345
346 if (!FULL_REGS(target->thread.regs)) {
347 /* We have a partial register set. Fill 14-31 with bogus values */
348 for (i = 14; i < 32; i++)
349 target->thread.regs->gpr[i] = NV_REG_POISON;
350 }
351
352 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
353 target->thread.regs,
354 0, offsetof(struct pt_regs, msr));
355 if (!ret) {
356 unsigned long msr = get_user_msr(target);
357 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
358 offsetof(struct pt_regs, msr),
359 offsetof(struct pt_regs, msr) +
360 sizeof(msr));
361 }
362
363 BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
364 offsetof(struct pt_regs, msr) + sizeof(long));
365
366 #ifdef CONFIG_PPC64
367 if (!ret)
368 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
369 &target->thread.regs->orig_gpr3,
370 offsetof(struct pt_regs, orig_gpr3),
371 offsetof(struct pt_regs, softe));
372
373 if (!ret) {
374 unsigned long softe = 0x1;
> 375 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
376 offsetof(struct pt_regs, softe),
377 offsetof(struct pt_regs, softe) +
378 sizeof(softe));
379 }
380
381 BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
382 offsetof(struct pt_regs, softe) + sizeof(long));
383
384 if (!ret)
385 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
386 &target->thread.regs->trap,
387 offsetof(struct pt_regs, trap),
388 sizeof(struct user_pt_regs));
389 #else
390 if (!ret)
391 ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
392 &target->thread.regs->orig_gpr3,
393 offsetof(struct pt_regs, orig_gpr3),
394 sizeof(struct user_pt_regs));
395 #endif
396 if (!ret)
397 ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
398 sizeof(struct user_pt_regs), -1);
399
400 return ret;
401 }
402
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62351 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2019-09-17 12:12 [PATCH?] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too Oleg Nesterov
2019-09-17 14:09 ` kbuild test robot
@ 2019-09-17 14:37 ` Oleg Nesterov
2020-06-10 15:07 ` Oleg Nesterov
2019-09-19 7:52 ` [PATCH?] " Michael Ellerman
2 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2019-09-17 14:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
Paul Mackerras
Cc: Jan Kratochvil, linuxppc-dev, linux-kernel
I don't have a ppc machine, this patch wasn't even compile tested,
could you please review?
The commit a8a4b03ab95f ("powerpc: Hard wire PT_SOFTE value to 1 in
ptrace & signals") changed ptrace_get_reg(PT_SOFTE) to report 0x1,
but PTRACE_GETREGS still copies pt_regs->softe as is.
This is not consistent and this breaks
http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/powerpc/kernel/ptrace.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 8c92feb..291acfb 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -363,11 +363,36 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
offsetof(struct pt_regs, msr) + sizeof(long));
+#ifdef CONFIG_PPC64
+ if (!ret)
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ &target->thread.regs->orig_gpr3,
+ offsetof(struct pt_regs, orig_gpr3),
+ offsetof(struct pt_regs, softe));
+
+ if (!ret) {
+ unsigned long softe = 0x1;
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &softe,
+ offsetof(struct pt_regs, softe),
+ offsetof(struct pt_regs, softe) +
+ sizeof(softe));
+ }
+
+ BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+ offsetof(struct pt_regs, softe) + sizeof(long));
+
+ if (!ret)
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ &target->thread.regs->trap,
+ offsetof(struct pt_regs, trap),
+ sizeof(struct user_pt_regs));
+#else
if (!ret)
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
&target->thread.regs->orig_gpr3,
offsetof(struct pt_regs, orig_gpr3),
sizeof(struct user_pt_regs));
+#endif
if (!ret)
ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
sizeof(struct user_pt_regs), -1);
--
2.5.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2019-09-17 14:37 ` [PATCH? v2] " Oleg Nesterov
@ 2020-06-10 15:07 ` Oleg Nesterov
2020-06-11 8:52 ` Madhavan Srinivasan
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2020-06-10 15:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
Paul Mackerras
Cc: Jan Kratochvil, linuxppc-dev, linux-kernel
Hi,
looks like this patch was forgotten.
Do you think this should be fixed or should we document that
PTRACE_GETREGS is not consistent with PTRACE_PEEKUSER on ppc64?
On 09/17, Oleg Nesterov wrote:
>
> I don't have a ppc machine, this patch wasn't even compile tested,
> could you please review?
>
> The commit a8a4b03ab95f ("powerpc: Hard wire PT_SOFTE value to 1 in
> ptrace & signals") changed ptrace_get_reg(PT_SOFTE) to report 0x1,
> but PTRACE_GETREGS still copies pt_regs->softe as is.
>
> This is not consistent and this breaks
> http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
>
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> arch/powerpc/kernel/ptrace.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 8c92feb..291acfb 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -363,11 +363,36 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
> BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> offsetof(struct pt_regs, msr) + sizeof(long));
>
> +#ifdef CONFIG_PPC64
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->orig_gpr3,
> + offsetof(struct pt_regs, orig_gpr3),
> + offsetof(struct pt_regs, softe));
> +
> + if (!ret) {
> + unsigned long softe = 0x1;
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &softe,
> + offsetof(struct pt_regs, softe),
> + offsetof(struct pt_regs, softe) +
> + sizeof(softe));
> + }
> +
> + BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> + offsetof(struct pt_regs, softe) + sizeof(long));
> +
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->trap,
> + offsetof(struct pt_regs, trap),
> + sizeof(struct user_pt_regs));
> +#else
> if (!ret)
> ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> &target->thread.regs->orig_gpr3,
> offsetof(struct pt_regs, orig_gpr3),
> sizeof(struct user_pt_regs));
> +#endif
> if (!ret)
> ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
> sizeof(struct user_pt_regs), -1);
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2020-06-10 15:07 ` Oleg Nesterov
@ 2020-06-11 8:52 ` Madhavan Srinivasan
2020-06-11 10:58 ` Oleg Nesterov
0 siblings, 1 reply; 8+ messages in thread
From: Madhavan Srinivasan @ 2020-06-11 8:52 UTC (permalink / raw)
To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
Michael Ellerman, Paul Mackerras
Cc: Jan Kratochvil, linuxppc-dev, linux-kernel
On 6/10/20 8:37 PM, Oleg Nesterov wrote:
> Hi,
>
> looks like this patch was forgotten.
yep, I missed this. But mpe did have comments for the patch.
https://lkml.org/lkml/2019/9/19/107
Maddy
>
> Do you think this should be fixed or should we document that
> PTRACE_GETREGS is not consistent with PTRACE_PEEKUSER on ppc64?
>
>
> On 09/17, Oleg Nesterov wrote:
>> I don't have a ppc machine, this patch wasn't even compile tested,
>> could you please review?
>>
>> The commit a8a4b03ab95f ("powerpc: Hard wire PT_SOFTE value to 1 in
>> ptrace & signals") changed ptrace_get_reg(PT_SOFTE) to report 0x1,
>> but PTRACE_GETREGS still copies pt_regs->softe as is.
>>
>> This is not consistent and this breaks
>> http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
>>
>> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>> ---
>> arch/powerpc/kernel/ptrace.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index 8c92feb..291acfb 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
>> @@ -363,11 +363,36 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>> BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
>> offsetof(struct pt_regs, msr) + sizeof(long));
>>
>> +#ifdef CONFIG_PPC64
>> + if (!ret)
>> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> + &target->thread.regs->orig_gpr3,
>> + offsetof(struct pt_regs, orig_gpr3),
>> + offsetof(struct pt_regs, softe));
>> +
>> + if (!ret) {
>> + unsigned long softe = 0x1;
>> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &softe,
>> + offsetof(struct pt_regs, softe),
>> + offsetof(struct pt_regs, softe) +
>> + sizeof(softe));
>> + }
>> +
>> + BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
>> + offsetof(struct pt_regs, softe) + sizeof(long));
>> +
>> + if (!ret)
>> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> + &target->thread.regs->trap,
>> + offsetof(struct pt_regs, trap),
>> + sizeof(struct user_pt_regs));
>> +#else
>> if (!ret)
>> ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> &target->thread.regs->orig_gpr3,
>> offsetof(struct pt_regs, orig_gpr3),
>> sizeof(struct user_pt_regs));
>> +#endif
>> if (!ret)
>> ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
>> sizeof(struct user_pt_regs), -1);
>> --
>> 2.5.0
>>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2020-06-11 8:52 ` Madhavan Srinivasan
@ 2020-06-11 10:58 ` Oleg Nesterov
2020-06-11 11:11 ` Jan Kratochvil
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2020-06-11 10:58 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
Paul Mackerras, Jan Kratochvil, linuxppc-dev, linux-kernel
On 06/11, Madhavan Srinivasan wrote:
>
>
> On 6/10/20 8:37 PM, Oleg Nesterov wrote:
> >Hi,
> >
> >looks like this patch was forgotten.
>
> yep, I missed this. But mpe did have comments for the patch.
>
> https://lkml.org/lkml/2019/9/19/107
Yes, and I thought that I have replied... apparently not, sorry!
So let me repeat, I am fine either way, I do not understand this
ppc-specific code and I can't really test this change.
Let me quote that email from Michael:
> We could do it like below. I'm 50/50 though on whether it's worth it, or
> if we should just go with the big ifdef like in your patch.
up to you ;)
Hmm. And yes,
> >>This is not consistent and this breaks
> >>http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
this is 404.
Jan, could correct the link above?
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2020-06-11 10:58 ` Oleg Nesterov
@ 2020-06-11 11:11 ` Jan Kratochvil
0 siblings, 0 replies; 8+ messages in thread
From: Jan Kratochvil @ 2020-06-11 11:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Madhavan Srinivasan, Benjamin Herrenschmidt, Madhavan Srinivasan,
Michael Ellerman, Paul Mackerras, linuxppc-dev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 371 bytes --]
On Thu, 11 Jun 2020 12:58:31 +0200, Oleg Nesterov wrote:
> On 06/11, Madhavan Srinivasan wrote:
> > On 6/10/20 8:37 PM, Oleg Nesterov wrote:
> > > > This is not consistent and this breaks
> > > > http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
>
> this is 404.
Attaching the testcase, the CVS web interface no longer works on
sourceware.org.
Jan
[-- Attachment #2: user-regs-peekpoke.c --]
[-- Type: text/plain, Size: 10499 bytes --]
/* Test case for PTRACE_SETREGS modifying the requested ragisters.
x86* counterpart of the s390* testcase `user-area-access.c'.
This software is provided 'as-is', without any express or implied
warranty. In no event will the authors be held liable for any damages
arising from the use of this software.
Permission is granted to anyone to use this software for any purpose,
including commercial applications, and to alter it and redistribute it
freely. */
/* FIXME: EFLAGS should be tested restricted on the appropriate bits. */
#define _GNU_SOURCE 1
#if defined __powerpc__ || defined __sparc__
# define user_regs_struct pt_regs
#endif
#ifdef __ia64__
#define ia64_fpreg ia64_fpreg_DISABLE
#define pt_all_user_regs pt_all_user_regs_DISABLE
#endif /* __ia64__ */
#include <sys/ptrace.h>
#ifdef __ia64__
#undef ia64_fpreg
#undef pt_all_user_regs
#endif /* __ia64__ */
#include <linux/ptrace.h>
#include <sys/types.h>
#include <sys/user.h>
#if defined __i386__ || defined __x86_64__
#include <sys/debugreg.h>
#endif
#include <asm/unistd.h>
#include <assert.h>
#include <errno.h>
#include <error.h>
#include <unistd.h>
#include <signal.h>
#include <stdlib.h>
#include <stdio.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <string.h>
#include <stddef.h>
/* ia64 has PTRACE_SETREGS but it has no USER_REGS_STRUCT. */
#if !defined PTRACE_SETREGS || defined __ia64__
int
main (void)
{
return 77;
}
#else /* PTRACE_SETREGS */
/* The minimal alignment we use for the random access ranges. */
#define REGALIGN (sizeof (long))
static pid_t child;
static void
cleanup (void)
{
if (child > 0)
kill (child, SIGKILL);
child = 0;
}
static void
handler_fail (int signo)
{
cleanup ();
signal (SIGABRT, SIG_DFL);
abort ();
}
int
main (void)
{
long l;
int status, i;
pid_t pid;
union
{
struct user_regs_struct user;
unsigned char byte[sizeof (struct user_regs_struct)];
} u, u2;
int start;
setbuf (stdout, NULL);
atexit (cleanup);
signal (SIGABRT, handler_fail);
signal (SIGALRM, handler_fail);
signal (SIGINT, handler_fail);
i = alarm (10);
assert (i == 0);
child = fork ();
switch (child)
{
case -1:
assert_perror (errno);
assert (0);
case 0:
l = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
assert (l == 0);
// Prevent rt_sigprocmask() call called by glibc after raise().
syscall (__NR_tkill, getpid (), SIGSTOP);
assert (0);
default:
break;
}
pid = waitpid (child, &status, 0);
assert (pid == child);
assert (WIFSTOPPED (status));
assert (WSTOPSIG (status) == SIGSTOP);
/* Fetch U2 from the inferior. */
errno = 0;
# ifdef __sparc__
l = ptrace (PTRACE_GETREGS, child, &u2.user, NULL);
# else
l = ptrace (PTRACE_GETREGS, child, NULL, &u2.user);
# endif
assert_perror (errno);
assert (l == 0);
/* Initialize U with a pattern. */
for (i = 0; i < sizeof u.byte; i++)
u.byte[i] = i;
#ifdef __x86_64__
/* non-EFLAGS modifications fail with EIO, EFLAGS gets back different. */
u.user.eflags = u2.user.eflags;
u.user.cs = u2.user.cs;
u.user.ds = u2.user.ds;
u.user.es = u2.user.es;
u.user.fs = u2.user.fs;
u.user.gs = u2.user.gs;
u.user.ss = u2.user.ss;
u.user.fs_base = u2.user.fs_base;
u.user.gs_base = u2.user.gs_base;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.rip = (unsigned long) handler_fail;
/* 2.6.25 always truncates and sign-extends orig_rax. */
u.user.orig_rax = (int) u.user.orig_rax;
#endif /* __x86_64__ */
#ifdef __i386__
/* These values get back different. */
u.user.xds = u2.user.xds;
u.user.xes = u2.user.xes;
u.user.xfs = u2.user.xfs;
u.user.xgs = u2.user.xgs;
u.user.xcs = u2.user.xcs;
u.user.eflags = u2.user.eflags;
u.user.xss = u2.user.xss;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.eip = (unsigned long) handler_fail;
#endif /* __i386__ */
#ifdef __powerpc__
/* These fields are constrained. */
u.user.msr = u2.user.msr;
# ifdef __powerpc64__
u.user.softe = u2.user.softe;
# else
u.user.mq = u2.user.mq;
# endif /* __powerpc64__ */
u.user.trap = u2.user.trap;
u.user.dar = u2.user.dar;
u.user.dsisr = u2.user.dsisr;
u.user.result = u2.user.result;
#endif /* __powerpc__ */
/* Poke U. */
# ifdef __sparc__
l = ptrace (PTRACE_SETREGS, child, &u.user, NULL);
# else
l = ptrace (PTRACE_SETREGS, child, NULL, &u.user);
# endif
assert (l == 0);
/* Peek into U2. */
# ifdef __sparc__
l = ptrace (PTRACE_GETREGS, child, &u2.user, NULL);
# else
l = ptrace (PTRACE_GETREGS, child, NULL, &u2.user);
# endif
assert (l == 0);
/* Verify it matches. */
if (memcmp (&u.user, &u2.user, sizeof u.byte) != 0)
{
for (start = 0; start + REGALIGN <= sizeof u.byte; start += REGALIGN)
if (*(unsigned long *) (u.byte + start)
!= *(unsigned long *) (u2.byte + start))
printf ("\
mismatch at offset %#x: SETREGS wrote %lx GETREGS read %lx\n",
start, *(unsigned long *) (u.byte + start),
*(unsigned long *) (u2.byte + start));
return 1;
}
/* Reverse the pattern. */
for (i = 0; i < sizeof u.byte; i++)
u.byte[i] ^= -1;
#ifdef __x86_64__
/* non-EFLAGS modifications fail with EIO, EFLAGS gets back different. */
u.user.eflags = u2.user.eflags;
u.user.cs = u2.user.cs;
u.user.ds = u2.user.ds;
u.user.es = u2.user.es;
u.user.fs = u2.user.fs;
u.user.gs = u2.user.gs;
u.user.ss = u2.user.ss;
u.user.fs_base = u2.user.fs_base;
u.user.gs_base = u2.user.gs_base;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.rip = (unsigned long) handler_fail;
/* 2.6.25 always truncates and sign-extends orig_rax. */
u.user.orig_rax = (int) u.user.orig_rax;
#endif /* __x86_64__ */
#ifdef __i386__
/* These values get back different. */
u.user.xds = u2.user.xds;
u.user.xes = u2.user.xes;
u.user.xfs = u2.user.xfs;
u.user.xgs = u2.user.xgs;
u.user.xcs = u2.user.xcs;
u.user.eflags = u2.user.eflags;
u.user.xss = u2.user.xss;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.eip = (unsigned long) handler_fail;
#endif /* __i386__ */
#ifdef __powerpc__
/* These fields are constrained. */
u.user.msr = u2.user.msr;
# ifdef __powerpc64__
u.user.softe = u2.user.softe;
# else
u.user.mq = u2.user.mq;
# endif /* __powerpc64__ */
u.user.trap = u2.user.trap;
u.user.dar = u2.user.dar;
u.user.dsisr = u2.user.dsisr;
u.user.result = u2.user.result;
#endif /* __powerpc__ */
/* Poke U. */
# ifdef __sparc__
l = ptrace (PTRACE_SETREGS, child, &u.user, NULL);
# else
l = ptrace (PTRACE_SETREGS, child, NULL, &u.user);
# endif
assert (l == 0);
/* Peek into U2. */
# ifdef __sparc__
l = ptrace (PTRACE_GETREGS, child, &u2.user, NULL);
# else
l = ptrace (PTRACE_GETREGS, child, NULL, &u2.user);
# endif
assert (l == 0);
/* Verify it matches. */
if (memcmp (&u.user, &u2.user, sizeof u.byte) != 0)
{
for (start = 0; start + REGALIGN <= sizeof u.byte; start += REGALIGN)
if (*(unsigned long *) (u.byte + start)
!= *(unsigned long *) (u2.byte + start))
printf ("\
mismatch at offset %#x: SETREGS wrote %lx GETREGS read %lx\n",
start, *(unsigned long *) (u.byte + start),
*(unsigned long *) (u2.byte + start));
return 1;
}
/* Now try poking arbitrary ranges and verifying it reads back right.
We expect the U area is already a random enough pattern. */
for (start = 0; start + REGALIGN <= sizeof u.byte; start += REGALIGN)
{
for (i = start; i < start + REGALIGN; i++)
u.byte[i]++;
#ifdef __x86_64__
/* non-EFLAGS modifications fail with EIO, EFLAGS gets back different. */
u.user.eflags = u2.user.eflags;
u.user.cs = u2.user.cs;
u.user.ds = u2.user.ds;
u.user.es = u2.user.es;
u.user.fs = u2.user.fs;
u.user.gs = u2.user.gs;
u.user.ss = u2.user.ss;
u.user.fs_base = u2.user.fs_base;
u.user.gs_base = u2.user.gs_base;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.rip = (unsigned long) handler_fail;
/* 2.6.25 always truncates and sign-extends orig_rax. */
u.user.orig_rax = (int) u.user.orig_rax;
#endif /* __x86_64__ */
#ifdef __i386__
/* These values get back different. */
u.user.xds = u2.user.xds;
u.user.xes = u2.user.xes;
u.user.xfs = u2.user.xfs;
u.user.xgs = u2.user.xgs;
u.user.xcs = u2.user.xcs;
u.user.eflags = u2.user.eflags;
u.user.xss = u2.user.xss;
/* RHEL-4 refuses to set too high (and invalid) PC values. */
u.user.eip = (unsigned long) handler_fail;
#endif /* __i386__ */
#ifdef __powerpc__
/* These fields are constrained. */
u.user.msr = u2.user.msr;
# ifdef __powerpc64__
u.user.softe = u2.user.softe;
# else
u.user.mq = u2.user.mq;
# endif /* __powerpc64__ */
u.user.trap = u2.user.trap;
u.user.dar = u2.user.dar;
u.user.dsisr = u2.user.dsisr;
u.user.result = u2.user.result;
if (start > offsetof (struct pt_regs, ccr))
break;
#endif /* __powerpc__ */
/* Poke U. */
l = ptrace (PTRACE_POKEUSER, child, (void *) (unsigned long) start,
(void *) *(unsigned long *) (u.byte + start));
if (l != 0)
error (1, errno, "PTRACE_POKEUSER at %x", start);
/* Peek into U2. */
# ifdef __sparc__
l = ptrace (PTRACE_GETREGS, child, &u2.user, NULL);
# else
l = ptrace (PTRACE_GETREGS, child, NULL, &u2.user);
# endif
assert (l == 0);
/* Verify it matches. */
if (memcmp (&u.user, &u2.user, sizeof u.byte) != 0)
{
printf ("mismatch at offset %#x: poked %lx but GETREGS read %lx\n",
start, *(unsigned long *) (u.byte + start),
*(unsigned long *) (u2.byte + start));
return 1;
}
}
/* Now try peeking arbitrary ranges and verifying it is the same.
We expect the U area is already a random enough pattern. */
for (start = 0; start + REGALIGN <= sizeof u.byte; start += REGALIGN)
{
/* Peek for the U comparation. */
errno = 0;
l = ptrace (PTRACE_PEEKUSER, child, (void *) (unsigned long) start,
NULL);
assert_perror (errno);
/* Verify it matches. */
if (*(unsigned long *) (u.byte + start) != l)
{
printf ("mismatch at offset %#x: poked %lx but peeked %lx\n",
start, *(unsigned long *) (u.byte + start), l);
return 1;
}
}
return 0;
}
#endif /* PTRACE_SETREGS */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH?] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
2019-09-17 12:12 [PATCH?] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too Oleg Nesterov
2019-09-17 14:09 ` kbuild test robot
2019-09-17 14:37 ` [PATCH? v2] " Oleg Nesterov
@ 2019-09-19 7:52 ` Michael Ellerman
2 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2019-09-19 7:52 UTC (permalink / raw)
To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
Paul Mackerras
Cc: Jan Kratochvil, linuxppc-dev, linux-kernel
Hi Oleg,
Thanks for the patch.
Oleg Nesterov <oleg@redhat.com> writes:
> I don't have a ppc machine, this patch wasn't even compile tested,
> could you please review?
>
> The commit a8a4b03ab95f ("powerpc: Hard wire PT_SOFTE value to 1 in
> ptrace & signals") changed ptrace_get_reg(PT_SOFTE) to report 0x1,
> but PTRACE_GETREGS still copies pt_regs->softe as is.
Ugh, that certainly seems broken. I guess we forgot/didn't-know that
there were two paths through ptrace to get the one register.
> This is not consistent and this breaks
> http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
That's a 404 for me?
Is it this: https://sourceware.org/systemtap/wiki/utrace/tests/
That seems to point me to a CVS repo? Which then didn't build. But now I
have that one test built, and you're right it fails with:
$ ./user-regs-peekpoke
mismatch at offset 0x138: poked 0 but peeked 1
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> arch/powerpc/kernel/ptrace.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 8c92feb..9e9342c 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -363,11 +363,36 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
> BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> offsetof(struct pt_regs, msr) + sizeof(long));
>
> +#ifdef CONFIG_PPC64
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->orig_gpr3,
> + offsetof(struct pt_regs, orig_gpr3),
> + offsetof(struct pt_regs, softe));
> +
> + if (!ret) {
> + unsigned long softe = 0x1;
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
> + offsetof(struct pt_regs, softe),
> + offsetof(struct pt_regs, softe) +
> + sizeof(softe));
> + }
> +
> + BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> + offsetof(struct pt_regs, softe) + sizeof(long));
> +
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->trap,
> + offsetof(struct pt_regs, trap),
> + sizeof(struct user_pt_regs));
> +#else
> if (!ret)
> ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> &target->thread.regs->orig_gpr3,
> offsetof(struct pt_regs, orig_gpr3),
> sizeof(struct user_pt_regs));
> +#endif
> if (!ret)
> ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
> sizeof(struct user_pt_regs), -1);
It would be nice if we could isolate the special logic in once place,
ie. ptrace_get_reg().
We could do it like below. I'm 50/50 though on whether it's worth it, or
if we should just go with the big ifdef like in your patch.
cheers
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 8c92febf5f44..55510f1a7ec1 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -334,6 +334,11 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
return -EIO;
}
+#ifndef __powerpc64__
+/* Needed on 32-bit to make the SOFTE logic below work without ifdefs */
+#define PT_SOFTE PT_MQ
+#endif
+
static int gpr_get(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
@@ -367,6 +372,24 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
&target->thread.regs->orig_gpr3,
offsetof(struct pt_regs, orig_gpr3),
+ PT_SOFTE * sizeof(long));
+
+ /* SOFTE is special on 64-bit, the logic is in ptrace_get_reg() */
+ if (!ret) {
+ unsigned long val = 0;
+ ptrace_get_reg(target, PT_SOFTE, &val);
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &val,
+ PT_SOFTE * sizeof(long),
+ offsetof(struct pt_regs, trap));
+ }
+
+ BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+ (PT_SOFTE * sizeof(long)) + sizeof(long));
+
+ if (!ret)
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+ &target->thread.regs->trap,
+ offsetof(struct pt_regs, trap),
sizeof(struct user_pt_regs));
if (!ret)
ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
@@ -3384,9 +3407,13 @@ void __init pt_regs_check(void)
#ifdef __powerpc64__
BUILD_BUG_ON(offsetof(struct pt_regs, softe) !=
offsetof(struct user_pt_regs, softe));
+ BUILD_BUG_ON(offsetof(struct pt_regs, softe) !=
+ PT_SOFTE * sizeof(long));
#else
BUILD_BUG_ON(offsetof(struct pt_regs, mq) !=
offsetof(struct user_pt_regs, mq));
+ BUILD_BUG_ON(offsetof(struct pt_regs, mq) !=
+ PT_MQ * sizeof(long));
#endif
BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
offsetof(struct user_pt_regs, trap));
^ permalink raw reply related [flat|nested] 8+ messages in thread