All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 16:01 ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:01 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel

Can we finally fix this problem? ;)

My previous attempt was ignored, see

	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/

Now that gpr_get() was changed to use membuf API we can make a simpler fix.

Sorry, uncompiled/untested, I don't have a ppc machine.

Oleg.

 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 21 ++++++++++++---------
 arch/powerpc/kernel/ptrace/ptrace-view.c | 21 ++++++++++++---------
 include/linux/regset.h                   | 12 ++++++++++++
 3 files changed, 36 insertions(+), 18 deletions(-)


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 16:01 ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:01 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev

Can we finally fix this problem? ;)

My previous attempt was ignored, see

	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/

Now that gpr_get() was changed to use membuf API we can make a simpler fix.

Sorry, uncompiled/untested, I don't have a ppc machine.

Oleg.

 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 21 ++++++++++++---------
 arch/powerpc/kernel/ptrace/ptrace-view.c | 21 ++++++++++++---------
 include/linux/regset.h                   | 12 ++++++++++++
 3 files changed, 36 insertions(+), 18 deletions(-)


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
  2020-11-19 16:01 ` Oleg Nesterov
@ 2020-11-19 16:02   ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:02 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel

gpr_get() does membuf_write() twice to override pt_regs->msr in between.
We can call membuf_write() once and change ->msr in the kernel buffer,
this simplifies the code and the next fix.

The patch adds a new simple helper, membuf_at(offs), it returns the new
membuf which can be safely used after membuf_write().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 13 +++++--------
 arch/powerpc/kernel/ptrace/ptrace-view.c | 13 +++++--------
 include/linux/regset.h                   | 12 ++++++++++++
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index 54f2d076206f..f8fcbd85d4cb 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -86,6 +86,8 @@ int tm_cgpr_active(struct task_struct *target, const struct user_regset *regset)
 int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 		struct membuf to)
 {
+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return -ENODEV;
 
@@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 	flush_altivec_to_thread(target);
 
 	membuf_write(&to, &target->thread.ckpt_regs,
-			offsetof(struct pt_regs, msr));
-	membuf_store(&to, get_user_ckpt_msr(target));
+				sizeof(struct user_pt_regs));
 
-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
-		     offsetof(struct pt_regs, msr) + sizeof(long));
+	membuf_store(&to_msr, get_user_ckpt_msr(target));
 
-	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
-			sizeof(struct user_pt_regs));
+				sizeof(struct user_pt_regs));
 }
 
 /*
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 7e6478e7ed07..39686ede40b3 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -217,6 +217,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
 static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		   struct membuf to)
 {
+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
 	int i;
 
 	if (target->thread.regs == NULL)
@@ -228,17 +229,13 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 			target->thread.regs->gpr[i] = NV_REG_POISON;
 	}
 
-	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
-	membuf_store(&to, get_user_msr(target));
+	membuf_write(&to, target->thread.regs,
+				sizeof(struct user_pt_regs));
 
-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
-		     offsetof(struct pt_regs, msr) + sizeof(long));
+	membuf_store(&to_msr, get_user_msr(target));
 
-	membuf_write(&to, &target->thread.regs->orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
-				 sizeof(struct user_pt_regs));
+				sizeof(struct user_pt_regs));
 }
 
 static int gpr_set(struct task_struct *target, const struct user_regset *regset,
diff --git a/include/linux/regset.h b/include/linux/regset.h
index c3403f328257..a00765f0e8cf 100644
--- a/include/linux/regset.h
+++ b/include/linux/regset.h
@@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
 	return s->left;
 }
 
+static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
+{
+	struct membuf n = *s;
+
+	if (offs > n.left)
+		offs = n.left;
+	n.p += offs;
+	n.left -= offs;
+
+	return n;
+}
+
 /* current s->p must be aligned for v; v must be a scalar */
 #define membuf_store(s, v)				\
 ({							\
-- 
2.25.1.362.g51ebf55



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
@ 2020-11-19 16:02   ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:02 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev

gpr_get() does membuf_write() twice to override pt_regs->msr in between.
We can call membuf_write() once and change ->msr in the kernel buffer,
this simplifies the code and the next fix.

The patch adds a new simple helper, membuf_at(offs), it returns the new
membuf which can be safely used after membuf_write().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 13 +++++--------
 arch/powerpc/kernel/ptrace/ptrace-view.c | 13 +++++--------
 include/linux/regset.h                   | 12 ++++++++++++
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index 54f2d076206f..f8fcbd85d4cb 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -86,6 +86,8 @@ int tm_cgpr_active(struct task_struct *target, const struct user_regset *regset)
 int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 		struct membuf to)
 {
+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return -ENODEV;
 
@@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 	flush_altivec_to_thread(target);
 
 	membuf_write(&to, &target->thread.ckpt_regs,
-			offsetof(struct pt_regs, msr));
-	membuf_store(&to, get_user_ckpt_msr(target));
+				sizeof(struct user_pt_regs));
 
-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
-		     offsetof(struct pt_regs, msr) + sizeof(long));
+	membuf_store(&to_msr, get_user_ckpt_msr(target));
 
-	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
-			sizeof(struct user_pt_regs));
+				sizeof(struct user_pt_regs));
 }
 
 /*
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 7e6478e7ed07..39686ede40b3 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -217,6 +217,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
 static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		   struct membuf to)
 {
+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
 	int i;
 
 	if (target->thread.regs == NULL)
@@ -228,17 +229,13 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 			target->thread.regs->gpr[i] = NV_REG_POISON;
 	}
 
-	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
-	membuf_store(&to, get_user_msr(target));
+	membuf_write(&to, target->thread.regs,
+				sizeof(struct user_pt_regs));
 
-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
-		     offsetof(struct pt_regs, msr) + sizeof(long));
+	membuf_store(&to_msr, get_user_msr(target));
 
-	membuf_write(&to, &target->thread.regs->orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
-				 sizeof(struct user_pt_regs));
+				sizeof(struct user_pt_regs));
 }
 
 static int gpr_set(struct task_struct *target, const struct user_regset *regset,
diff --git a/include/linux/regset.h b/include/linux/regset.h
index c3403f328257..a00765f0e8cf 100644
--- a/include/linux/regset.h
+++ b/include/linux/regset.h
@@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
 	return s->left;
 }
 
+static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
+{
+	struct membuf n = *s;
+
+	if (offs > n.left)
+		offs = n.left;
+	n.p += offs;
+	n.left -= offs;
+
+	return n;
+}
+
 /* current s->p must be aligned for v; v must be a scalar */
 #define membuf_store(s, v)				\
 ({							\
-- 
2.25.1.362.g51ebf55



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:01 ` Oleg Nesterov
@ 2020-11-19 16:02   ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:02 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel

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 the user-regs-peekpoke test
from https://sourceware.org/systemtap/wiki/utrace/tests/

Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
 arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index f8fcbd85d4cb..d0d339f86e61 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -87,6 +87,10 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 		struct membuf to)
 {
 	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+#ifdef CONFIG_PPC64
+	struct membuf to_softe = membuf_at(&to,
+					offsetof(struct pt_regs, softe));
+#endif
 
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return -ENODEV;
@@ -102,7 +106,9 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 				sizeof(struct user_pt_regs));
 
 	membuf_store(&to_msr, get_user_ckpt_msr(target));
