diff mbox

[v3,1/3] stacktrace: move arch_within_stack_frames from thread_info.h

Message ID 1523275156-29087-2-git-send-email-kpark3469@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

kpark3469@gmail.com April 9, 2018, 11:59 a.m. UTC 
From: Sahara <keun-o.park@darkmatter.ae>

Since the inlined arch_within_stack_frames function was placed within
asm/thread_info.h, using stacktrace functions to unwind stack was
restricted. Now in order to have this function use more abundant apis,
it is moved to architecture's stacktrace.c. And, it is changed from
inline to noinline function to clearly have three depth calls like:

do_usercopy_stack()
  copy_[to|from]_user() : inline
    check_copy_size() : inline
      __check_object_size()
        check_stack_object()
          arch_within_stack_frames()

With this change, the x86's implementation was slightly changed also.

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/thread_info.h | 51 +-------------------------------------
 arch/x86/kernel/Makefile           |  2 +-
 arch/x86/kernel/stacktrace.c       | 49 ++++++++++++++++++++++++++++++++++++
 include/linux/page_ext.h           |  1 -
 include/linux/stacktrace.h         | 24 ++++++++++++++++++
 include/linux/thread_info.h        | 21 ----------------
 mm/usercopy.c                      |  2 +-
 7 files changed, 76 insertions(+), 74 deletions(-)

Comments

