On Tue, Apr 07, 2020 at 07:21:06AM +0200, Christophe Leroy wrote:

Le 06/04/2020 à 23:00, Michal Suchanek a écrit :

quoted

perf_callchain_user_64 and perf_callchain_user_32 are nearly identical.
Consolidate into one function with thin wrappers.

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Michal Suchanek <redacted>
---
  arch/powerpc/perf/callchain.h    | 24 +++++++++++++++++++++++-
  arch/powerpc/perf/callchain_32.c | 21 ++-------------------
  arch/powerpc/perf/callchain_64.c | 14 ++++----------
  3 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/perf/callchain.h b/arch/powerpc/perf/callchain.h
index 7a2cb9e1181a..7540bb71cb60 100644
--- a/arch/powerpc/perf/callchain.h
+++ b/arch/powerpc/perf/callchain.h

@@ -2,7 +2,7 @@
  #ifndef _POWERPC_PERF_CALLCHAIN_H
  #define _POWERPC_PERF_CALLCHAIN_H
-int read_user_stack_slow(void __user *ptr, void *buf, int nb);
+int read_user_stack_slow(const void __user *ptr, void *buf, int nb);

Does the constification of ptr has to be in this patch ?

It was in the original patch. The code is touched anyway.

Wouldn't it be better to have it as a separate patch ?

Don't care much either way. Can resend it as separate patches.

quoted

  void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
  			    struct pt_regs *regs);
  void perf_callchain_user_32(struct perf_callchain_entry_ctx *entry,

@@ -16,4 +16,26 @@ static inline bool invalid_user_sp(unsigned long sp)
  	return (!sp || (sp & mask) || (sp > top));
  }
+/*
+ * On 32-bit we just access the address and let hash_page create a
+ * HPTE if necessary, so there is no need to fall back to reading
+ * the page tables.  Since this is called at interrupt level,
+ * do_page_fault() won't treat a DSI as a page fault.
+ */
+static inline int __read_user_stack(const void __user *ptr, void *ret,
+				    size_t size)
+{
+	int rc;
+
+	if ((unsigned long)ptr > TASK_SIZE - size ||
+			((unsigned long)ptr & (size - 1)))
+		return -EFAULT;
+	rc = probe_user_read(ret, ptr, size);
+
+	if (rc && IS_ENABLED(CONFIG_PPC64))

gcc is probably smart enough to deal with it efficiently, but it would
be more correct to test rc after checking CONFIG_PPC64.

IS_ENABLED(CONFIG_PPC64) is constant so that part of the check should be
compiled out in any case.

Thanks

Michal