-
+#ifdef CONFIG_PPC64
+	membuf_store(&to_softe, 0x1ul);
+#endif
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
 				sizeof(struct user_pt_regs));
 }
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 39686ede40b3..f554ccfcbfae 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -218,6 +218,10 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		   struct membuf to)
 {
 	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+#ifdef CONFIG_PPC64
+	struct membuf to_softe = membuf_at(&to,
+					offsetof(struct pt_regs, softe));
+#endif
 	int i;
 
 	if (target->thread.regs == NULL)
@@ -233,7 +237,9 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 				sizeof(struct user_pt_regs));
 
 	membuf_store(&to_msr, get_user_msr(target));
-
+#ifdef CONFIG_PPC64
+	membuf_store(&to_softe, 0x1ul);
+#endif
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
 				sizeof(struct user_pt_regs));
 }
-- 
2.25.1.362.g51ebf55



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 16:02   ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:02 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev

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 the user-regs-peekpoke test
from https://sourceware.org/systemtap/wiki/utrace/tests/

Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
 arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index f8fcbd85d4cb..d0d339f86e61 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -87,6 +87,10 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 		struct membuf to)
 {
 	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+#ifdef CONFIG_PPC64
+	struct membuf to_softe = membuf_at(&to,
+					offsetof(struct pt_regs, softe));
+#endif
 
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return -ENODEV;
@@ -102,7 +106,9 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
 				sizeof(struct user_pt_regs));
 
 	membuf_store(&to_msr, get_user_ckpt_msr(target));
-
+#ifdef CONFIG_PPC64
+	membuf_store(&to_softe, 0x1ul);
+#endif
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
 				sizeof(struct user_pt_regs));
 }
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 39686ede40b3..f554ccfcbfae 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -218,6 +218,10 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		   struct membuf to)
 {
 	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
+#ifdef CONFIG_PPC64
+	struct membuf to_softe = membuf_at(&to,
+					offsetof(struct pt_regs, softe));
+#endif
 	int i;
 
 	if (target->thread.regs == NULL)
@@ -233,7 +237,9 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 				sizeof(struct user_pt_regs));
 
 	membuf_store(&to_msr, get_user_msr(target));
-
+#ifdef CONFIG_PPC64
+	membuf_store(&to_softe, 0x1ul);
+#endif
 	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
 				sizeof(struct user_pt_regs));
 }
-- 
2.25.1.362.g51ebf55



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:02   ` Oleg Nesterov
@ 2020-11-19 16:05     ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:05 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel

On 11/19, Oleg Nesterov wrote:
>
> This is not consistent and this breaks the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/

See the test-case below.

Oleg.

/* 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] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 16:05     ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 16:05 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev

On 11/19, Oleg Nesterov wrote:
>
> This is not consistent and this breaks the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/

See the test-case below.

Oleg.

/* 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] 28+ messages in thread

* Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
  2020-11-19 16:02   ` Oleg Nesterov