kernel test robot April 9, 2018, 9:03 p.m. UTC | #1 
Hi Sahara,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16 next-20180409]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/kpark3469-gmail-com/usercopy-reimplement-arch_within_stack_frames/20180409-221953
config: x86_64-randconfig-s0-04100256 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/drm_mm.c: In function 'save_stack':
   drivers/gpu//drm/drm_mm.c:109:9: error: variable 'trace' has initializer but incomplete type
     struct stack_trace trace = {
            ^~~~~~~~~~~
>> drivers/gpu//drm/drm_mm.c:110:3: error: unknown field 'entries' specified in initializer
      .entries = entries,
      ^
   drivers/gpu//drm/drm_mm.c:110:14: warning: excess elements in struct initializer
      .entries = entries,
                 ^~~~~~~
   drivers/gpu//drm/drm_mm.c:110:14: note: (near initialization for 'trace')
>> drivers/gpu//drm/drm_mm.c:111:3: error: unknown field 'max_entries' specified in initializer
      .max_entries = STACKDEPTH,
      ^
   drivers/gpu//drm/drm_mm.c:103:20: warning: excess elements in struct initializer
    #define STACKDEPTH 32
                       ^
   drivers/gpu//drm/drm_mm.c:111:18: note: in expansion of macro 'STACKDEPTH'
      .max_entries = STACKDEPTH,
                     ^~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:103:20: note: (near initialization for 'trace')
    #define STACKDEPTH 32
                       ^
   drivers/gpu//drm/drm_mm.c:111:18: note: in expansion of macro 'STACKDEPTH'
      .max_entries = STACKDEPTH,
                     ^~~~~~~~~~
>> drivers/gpu//drm/drm_mm.c:112:3: error: unknown field 'skip' specified in initializer
      .skip = 1
      ^
   drivers/gpu//drm/drm_mm.c:112:11: warning: excess elements in struct initializer
      .skip = 1
              ^
   drivers/gpu//drm/drm_mm.c:112:11: note: (near initialization for 'trace')
   drivers/gpu//drm/drm_mm.c:109:21: error: storage size of 'trace' isn't known
     struct stack_trace trace = {
                        ^~~~~
>> drivers/gpu//drm/drm_mm.c:115:2: error: implicit declaration of function 'save_stack_trace' [-Werror=implicit-function-declaration]
     save_stack_trace(&trace);
     ^~~~~~~~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:109:21: warning: unused variable 'trace' [-Wunused-variable]
     struct stack_trace trace = {
                        ^~~~~
   drivers/gpu//drm/drm_mm.c: In function 'show_leaks':
   drivers/gpu//drm/drm_mm.c:135:10: error: variable 'trace' has initializer but incomplete type
      struct stack_trace trace = {
             ^~~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:136:4: error: unknown field 'entries' specified in initializer
       .entries = entries,
       ^
   drivers/gpu//drm/drm_mm.c:136:15: warning: excess elements in struct initializer
       .entries = entries,
                  ^~~~~~~
   drivers/gpu//drm/drm_mm.c:136:15: note: (near initialization for 'trace')
   drivers/gpu//drm/drm_mm.c:137:4: error: unknown field 'max_entries' specified in initializer
       .max_entries = STACKDEPTH
       ^
   drivers/gpu//drm/drm_mm.c:103:20: warning: excess elements in struct initializer
    #define STACKDEPTH 32
                       ^
   drivers/gpu//drm/drm_mm.c:137:19: note: in expansion of macro 'STACKDEPTH'
       .max_entries = STACKDEPTH
                      ^~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:103:20: note: (near initialization for 'trace')
    #define STACKDEPTH 32
                       ^
   drivers/gpu//drm/drm_mm.c:137:19: note: in expansion of macro 'STACKDEPTH'
       .max_entries = STACKDEPTH
                      ^~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:135:22: error: storage size of 'trace' isn't known
      struct stack_trace trace = {
                         ^~~~~
   drivers/gpu//drm/drm_mm.c:147:3: error: implicit declaration of function 'snprint_stack_trace' [-Werror=implicit-function-declaration]
      snprint_stack_trace(buf, BUFSZ, &trace, 0);
      ^~~~~~~~~~~~~~~~~~~
   drivers/gpu//drm/drm_mm.c:135:22: warning: unused variable 'trace' [-Wunused-variable]
      struct stack_trace trace = {
                         ^~~~~
   cc1: some warnings being treated as errors

vim +/max_entries +111 drivers/gpu//drm/drm_mm.c

5705670d Chris Wilson 2016-10-31  105  
5705670d Chris Wilson 2016-10-31  106  static noinline void save_stack(struct drm_mm_node *node)
5705670d Chris Wilson 2016-10-31  107  {
5705670d Chris Wilson 2016-10-31  108  	unsigned long entries[STACKDEPTH];
5705670d Chris Wilson 2016-10-31 @109  	struct stack_trace trace = {
5705670d Chris Wilson 2016-10-31 @110  		.entries = entries,
5705670d Chris Wilson 2016-10-31 @111  		.max_entries = STACKDEPTH,
5705670d Chris Wilson 2016-10-31 @112  		.skip = 1
5705670d Chris Wilson 2016-10-31  113  	};
5705670d Chris Wilson 2016-10-31  114  
5705670d Chris Wilson 2016-10-31 @115  	save_stack_trace(&trace);
5705670d Chris Wilson 2016-10-31  116  	if (trace.nr_entries != 0 &&
5705670d Chris Wilson 2016-10-31  117  	    trace.entries[trace.nr_entries-1] == ULONG_MAX)
5705670d Chris Wilson 2016-10-31  118  		trace.nr_entries--;
5705670d Chris Wilson 2016-10-31  119  
5705670d Chris Wilson 2016-10-31  120  	/* May be called under spinlock, so avoid sleeping */
5705670d Chris Wilson 2016-10-31  121  	node->stack = depot_save_stack(&trace, GFP_NOWAIT);
5705670d Chris Wilson 2016-10-31  122  }
5705670d Chris Wilson 2016-10-31  123  

:::::: The code at line 111 was first introduced by commit
:::::: 5705670d0463423337c82d00877989e7df8b169d drm: Track drm_mm allocators and show leaks on shutdown

:::::: TO: Chris Wilson <chris@chris-wilson.co.uk>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index a5d9521..e25d70a 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -156,59 +156,10 @@  struct thread_info {
  *
  * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
-#ifndef __ASSEMBLY__
-
-/*
- * Walks up the stack frames to make sure that the specified object is
- * entirely contained by a single stack frame.
- *
- * Returns:
- *	GOOD_FRAME	if within a frame
- *	BAD_STACK	if placed across a frame boundary (or outside stack)
- *	NOT_STACK	unable to determine (no frame pointers, etc)
- */
-static inline int arch_within_stack_frames(const void * const stack,
-					   const void * const stackend,
-					   const void *obj, unsigned long len)
-{
-#if defined(CONFIG_FRAME_POINTER)
-	const void *frame = NULL;
-	const void *oldframe;
-
-	oldframe = __builtin_frame_address(1);
-	if (oldframe)
-		frame = __builtin_frame_address(2);
-	/*
-	 * low ----------------------------------------------> high
-	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
-	 *                     ^----------------^
-	 *               allow copies only within here
-	 */
-	while (stack <= frame && frame < stackend) {
-		/*
-		 * If obj + len extends past the last frame, this
-		 * check won't pass and the next frame will be 0,
-		 * causing us to bail out and correctly report
-		 * the copy as invalid.
-		 */
-		if (obj + len <= frame)
-			return obj >= oldframe + 2 * sizeof(void *) ?
-				GOOD_FRAME : BAD_STACK;
-		oldframe = frame;
-		frame = *(const void * const *)frame;
-	}
-	return BAD_STACK;
-#else
-	return NOT_STACK;
-#endif
-}
-
-#else /* !__ASSEMBLY__ */
-
+#ifdef __ASSEMBLY__
 #ifdef CONFIG_X86_64
 # define cpu_current_top_of_stack (cpu_tss_rw + TSS_sp1)
 #endif
-
 #endif
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5c..3b8afd5 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -70,7 +70,7 @@  obj-$(CONFIG_IA32_EMULATION)	+= tls.o
 obj-y				+= step.o
 obj-$(CONFIG_INTEL_TXT)		+= tboot.o
 obj-$(CONFIG_ISA_DMA_API)	+= i8237.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
+obj-y				+= stacktrace.o
 obj-y				+= cpu/
 obj-y				+= acpi/
 obj-y				+= reboot.o
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 093f2ea..ff178a0 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -12,6 +12,54 @@ 
 #include <asm/stacktrace.h>
 #include <asm/unwind.h>
 
+
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ *	GOOD_FRAME	if within a frame
+ *	BAD_STACK	if placed across a frame boundary (or outside stack)
+ *	NOT_STACK	unable to determine (no frame pointers, etc)
+ */
+int arch_within_stack_frames(const void * const stack,
+			     const void * const stackend,
+			     const void *obj, unsigned long len)
+{
+#if defined(CONFIG_FRAME_POINTER)
+	const void *frame = NULL;
+	const void *oldframe;
+
+	oldframe = __builtin_frame_address(2);
+	if (oldframe)
+		frame = __builtin_frame_address(3);
+	/*
+	 * low ----------------------------------------------> high
+	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
+	 *                     ^----------------^
+	 *               allow copies only within here
+	 */
+	while (stack <= frame && frame < stackend) {
+		/*
+		 * If obj + len extends past the last frame, this
+		 * check won't pass and the next frame will be 0,
+		 * causing us to bail out and correctly report
+		 * the copy as invalid.
+		 */
+		if (obj + len <= frame)
+			return obj >= oldframe + 2 * sizeof(void *) ?
+				GOOD_FRAME : BAD_STACK;
+		oldframe = frame;
+		frame = *(const void * const *)frame;
+	}
+	return BAD_STACK;
+#else
+	return NOT_STACK;
+#endif
+}
+
+#ifdef CONFIG_STACKTRACE
+
 static int save_stack_address(struct stack_trace *trace, unsigned long addr,
 			      bool nosched)
 {
@@ -241,3 +289,4 @@  void save_stack_trace_user(struct stack_trace *trace)
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
+#endif /* CONFIG_STACKTRACE */
diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
index ca5461e..f3b7dd9 100644
--- a/include/linux/page_ext.h
+++ b/include/linux/page_ext.h
@@ -3,7 +3,6 @@ 
 #define __LINUX_PAGE_EXT_H
 
 #include <linux/types.h>
-#include <linux/stacktrace.h>
 #include <linux/stackdepot.h>
 
 struct pglist_data;
diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
index ba29a06..60bb988 100644
--- a/include/linux/stacktrace.h
+++ b/include/linux/stacktrace.h
@@ -7,6 +7,30 @@ 
 struct task_struct;
 struct pt_regs;
 
+/*
+ * For per-arch arch_within_stack_frames() implementations, defined in
+ * kernel/stacktrace.c.
+ */
+enum {
+	BAD_STACK = -1,
+	NOT_STACK = 0,
+	GOOD_FRAME,
+	GOOD_STACK,
+};
+
+#ifdef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
+extern int arch_within_stack_frames(const void * const stack,
+				    const void * const stackend,
+				    const void *obj, unsigned long len);
+#else
+static inline int arch_within_stack_frames(const void * const stack,
+					   const void * const stackend,
+					   const void *obj, unsigned long len)
+{
+	return NOT_STACK;
+}
+#endif
+
 #ifdef CONFIG_STACKTRACE
 struct stack_trace {
 	unsigned int nr_entries, max_entries;
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 34f053a..5403851 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -23,18 +23,6 @@ 
 #endif
 
 #include <linux/bitops.h>
-
-/*
- * For per-arch arch_within_stack_frames() implementations, defined in
- * asm/thread_info.h.
- */
-enum {
-	BAD_STACK = -1,
-	NOT_STACK = 0,
-	GOOD_FRAME,
-	GOOD_STACK,
-};
-
 #include <asm/thread_info.h>
 
 #ifdef __KERNEL__
@@ -92,15 +80,6 @@  static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 
 #define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
 
-#ifndef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
-static inline int arch_within_stack_frames(const void * const stack,
-					   const void * const stackend,
-					   const void *obj, unsigned long len)
-{
-	return 0;
-}
-#endif
-
 #ifdef CONFIG_HARDENED_USERCOPY
 extern void __check_object_size(const void *ptr, unsigned long n,
 					bool to_user);
diff --git a/mm/usercopy.c b/mm/usercopy.c
index e9e9325..6a74776 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -19,7 +19,7 @@ 
 #include <linux/sched.h>
 #include <linux/sched/task.h>
 #include <linux/sched/task_stack.h>
-#include <linux/thread_info.h>
+#include <linux/stacktrace.h>
 #include <asm/sections.h>
 
 /*