@ 2020-11-19 17:16     ` Christophe Leroy
  -1 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:16 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel



Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> gpr_get() does membuf_write() twice to override pt_regs->msr in between.

Is there anything wrong with that ?

> We can call membuf_write() once and change ->msr in the kernel buffer,
> this simplifies the code and the next fix.
> 
> The patch adds a new simple helper, membuf_at(offs), it returns the new
> membuf which can be safely used after membuf_write().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 13 +++++--------
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 13 +++++--------
>   include/linux/regset.h                   | 12 ++++++++++++
>   3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> index 54f2d076206f..f8fcbd85d4cb 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> @@ -86,6 +86,8 @@ int tm_cgpr_active(struct task_struct *target, const struct user_regset *regset)
>   int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   		struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +
>   	if (!cpu_has_feature(CPU_FTR_TM))
>   		return -ENODEV;
>   
> @@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   	flush_altivec_to_thread(target);
>   
>   	membuf_write(&to, &target->thread.ckpt_regs,
> -			offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_ckpt_msr(target));
> +				sizeof(struct user_pt_regs));

This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_ckpt_msr(target));
>   
> -	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -			sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

I can't see any change here except the alignment. Can you leave it as is ?


>   }
>   
>   /*
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 7e6478e7ed07..39686ede40b3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -217,6 +217,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
>   static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   		   struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
>   	int i;
>   
>   	if (target->thread.regs == NULL)
> @@ -228,17 +229,13 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   			target->thread.regs->gpr[i] = NV_REG_POISON;
>   	}
>   
> -	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_msr(target));
> +	membuf_write(&to, target->thread.regs,
> +				sizeof(struct user_pt_regs));

This should fit on a single line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_msr(target));
>   
> -	membuf_write(&to, &target->thread.regs->orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -				 sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

This should not change, it's not part of the changes for this patch.

>   }
>   
>   static int gpr_set(struct task_struct *target, const struct user_regset *regset,
> diff --git a/include/linux/regset.h b/include/linux/regset.h
> index c3403f328257..a00765f0e8cf 100644
> --- a/include/linux/regset.h
> +++ b/include/linux/regset.h
> @@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
>   	return s->left;
>   }
>   
> +static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> +{
> +	struct membuf n = *s;

Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

> +
> +	if (offs > n.left)
> +		offs = n.left;
> +	n.p += offs;
> +	n.left -= offs;
> +
> +	return n;
> +}
> +
>   /* current s->p must be aligned for v; v must be a scalar */
>   #define membuf_store(s, v)				\
>   ({							\
> 

Christophe

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
@ 2020-11-19 17:16     ` Christophe Leroy
  0 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:16 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev



Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> gpr_get() does membuf_write() twice to override pt_regs->msr in between.

Is there anything wrong with that ?

> We can call membuf_write() once and change ->msr in the kernel buffer,
> this simplifies the code and the next fix.
> 
> The patch adds a new simple helper, membuf_at(offs), it returns the new
> membuf which can be safely used after membuf_write().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 13 +++++--------
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 13 +++++--------
>   include/linux/regset.h                   | 12 ++++++++++++
>   3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> index 54f2d076206f..f8fcbd85d4cb 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> @@ -86,6 +86,8 @@ int tm_cgpr_active(struct task_struct *target, const struct user_regset *regset)
>   int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   		struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +
>   	if (!cpu_has_feature(CPU_FTR_TM))
>   		return -ENODEV;
>   
> @@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   	flush_altivec_to_thread(target);
>   
>   	membuf_write(&to, &target->thread.ckpt_regs,
> -			offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_ckpt_msr(target));
> +				sizeof(struct user_pt_regs));

This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_ckpt_msr(target));
>   
> -	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -			sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

I can't see any change here except the alignment. Can you leave it as is ?


>   }
>   
>   /*
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 7e6478e7ed07..39686ede40b3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -217,6 +217,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
>   static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   		   struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
>   	int i;
>   
>   	if (target->thread.regs == NULL)
> @@ -228,17 +229,13 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   			target->thread.regs->gpr[i] = NV_REG_POISON;
>   	}
>   
> -	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_msr(target));
> +	membuf_write(&to, target->thread.regs,
> +				sizeof(struct user_pt_regs));

This should fit on a single line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_msr(target));
>   
> -	membuf_write(&to, &target->thread.regs->orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -				 sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

This should not change, it's not part of the changes for this patch.

>   }
>   
>   static int gpr_set(struct task_struct *target, const struct user_regset *regset,
> diff --git a/include/linux/regset.h b/include/linux/regset.h
> index c3403f328257..a00765f0e8cf 100644
> --- a/include/linux/regset.h
> +++ b/include/linux/regset.h
> @@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
>   	return s->left;
>   }
>   
> +static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> +{
> +	struct membuf n = *s;

Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

> +
> +	if (offs > n.left)
> +		offs = n.left;
> +	n.p += offs;
> +	n.left -= offs;
> +
> +	return n;
> +}
> +
>   /* current s->p must be aligned for v; v must be a scalar */
>   #define membuf_store(s, v)				\
>   ({							\
> 

Christophe

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:02   ` Oleg Nesterov
@ 2020-11-19 17:18     ` Christophe Leroy
  -1 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:18 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel



Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> 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 the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/
> 
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> index f8fcbd85d4cb..d0d339f86e61 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> @@ -87,6 +87,10 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   		struct membuf to)
>   {
>   	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +#ifdef CONFIG_PPC64
> +	struct membuf to_softe = membuf_at(&to,
> +					offsetof(struct pt_regs, softe));

Should fit on a single line I think.

> +#endif
>   
>   	if (!cpu_has_feature(CPU_FTR_TM))
>   		return -ENODEV;
> @@ -102,7 +106,9 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   				sizeof(struct user_pt_regs));
>   
>   	membuf_store(&to_msr, get_user_ckpt_msr(target));
> -
> +#ifdef CONFIG_PPC64
> +	membuf_store(&to_softe, 0x1ul);
> +#endif
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>   				sizeof(struct user_pt_regs));
>   }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 39686ede40b3..f554ccfcbfae 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -218,6 +218,10 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   		   struct membuf to)
>   {
>   	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +#ifdef CONFIG_PPC64
> +	struct membuf to_softe = membuf_at(&to,
> +					offsetof(struct pt_regs, softe));

Should fit on a single line I think.

> +#endif
>   	int i;
>   
>   	if (target->thread.regs == NULL)
> @@ -233,7 +237,9 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   				sizeof(struct user_pt_regs));
>   
>   	membuf_store(&to_msr, get_user_msr(target));
> -
> +#ifdef CONFIG_PPC64
> +	membuf_store(&to_softe, 0x1ul);
> +#endif
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>   				sizeof(struct user_pt_regs));
>   }
> 

Christophe

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 17:18     ` Christophe Leroy
  0 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:18 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev



Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> 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 the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/
> 
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> index f8fcbd85d4cb..d0d339f86e61 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> @@ -87,6 +87,10 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   		struct membuf to)
>   {
>   	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +#ifdef CONFIG_PPC64
> +	struct membuf to_softe = membuf_at(&to,
> +					offsetof(struct pt_regs, softe));

Should fit on a single line I think.

> +#endif
>   
>   	if (!cpu_has_feature(CPU_FTR_TM))
>   		return -ENODEV;
> @@ -102,7 +106,9 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   				sizeof(struct user_pt_regs));
>   
>   	membuf_store(&to_msr, get_user_ckpt_msr(target));
> -
> +#ifdef CONFIG_PPC64
> +	membuf_store(&to_softe, 0x1ul);
> +#endif
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>   				sizeof(struct user_pt_regs));
>   }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 39686ede40b3..f554ccfcbfae 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -218,6 +218,10 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   		   struct membuf to)
>   {
>   	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +#ifdef CONFIG_PPC64
> +	struct membuf to_softe = membuf_at(&to,
> +					offsetof(struct pt_regs, softe));

Should fit on a single line I think.

> +#endif
>   	int i;
>   
>   	if (target->thread.regs == NULL)
> @@ -233,7 +237,9 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   				sizeof(struct user_pt_regs));
>   
>   	membuf_store(&to_msr, get_user_msr(target));
> -
> +#ifdef CONFIG_PPC64
> +	membuf_store(&to_softe, 0x1ul);
> +#endif
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>   				sizeof(struct user_pt_regs));
>   }
> 

Christophe

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:01 ` Oleg Nesterov
@ 2020-11-19 17:19   ` Christophe Leroy
  -1 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:19 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel



Le 19/11/2020 à 17:01, Oleg Nesterov a écrit :
> Can we finally fix this problem? ;)
> 
> My previous attempt was ignored, see

That doesn't seems right.

Michael made some suggestion it seems, can you respond to it ?

> 
> 	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/
> 
> Now that gpr_get() was changed to use membuf API we can make a simpler fix.
> 
> Sorry, uncompiled/untested, I don't have a ppc machine.

I compiled with ppc64_defconfig, that seems ok. Still untested.

Christophe

> 
> Oleg.
> 
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 21 ++++++++++++---------
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 21 ++++++++++++---------
>   include/linux/regset.h                   | 12 ++++++++++++
>   3 files changed, 36 insertions(+), 18 deletions(-)
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 17:19   ` Christophe Leroy
  0 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 17:19 UTC (permalink / raw)
  To: Oleg Nesterov, Benjamin Herrenschmidt, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Christophe Leroy, Aneesh Kumar K.V, linux-kernel,
	Nicholas Piggin, Jan Kratochvil, Al Viro, linuxppc-dev



Le 19/11/2020 à 17:01, Oleg Nesterov a écrit :
> Can we finally fix this problem? ;)
> 
> My previous attempt was ignored, see

That doesn't seems right.

Michael made some suggestion it seems, can you respond to it ?

> 
> 	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/
> 
> Now that gpr_get() was changed to use membuf API we can make a simpler fix.
> 
> Sorry, uncompiled/untested, I don't have a ppc machine.

I compiled with ppc64_defconfig, that seems ok. Still untested.

Christophe

> 
> Oleg.
> 
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 21 ++++++++++++---------
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 21 ++++++++++++---------
>   include/linux/regset.h                   | 12 ++++++++++++
>   3 files changed, 36 insertions(+), 18 deletions(-)
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
  2020-11-19 17:16     ` Christophe Leroy
@ 2020-11-19 18:18       ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 18:18 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras, Al Viro, Aneesh Kumar K.V, Christophe Leroy,
	Jan Kratochvil, Nicholas Piggin, linuxppc-dev, linux-kernel

On 11/19, Christophe Leroy wrote:
>
>
> Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> >gpr_get() does membuf_write() twice to override pt_regs->msr in between.
>
> Is there anything wrong with that ?

Nothing wrong, but imo the code and 2/2 looks simpler after this patch.
I tried to explain this in the changelog.

> >  int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  		struct membuf to)
> >  {
> >+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> >+
> >  	if (!cpu_has_feature(CPU_FTR_TM))
> >  		return -ENODEV;
> >@@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  	flush_altivec_to_thread(target);
> >  	membuf_write(&to, &target->thread.ckpt_regs,
> >-			offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_ckpt_msr(target));
> >+				sizeof(struct user_pt_regs));
>
> This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

OK, I can change this.

> >-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> >-		     offsetof(struct pt_regs, msr) + sizeof(long));
> >+	membuf_store(&to_msr, get_user_ckpt_msr(target));
> >-	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> >-			sizeof(struct user_pt_regs) -
> >-			offsetof(struct pt_regs, orig_gpr3));
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-			sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> I can't see any change here except the alignment. Can you leave it as is ?

I just tried to make tm_cgpr_get() and gpr_get() look similar.

Sure, I can leave it as is.

Better yet, could you please fix this problem somehow so that I could forget
about the bug assigned to me?

I know nothing about powerpc, and personally I do not care about this (minor)
bug, I agree with any changes.

> >-	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_msr(target));
> >+	membuf_write(&to, target->thread.regs,
> >+				sizeof(struct user_pt_regs));
>
> This should fit on a single line.
>
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-				 sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> This should not change, it's not part of the changes for this patch.

See above, I can leave it as is.

> >--- a/include/linux/regset.h
> >+++ b/include/linux/regset.h
> >@@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
> >  	return s->left;
> >  }
> >+static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> >+{
> >+	struct membuf n = *s;
>
> Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

This matches other membuf_ helpers.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
@ 2020-11-19 18:18       ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 18:18 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil

On 11/19, Christophe Leroy wrote:
>
>
> Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> >gpr_get() does membuf_write() twice to override pt_regs->msr in between.
>
> Is there anything wrong with that ?

Nothing wrong, but imo the code and 2/2 looks simpler after this patch.
I tried to explain this in the changelog.

> >  int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  		struct membuf to)
> >  {
> >+	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> >+
> >  	if (!cpu_has_feature(CPU_FTR_TM))
> >  		return -ENODEV;
> >@@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
> >  	flush_altivec_to_thread(target);
> >  	membuf_write(&to, &target->thread.ckpt_regs,
> >-			offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_ckpt_msr(target));
> >+				sizeof(struct user_pt_regs));
>
> This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

OK, I can change this.

> >-	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> >-		     offsetof(struct pt_regs, msr) + sizeof(long));
> >+	membuf_store(&to_msr, get_user_ckpt_msr(target));
> >-	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> >-			sizeof(struct user_pt_regs) -
> >-			offsetof(struct pt_regs, orig_gpr3));
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-			sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> I can't see any change here except the alignment. Can you leave it as is ?

I just tried to make tm_cgpr_get() and gpr_get() look similar.

Sure, I can leave it as is.

Better yet, could you please fix this problem somehow so that I could forget
about the bug assigned to me?

I know nothing about powerpc, and personally I do not care about this (minor)
bug, I agree with any changes.

> >-	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> >-	membuf_store(&to, get_user_msr(target));
> >+	membuf_write(&to, target->thread.regs,
> >+				sizeof(struct user_pt_regs));
>
> This should fit on a single line.
>
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >-				 sizeof(struct user_pt_regs));
> >+				sizeof(struct user_pt_regs));
>
> This should not change, it's not part of the changes for this patch.

See above, I can leave it as is.

> >--- a/include/linux/regset.h
> >+++ b/include/linux/regset.h
> >@@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
> >  	return s->left;
> >  }
> >+static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> >+{
> >+	struct membuf n = *s;
>
> Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

This matches other membuf_ helpers.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 17:19   ` Christophe Leroy
@ 2020-11-19 18:22     ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 18:22 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras, Al Viro, Aneesh Kumar K.V, Christophe Leroy,
	Jan Kratochvil, Nicholas Piggin, linuxppc-dev, linux-kernel

On 11/19, Christophe Leroy wrote:
>
>
> Le 19/11/2020 à 17:01, Oleg Nesterov a écrit :
> >Can we finally fix this problem? ;)
> >
> >My previous attempt was ignored, see
>
> That doesn't seems right.
>
> Michael made some suggestion it seems, can you respond to it ?

I did, see https://lore.kernel.org/lkml/20200611105830.GB12500@redhat.com/

> >Sorry, uncompiled/untested, I don't have a ppc machine.
>
> I compiled with ppc64_defconfig, that seems ok. Still untested.

Thanks.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 18:22     ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 18:22 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil

On 11/19, Christophe Leroy wrote:
>
>
> Le 19/11/2020 à 17:01, Oleg Nesterov a écrit :
> >Can we finally fix this problem? ;)
> >
> >My previous attempt was ignored, see
>
> That doesn't seems right.
>
> Michael made some suggestion it seems, can you respond to it ?

I did, see https://lore.kernel.org/lkml/20200611105830.GB12500@redhat.com/

> >Sorry, uncompiled/untested, I don't have a ppc machine.
>
> I compiled with ppc64_defconfig, that seems ok. Still untested.

Thanks.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:02   ` Oleg Nesterov
@ 2020-11-19 21:10     ` Christophe Leroy
  -1 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 21:10 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras, Al Viro, Aneesh Kumar K.V, Christophe Leroy,
	Jan Kratochvil, Nicholas Piggin, linuxppc-dev, linux-kernel


Quoting Oleg Nesterov <oleg@redhat.com>:

> 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 the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/
>
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
>  arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>

I think the following should work, and not require the first patch  
(compile tested only).

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c  
b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index 54f2d076206f..f779b3bc0279 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -104,8 +104,14 @@ int tm_cgpr_get(struct task_struct *target, const  
struct user_regset *regset,
  		     offsetof(struct pt_regs, msr) + sizeof(long));

  	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
+		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,  
orig_gpr3));
+	membuf_store(&to, 1UL);
+
+	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+		     offsetof(struct pt_regs, softe) + sizeof(long));
+
+	membuf_write(&to, &target->thread.ckpt_regs.trap,
+		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
  			sizeof(struct user_pt_regs));
  }
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c  
b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 7e6478e7ed07..736bfbf33890 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -234,9 +234,21 @@ 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
+	membuf_write(&to, &target->thread.regs->orig_gpr3,
+		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,  
orig_gpr3));
+	membuf_store(&to, 1UL);
+
+	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+		     offsetof(struct pt_regs, softe) + sizeof(long));
+
+	membuf_write(&to, &target->thread.regs->trap,
+		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
+#else
  	membuf_write(&to, &target->thread.regs->orig_gpr3,
  			sizeof(struct user_pt_regs) -
  			offsetof(struct pt_regs, orig_gpr3));
+#endif
  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
  				 sizeof(struct user_pt_regs));
  }
---
Christophe

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 21:10     ` Christophe Leroy
  0 siblings, 0 replies; 28+ messages in thread
From: Christophe Leroy @ 2020-11-19 21:10 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil


Quoting Oleg Nesterov <oleg@redhat.com>:

> 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 the user-regs-peekpoke test
> from https://sourceware.org/systemtap/wiki/utrace/tests/
>
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/powerpc/kernel/ptrace/ptrace-tm.c   | 8 +++++++-
>  arch/powerpc/kernel/ptrace/ptrace-view.c | 8 +++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>

I think the following should work, and not require the first patch  
(compile tested only).

diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c  
b/arch/powerpc/kernel/ptrace/ptrace-tm.c
index 54f2d076206f..f779b3bc0279 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
@@ -104,8 +104,14 @@ int tm_cgpr_get(struct task_struct *target, const  
struct user_regset *regset,
  		     offsetof(struct pt_regs, msr) + sizeof(long));

  	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
-			sizeof(struct user_pt_regs) -
-			offsetof(struct pt_regs, orig_gpr3));
+		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,  
orig_gpr3));
+	membuf_store(&to, 1UL);
+
+	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+		     offsetof(struct pt_regs, softe) + sizeof(long));
+
+	membuf_write(&to, &target->thread.ckpt_regs.trap,
+		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
  			sizeof(struct user_pt_regs));
  }
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c  
b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 7e6478e7ed07..736bfbf33890 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -234,9 +234,21 @@ 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
+	membuf_write(&to, &target->thread.regs->orig_gpr3,
+		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,  
orig_gpr3));
+	membuf_store(&to, 1UL);
+
+	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
+		     offsetof(struct pt_regs, softe) + sizeof(long));
+
+	membuf_write(&to, &target->thread.regs->trap,
+		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
+#else
  	membuf_write(&to, &target->thread.regs->orig_gpr3,
  			sizeof(struct user_pt_regs) -
  			offsetof(struct pt_regs, orig_gpr3));
+#endif
  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
  				 sizeof(struct user_pt_regs));
  }
---
Christophe

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 21:10     ` Christophe Leroy
@ 2020-11-19 22:43       ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 22:43 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras, Al Viro, Aneesh Kumar K.V, Christophe Leroy,
	Jan Kratochvil, Nicholas Piggin, linuxppc-dev, linux-kernel

On 11/19, Christophe Leroy wrote:
>
> I think the following should work, and not require the first patch (compile
> tested only).
>
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -234,9 +234,21 @@ 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
> +	membuf_write(&to, &target->thread.regs->orig_gpr3,
> +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
> orig_gpr3));
> +	membuf_store(&to, 1UL);
> +
> +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> +		     offsetof(struct pt_regs, softe) + sizeof(long));
> +
> +	membuf_write(&to, &target->thread.regs->trap,
> +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
> +#else
>  	membuf_write(&to, &target->thread.regs->orig_gpr3,
>  			sizeof(struct user_pt_regs) -
>  			offsetof(struct pt_regs, orig_gpr3));
> +#endif
>  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>  				 sizeof(struct user_pt_regs));
>  }

Probably yes.

This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
and this is exactly what I tried to avoid, we can make a simpler fix now.

But let me repeat, I agree with any fix even if imp my version simplifies the code, just
commit this change and lets forget this problem.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-19 22:43       ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-19 22:43 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil

On 11/19, Christophe Leroy wrote:
>
> I think the following should work, and not require the first patch (compile
> tested only).
>
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -234,9 +234,21 @@ 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
> +	membuf_write(&to, &target->thread.regs->orig_gpr3,
> +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
> orig_gpr3));
> +	membuf_store(&to, 1UL);
> +
> +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> +		     offsetof(struct pt_regs, softe) + sizeof(long));
> +
> +	membuf_write(&to, &target->thread.regs->trap,
> +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
> +#else
>  	membuf_write(&to, &target->thread.regs->orig_gpr3,
>  			sizeof(struct user_pt_regs) -
>  			offsetof(struct pt_regs, orig_gpr3));
> +#endif
>  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>  				 sizeof(struct user_pt_regs));
>  }

Probably yes.

This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
and this is exactly what I tried to avoid, we can make a simpler fix now.

But let me repeat, I agree with any fix even if imp my version simplifies the code, just
commit this change and lets forget this problem.

Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 22:43       ` Oleg Nesterov
@ 2020-11-23 18:01         ` Oleg Nesterov
  -1 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-23 18:01 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
	Paul Mackerras, Al Viro, Aneesh Kumar K.V, Christophe Leroy,
	Jan Kratochvil, Nicholas Piggin, linuxppc-dev, linux-kernel

Christophe, et al,

So what?

Are you going to push your change or should I re-send 1-2 without
whitespace cleanups?

On 11/19, Oleg Nesterov wrote:
>
> On 11/19, Christophe Leroy wrote:
> >
> > I think the following should work, and not require the first patch (compile
> > tested only).
> >
> > --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> > @@ -234,9 +234,21 @@ 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
> > +	membuf_write(&to, &target->thread.regs->orig_gpr3,
> > +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
> > orig_gpr3));
> > +	membuf_store(&to, 1UL);
> > +
> > +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> > +		     offsetof(struct pt_regs, softe) + sizeof(long));
> > +
> > +	membuf_write(&to, &target->thread.regs->trap,
> > +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
> > +#else
> >  	membuf_write(&to, &target->thread.regs->orig_gpr3,
> >  			sizeof(struct user_pt_regs) -
> >  			offsetof(struct pt_regs, orig_gpr3));
> > +#endif
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >  				 sizeof(struct user_pt_regs));
> >  }
> 
> Probably yes.
> 
> This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
> and this is exactly what I tried to avoid, we can make a simpler fix now.
> 
> But let me repeat, I agree with any fix even if imp my version simplifies the code, just
> commit this change and lets forget this problem.
> 
> Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-23 18:01         ` Oleg Nesterov
  0 siblings, 0 replies; 28+ messages in thread
From: Oleg Nesterov @ 2020-11-23 18:01 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil

Christophe, et al,

So what?

Are you going to push your change or should I re-send 1-2 without
whitespace cleanups?

On 11/19, Oleg Nesterov wrote:
>
> On 11/19, Christophe Leroy wrote:
> >
> > I think the following should work, and not require the first patch (compile
> > tested only).
> >
> > --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> > @@ -234,9 +234,21 @@ 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
> > +	membuf_write(&to, &target->thread.regs->orig_gpr3,
> > +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
> > orig_gpr3));
> > +	membuf_store(&to, 1UL);
> > +
> > +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> > +		     offsetof(struct pt_regs, softe) + sizeof(long));
> > +
> > +	membuf_write(&to, &target->thread.regs->trap,
> > +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
> > +#else
> >  	membuf_write(&to, &target->thread.regs->orig_gpr3,
> >  			sizeof(struct user_pt_regs) -
> >  			offsetof(struct pt_regs, orig_gpr3));
> > +#endif
> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> >  				 sizeof(struct user_pt_regs));
> >  }
> 
> Probably yes.
> 
> This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
> and this is exactly what I tried to avoid, we can make a simpler fix now.
> 
> But let me repeat, I agree with any fix even if imp my version simplifies the code, just
> commit this change and lets forget this problem.
> 
> Oleg.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-23 18:01         ` Oleg Nesterov
@ 2020-11-24  0:53           ` Michael Ellerman
  -1 siblings, 0 replies; 28+ messages in thread
From: Michael Ellerman @ 2020-11-24  0:53 UTC (permalink / raw)
  To: Oleg Nesterov, Christophe Leroy
  Cc: Benjamin Herrenschmidt, Madhavan Srinivasan, Paul Mackerras,
	Al Viro, Aneesh Kumar K.V, Christophe Leroy, Jan Kratochvil,
	Nicholas Piggin, linuxppc-dev, linux-kernel

Oleg Nesterov <oleg@redhat.com> writes:
> Christophe, et al,
>
> So what?
>
> Are you going to push your change or should I re-send 1-2 without
> whitespace cleanups?

I'll take your 1 & 2 and fixup the whitespace issues when applying.

cheers

> On 11/19, Oleg Nesterov wrote:
>>
>> On 11/19, Christophe Leroy wrote:
>> >
>> > I think the following should work, and not require the first patch (compile
>> > tested only).
>> >
>> > --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
>> > +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
>> > @@ -234,9 +234,21 @@ 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
>> > +	membuf_write(&to, &target->thread.regs->orig_gpr3,
>> > +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
>> > orig_gpr3));
>> > +	membuf_store(&to, 1UL);
>> > +
>> > +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
>> > +		     offsetof(struct pt_regs, softe) + sizeof(long));
>> > +
>> > +	membuf_write(&to, &target->thread.regs->trap,
>> > +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
>> > +#else
>> >  	membuf_write(&to, &target->thread.regs->orig_gpr3,
>> >  			sizeof(struct user_pt_regs) -
>> >  			offsetof(struct pt_regs, orig_gpr3));
>> > +#endif
>> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>> >  				 sizeof(struct user_pt_regs));
>> >  }
>> 
>> Probably yes.
>> 
>> This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
>> and this is exactly what I tried to avoid, we can make a simpler fix now.
>> 
>> But let me repeat, I agree with any fix even if imp my version simplifies the code, just
>> commit this change and lets forget this problem.
>> 
>> Oleg.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-11-24  0:53           ` Michael Ellerman
  0 siblings, 0 replies; 28+ messages in thread
From: Michael Ellerman @ 2020-11-24  0:53 UTC (permalink / raw)
  To: Oleg Nesterov, Christophe Leroy
  Cc: Christophe Leroy, Madhavan Srinivasan, linuxppc-dev,
	Nicholas Piggin, linux-kernel, Paul Mackerras, Al Viro,
	Aneesh Kumar K.V, Jan Kratochvil

Oleg Nesterov <oleg@redhat.com> writes:
> Christophe, et al,
>
> So what?
>
> Are you going to push your change or should I re-send 1-2 without
> whitespace cleanups?

I'll take your 1 & 2 and fixup the whitespace issues when applying.

cheers

> On 11/19, Oleg Nesterov wrote:
>>
>> On 11/19, Christophe Leroy wrote:
>> >
>> > I think the following should work, and not require the first patch (compile
>> > tested only).
>> >
>> > --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
>> > +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
>> > @@ -234,9 +234,21 @@ 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
>> > +	membuf_write(&to, &target->thread.regs->orig_gpr3,
>> > +		     offsetof(struct pt_regs, softe) - offsetof(struct pt_regs,
>> > orig_gpr3));
>> > +	membuf_store(&to, 1UL);
>> > +
>> > +	BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
>> > +		     offsetof(struct pt_regs, softe) + sizeof(long));
>> > +
>> > +	membuf_write(&to, &target->thread.regs->trap,
>> > +		     sizeof(struct user_pt_regs) - offsetof(struct pt_regs, trap));
>> > +#else
>> >  	membuf_write(&to, &target->thread.regs->orig_gpr3,
>> >  			sizeof(struct user_pt_regs) -
>> >  			offsetof(struct pt_regs, orig_gpr3));
>> > +#endif
>> >  	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
>> >  				 sizeof(struct user_pt_regs));
>> >  }
>> 
>> Probably yes.
>> 
>> This mirrors the previous patch I sent (https://lore.kernel.org/lkml/20190917143753.GA12300@redhat.com/)
>> and this is exactly what I tried to avoid, we can make a simpler fix now.
>> 
>> But let me repeat, I agree with any fix even if imp my version simplifies the code, just
>> commit this change and lets forget this problem.
>> 
>> Oleg.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
  2020-11-19 16:01 ` Oleg Nesterov
@ 2020-12-10 11:30   ` Michael Ellerman
  -1 siblings, 0 replies; 28+ messages in thread
From: Michael Ellerman @ 2020-12-10 11:30 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Oleg Nesterov, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: linuxppc-dev, Al Viro, Nicholas Piggin, linux-kernel,
	Jan Kratochvil, Christophe Leroy, Aneesh Kumar K.V

On Thu, 19 Nov 2020 17:01:54 +0100, Oleg Nesterov wrote:
> Can we finally fix this problem? ;)
> 
> My previous attempt was ignored, see
> 
> 	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/
> 
> Now that gpr_get() was changed to use membuf API we can make a simpler fix.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/ptrace: Simplify gpr_get()/tm_cgpr_get()
      https://git.kernel.org/powerpc/c/640586f8af356096e084d69a9909d217852bde48
[2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
      https://git.kernel.org/powerpc/c/324a69467f12652b21b17f9644faa967d3d8bbdf

cheers

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
@ 2020-12-10 11:30   ` Michael Ellerman
  0 siblings, 0 replies; 28+ messages in thread
From: Michael Ellerman @ 2020-12-10 11:30 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Oleg Nesterov, Madhavan Srinivasan,
	Michael Ellerman, Paul Mackerras
  Cc: Christophe Leroy, linuxppc-dev, linux-kernel, Nicholas Piggin,
	Al Viro, Aneesh Kumar K.V, Jan Kratochvil

On Thu, 19 Nov 2020 17:01:54 +0100, Oleg Nesterov wrote:
> Can we finally fix this problem? ;)
> 
> My previous attempt was ignored, see
> 
> 	https://lore.kernel.org/lkml/20190917121256.GA8659@redhat.com/
> 
> Now that gpr_get() was changed to use membuf API we can make a simpler fix.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/ptrace: Simplify gpr_get()/tm_cgpr_get()
      https://git.kernel.org/powerpc/c/640586f8af356096e084d69a9909d217852bde48
[2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too
      https://git.kernel.org/powerpc/c/324a69467f12652b21b17f9644faa967d3d8bbdf

cheers

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-12-10 14:41 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 16:01 [PATCH v3 0/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too Oleg Nesterov
2020-11-19 16:01 ` Oleg Nesterov
2020-11-19 16:02 ` [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get Oleg Nesterov
2020-11-19 16:02   ` Oleg Nesterov
2020-11-19 17:16   ` Christophe Leroy
2020-11-19 17:16     ` Christophe Leroy
2020-11-19 18:18     ` Oleg Nesterov
2020-11-19 18:18       ` Oleg Nesterov
2020-11-19 16:02 ` [PATCH v3 2/2] powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too Oleg Nesterov
2020-11-19 16:02   ` Oleg Nesterov
2020-11-19 16:05   ` Oleg Nesterov
2020-11-19 16:05     ` Oleg Nesterov
2020-11-19 17:18   ` Christophe Leroy
2020-11-19 17:18     ` Christophe Leroy
2020-11-19 21:10   ` Christophe Leroy
2020-11-19 21:10     ` Christophe Leroy
2020-11-19 22:43     ` Oleg Nesterov
2020-11-19 22:43       ` Oleg Nesterov
2020-11-23 18:01       ` Oleg Nesterov
2020-11-23 18:01         ` Oleg Nesterov
2020-11-24  0:53         ` Michael Ellerman
2020-11-24  0:53           ` Michael Ellerman
2020-11-19 17:19 ` [PATCH v3 0/2] " Christophe Leroy
2020-11-19 17:19   ` Christophe Leroy
2020-11-19 18:22   ` Oleg Nesterov
2020-11-19 18:22     ` Oleg Nesterov
2020-12-10 11:30 ` Michael Ellerman
2020-12-10 11:30   ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.