| Message ID | 1512516827-29797-6-git-send-email-alex.popov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/05/2017 03:33 PM, Alexander Popov wrote: > Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about > tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack > shows the current lowest_stack value and its final value from the previous > syscall. That information can be useful for estimating the STACKLEAK > performance impact for different workloads. > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > --- > arch/Kconfig | 11 +++++++++++ > arch/x86/entry/entry_32.S | 4 ++++ > arch/x86/entry/entry_64.S | 4 ++++ > arch/x86/include/asm/processor.h | 3 +++ > arch/x86/kernel/asm-offsets.c | 3 +++ > arch/x86/kernel/process_32.c | 3 +++ > arch/x86/kernel/process_64.c | 3 +++ > fs/proc/base.c | 14 ++++++++++++++ > 8 files changed, 45 insertions(+) > > diff --git a/arch/Kconfig b/arch/Kconfig > index ba8e67b..3d8405c 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -559,6 +559,17 @@ config STACKLEAK_TRACK_MIN_SIZE > this setting, don't break the poison search in erase_kstack. > If unsure, leave the default value 100. > > +config STACKLEAK_METRICS > + bool "Show STACKLEAK metrics in the /proc file system" > + depends on GCC_PLUGIN_STACKLEAK > + depends on PROC_FS > + help > + If this is set, STACKLEAK metrics for every task are available in > + the /proc file system. In particular, /proc/<pid>/lowest_stack > + shows the current lowest_stack value and its final value from the > + previous syscall. That information can be useful for estimating > + the STACKLEAK performance impact for your workloads. > + > config HAVE_CC_STACKPROTECTOR > bool > help > diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S > index 8e4f815..2b76020 100644 > --- a/arch/x86/entry/entry_32.S > +++ b/arch/x86/entry/entry_32.S > @@ -116,6 +116,10 @@ ENTRY(erase_kstack) > mov %esp, %ecx > sub %edi, %ecx > > +#ifdef CONFIG_STACKLEAK_METRICS > + mov %edi, TASK_prev_lowest_stack(%ebp) > +#endif > + > cmp $THREAD_SIZE_asm, %ecx > jb 3f > ud2 > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S > index 94f659d..32ee040 100644 > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -121,6 +121,10 @@ ENTRY(erase_kstack) > mov %esp, %ecx > sub %edi, %ecx > > +#ifdef CONFIG_STACKLEAK_METRICS > + mov %rdi, TASK_prev_lowest_stack(%r11) > +#endif > + > /* Check that the counter value is sane. */ > cmp $THREAD_SIZE_asm, %rcx > jb 3f > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index 520508d..c94fc2f 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -483,6 +483,9 @@ struct thread_struct { > > #ifdef CONFIG_GCC_PLUGIN_STACKLEAK > unsigned long lowest_stack; > +# ifdef CONFIG_STACKLEAK_METRICS > + unsigned long prev_lowest_stack; > +# endif > #endif > > unsigned int sig_on_uaccess_err:1; > diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c > index 692c10e..84c5a29 100644 > --- a/arch/x86/kernel/asm-offsets.c > +++ b/arch/x86/kernel/asm-offsets.c > @@ -43,6 +43,9 @@ void common(void) { > # ifdef CONFIG_X86_32 > OFFSET(TASK_thread_sp0, task_struct, thread.sp0); > # endif > +# ifdef CONFIG_STACKLEAK_METRICS > + OFFSET(TASK_prev_lowest_stack, task_struct, thread.prev_lowest_stack); > +# endif > #endif > > BLANK(); > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c > index 2bea3bf..5615b3d 100644 > --- a/arch/x86/kernel/process_32.c > +++ b/arch/x86/kernel/process_32.c > @@ -139,6 +139,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp, > #ifdef CONFIG_GCC_PLUGIN_STACKLEAK > p->thread.lowest_stack = (unsigned long)task_stack_page(p) + > 2 * sizeof(unsigned long); > +# ifdef CONFIG_STACKLEAK_METRICS > + p->thread.prev_lowest_stack = p->thread.lowest_stack; > +# endif > #endif > > if (unlikely(p->flags & PF_KTHREAD)) { > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c > index 1641463..d4a50ef 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -285,6 +285,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp, > #ifdef CONFIG_GCC_PLUGIN_STACKLEAK > p->thread.lowest_stack = (unsigned long)task_stack_page(p) + > 2 * sizeof(unsigned long); > +# ifdef CONFIG_STACKLEAK_METRICS > + p->thread.prev_lowest_stack = p->thread.lowest_stack; > +# endif > #endif > > savesegment(gs, p->thread.gsindex); > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 28fa852..3569446 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns, > } > #endif /* CONFIG_LIVEPATCH */ > > +#ifdef CONFIG_STACKLEAK_METRICS > +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns, > + struct pid *pid, struct task_struct *task) > +{ > + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", > + (void *)task->thread.prev_lowest_stack, > + (void *)task->thread.lowest_stack); > + return 0; > +} > +#endif /* CONFIG_STACKLEAK_METRICS */ > + This just prints the hashed value with the new pointer leak work. I don't think we want to print the fully exposed value via %px so it's not clear how valuable this proc file is now. Thanks, Laura > /* > * Thread groups > */ > @@ -2988,6 +2999,9 @@ static const struct pid_entry tgid_base_stuff[] = { > #ifdef CONFIG_LIVEPATCH > ONE("patch_state", S_IRUSR, proc_pid_patch_state), > #endif > +#ifdef CONFIG_STACKLEAK_METRICS > + ONE("lowest_stack", S_IRUGO, proc_lowest_stack), > +#endif > }; > > static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) >
On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote: > On 12/05/2017 03:33 PM, Alexander Popov wrote: >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 28fa852..3569446 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, >> struct pid_namespace *ns, >> } >> #endif /* CONFIG_LIVEPATCH */ >> +#ifdef CONFIG_STACKLEAK_METRICS >> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace >> *ns, >> + struct pid *pid, struct task_struct *task) >> +{ >> + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", >> + (void *)task->thread.prev_lowest_stack, >> + (void *)task->thread.lowest_stack); >> + return 0; >> +} >> +#endif /* CONFIG_STACKLEAK_METRICS */ >> + > > > This just prints the hashed value with the new pointer leak work. > I don't think we want to print the fully exposed value via %px so > it's not clear how valuable this proc file is now. Maybe print the size, not the location? -Kees
On 12/06/2017 12:40 PM, Kees Cook wrote: > On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote: >> On 12/05/2017 03:33 PM, Alexander Popov wrote: >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index 28fa852..3569446 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, >>> struct pid_namespace *ns, >>> } >>> #endif /* CONFIG_LIVEPATCH */ >>> +#ifdef CONFIG_STACKLEAK_METRICS >>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace >>> *ns, >>> + struct pid *pid, struct task_struct *task) >>> +{ >>> + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", >>> + (void *)task->thread.prev_lowest_stack, >>> + (void *)task->thread.lowest_stack); >>> + return 0; >>> +} >>> +#endif /* CONFIG_STACKLEAK_METRICS */ >>> + >> >> >> This just prints the hashed value with the new pointer leak work. >> I don't think we want to print the fully exposed value via %px so >> it's not clear how valuable this proc file is now. > > Maybe print the size, not the location? > > -Kees > Hmmmmm, that starts to overlap with CONFIG_DEBUG_STACK_USAGE. That's not a bad thing but it would be good to clarify what this is tracking vs. CONFIG_DEBUG_STACK_USAGE. Thanks, Laura
Hello Laura and Kees, [adding Tobin C. Harding] On 06.12.2017 22:22, Laura Abbott wrote: > On 12/05/2017 03:33 PM, Alexander Popov wrote: >> Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about >> tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack >> shows the current lowest_stack value and its final value from the previous >> syscall. That information can be useful for estimating the STACKLEAK >> performance impact for different workloads. >> >> Signed-off-by: Alexander Popov <alex.popov@linux.com> >> --- >> arch/Kconfig | 11 +++++++++++ >> arch/x86/entry/entry_32.S | 4 ++++ >> arch/x86/entry/entry_64.S | 4 ++++ >> arch/x86/include/asm/processor.h | 3 +++ >> arch/x86/kernel/asm-offsets.c | 3 +++ >> arch/x86/kernel/process_32.c | 3 +++ >> arch/x86/kernel/process_64.c | 3 +++ >> fs/proc/base.c | 14 ++++++++++++++ >> 8 files changed, 45 insertions(+) [...] >> +#ifdef CONFIG_STACKLEAK_METRICS >> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns, >> + struct pid *pid, struct task_struct *task) >> +{ >> + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", >> + (void *)task->thread.prev_lowest_stack, >> + (void *)task->thread.lowest_stack); >> + return 0; >> +} >> +#endif /* CONFIG_STACKLEAK_METRICS */ >> + > > This just prints the hashed value with the new pointer leak work. > I don't think we want to print the fully exposed value via %px so > it's not clear how valuable this proc file is now. Yes, I tested that before sending the patch. I was confused when I saw the hashed values. But setting kptr_restrict to 1 fixed that for me: root@hobbit:~# cat /proc/2627/lowest_stack prev_lowest_stack: 00000000ed8ca991 lowest_stack: 0000000040579d76 root@hobbit:~# echo 1 > /proc/sys/kernel/kptr_restrict root@hobbit:~# cat /proc/2627/lowest_stack prev_lowest_stack: ffffc9000094fdb8 lowest_stack: ffffc9000094f9e0 However, Documentation/printk-formats.txt and Documentation/sysctl/kernel.txt don't specify that behaviour. Best regards, Alexander
On Thu, Dec 07, 2017 at 10:09:27AM +0300, Alexander Popov wrote: > Hello Laura and Kees, > > [adding Tobin C. Harding] > > On 06.12.2017 22:22, Laura Abbott wrote: > > On 12/05/2017 03:33 PM, Alexander Popov wrote: > >> Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about > >> tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack > >> shows the current lowest_stack value and its final value from the previous > >> syscall. That information can be useful for estimating the STACKLEAK > >> performance impact for different workloads. > >> > >> Signed-off-by: Alexander Popov <alex.popov@linux.com> > >> --- > >> arch/Kconfig | 11 +++++++++++ > >> arch/x86/entry/entry_32.S | 4 ++++ > >> arch/x86/entry/entry_64.S | 4 ++++ > >> arch/x86/include/asm/processor.h | 3 +++ > >> arch/x86/kernel/asm-offsets.c | 3 +++ > >> arch/x86/kernel/process_32.c | 3 +++ > >> arch/x86/kernel/process_64.c | 3 +++ > >> fs/proc/base.c | 14 ++++++++++++++ > >> 8 files changed, 45 insertions(+) > > [...] > > >> +#ifdef CONFIG_STACKLEAK_METRICS > >> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns, > >> + struct pid *pid, struct task_struct *task) > >> +{ > >> + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", > >> + (void *)task->thread.prev_lowest_stack, > >> + (void *)task->thread.lowest_stack); > >> + return 0; > >> +} > >> +#endif /* CONFIG_STACKLEAK_METRICS */ > >> + > > > > This just prints the hashed value with the new pointer leak work. > > I don't think we want to print the fully exposed value via %px so > > it's not clear how valuable this proc file is now. > > Yes, I tested that before sending the patch. I was confused when I saw the > hashed values. But setting kptr_restrict to 1 fixed that for me: > > root@hobbit:~# cat /proc/2627/lowest_stack > prev_lowest_stack: 00000000ed8ca991 > lowest_stack: 0000000040579d76 > root@hobbit:~# echo 1 > /proc/sys/kernel/kptr_restrict > root@hobbit:~# cat /proc/2627/lowest_stack > prev_lowest_stack: ffffc9000094fdb8 > lowest_stack: ffffc9000094f9e0 > > However, Documentation/printk-formats.txt and Documentation/sysctl/kernel.txt > don't specify that behaviour. thanks for pointing this out. Yes, %pK hashes the address now when kptr_restrict==0 And yes, I forgot to update the docs :( will amend. thanks, Tobin.
On 07.12.2017 02:06, Laura Abbott wrote: > On 12/06/2017 12:40 PM, Kees Cook wrote: >> On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote: >>> On 12/05/2017 03:33 PM, Alexander Popov wrote: >>>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>>> index 28fa852..3569446 100644 >>>> --- a/fs/proc/base.c >>>> +++ b/fs/proc/base.c >>>> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, >>>> struct pid_namespace *ns, >>>> } >>>> #endif /* CONFIG_LIVEPATCH */ >>>> +#ifdef CONFIG_STACKLEAK_METRICS >>>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace >>>> *ns, >>>> + struct pid *pid, struct task_struct *task) >>>> +{ >>>> + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", >>>> + (void *)task->thread.prev_lowest_stack, >>>> + (void *)task->thread.lowest_stack); >>>> + return 0; >>>> +} >>>> +#endif /* CONFIG_STACKLEAK_METRICS */ >>>> + >>> >>> This just prints the hashed value with the new pointer leak work. >>> I don't think we want to print the fprev_lowest_stackully exposed value via %px so >>> it's not clear how valuable this proc file is now. >> >> Maybe print the size, not the location? Yes, I think I can print: THREAD_SIZE - (addr & (THREAD_SIZE - 1)). I can call it "stack_depth", do you like it? N.B. this value is not a really precise stack depth, because: - we don't instrument all kernel functions, so a lot of them don't update the lowest_stack value; - prev_lowest_stack is a final point of the poison search in erase_kstack(), it is not an actual stack depth. Or should I dwell on the current version and rely on non-zero kptr_restrict? > Hmmmmm, that starts to overlap with CONFIG_DEBUG_STACK_USAGE. > That's not a bad thing but it would be good to clarify what > this is tracking vs. CONFIG_DEBUG_STACK_USAGE. Thanks, Laura, I didn't know about CONFIG_DEBUG_STACK_USAGE. After testing it I think that it should remain independent, because: - it works on sysrq; - it dumps information about all tasks in the system at once; - it provides precise information (in contrast to my metrics). In addition, I guess, modifying sysrq output format might break the workflow of users, who parse it (but I'm not sure). Best regards, Alexander
diff --git a/arch/Kconfig b/arch/Kconfig index ba8e67b..3d8405c 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -559,6 +559,17 @@ config STACKLEAK_TRACK_MIN_SIZE this setting, don't break the poison search in erase_kstack. If unsure, leave the default value 100. +config STACKLEAK_METRICS + bool "Show STACKLEAK metrics in the /proc file system" + depends on GCC_PLUGIN_STACKLEAK + depends on PROC_FS + help + If this is set, STACKLEAK metrics for every task are available in + the /proc file system. In particular, /proc/<pid>/lowest_stack + shows the current lowest_stack value and its final value from the + previous syscall. That information can be useful for estimating + the STACKLEAK performance impact for your workloads. + config HAVE_CC_STACKPROTECTOR bool help diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index 8e4f815..2b76020 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -116,6 +116,10 @@ ENTRY(erase_kstack) mov %esp, %ecx sub %edi, %ecx +#ifdef CONFIG_STACKLEAK_METRICS + mov %edi, TASK_prev_lowest_stack(%ebp) +#endif + cmp $THREAD_SIZE_asm, %ecx jb 3f ud2 diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 94f659d..32ee040 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -121,6 +121,10 @@ ENTRY(erase_kstack) mov %esp, %ecx sub %edi, %ecx +#ifdef CONFIG_STACKLEAK_METRICS + mov %rdi, TASK_prev_lowest_stack(%r11) +#endif + /* Check that the counter value is sane. */ cmp $THREAD_SIZE_asm, %rcx jb 3f diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 520508d..c94fc2f 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -483,6 +483,9 @@ struct thread_struct { #ifdef CONFIG_GCC_PLUGIN_STACKLEAK unsigned long lowest_stack; +# ifdef CONFIG_STACKLEAK_METRICS + unsigned long prev_lowest_stack; +# endif #endif unsigned int sig_on_uaccess_err:1; diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c index 692c10e..84c5a29 100644 --- a/arch/x86/kernel/asm-offsets.c +++ b/arch/x86/kernel/asm-offsets.c @@ -43,6 +43,9 @@ void common(void) { # ifdef CONFIG_X86_32 OFFSET(TASK_thread_sp0, task_struct, thread.sp0); # endif +# ifdef CONFIG_STACKLEAK_METRICS + OFFSET(TASK_prev_lowest_stack, task_struct, thread.prev_lowest_stack); +# endif #endif BLANK(); diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index 2bea3bf..5615b3d 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -139,6 +139,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp, #ifdef CONFIG_GCC_PLUGIN_STACKLEAK p->thread.lowest_stack = (unsigned long)task_stack_page(p) + 2 * sizeof(unsigned long); +# ifdef CONFIG_STACKLEAK_METRICS + p->thread.prev_lowest_stack = p->thread.lowest_stack; +# endif #endif if (unlikely(p->flags & PF_KTHREAD)) { diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 1641463..d4a50ef 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -285,6 +285,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp, #ifdef CONFIG_GCC_PLUGIN_STACKLEAK p->thread.lowest_stack = (unsigned long)task_stack_page(p) + 2 * sizeof(unsigned long); +# ifdef CONFIG_STACKLEAK_METRICS + p->thread.prev_lowest_stack = p->thread.lowest_stack; +# endif #endif savesegment(gs, p->thread.gsindex); diff --git a/fs/proc/base.c b/fs/proc/base.c index 28fa852..3569446 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns, } #endif /* CONFIG_LIVEPATCH */ +#ifdef CONFIG_STACKLEAK_METRICS +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns, + struct pid *pid, struct task_struct *task) +{ + seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n", + (void *)task->thread.prev_lowest_stack, + (void *)task->thread.lowest_stack); + return 0; +} +#endif /* CONFIG_STACKLEAK_METRICS */ + /* * Thread groups */ @@ -2988,6 +2999,9 @@ static const struct pid_entry tgid_base_stuff[] = { #ifdef CONFIG_LIVEPATCH ONE("patch_state", S_IRUSR, proc_pid_patch_state), #endif +#ifdef CONFIG_STACKLEAK_METRICS + ONE("lowest_stack", S_IRUGO, proc_lowest_stack), +#endif }; static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack shows the current lowest_stack value and its final value from the previous syscall. That information can be useful for estimating the STACKLEAK performance impact for different workloads. Signed-off-by: Alexander Popov <alex.popov@linux.com> --- arch/Kconfig | 11 +++++++++++ arch/x86/entry/entry_32.S | 4 ++++ arch/x86/entry/entry_64.S | 4 ++++ arch/x86/include/asm/processor.h | 3 +++ arch/x86/kernel/asm-offsets.c | 3 +++ arch/x86/kernel/process_32.c | 3 +++ arch/x86/kernel/process_64.c | 3 +++ fs/proc/base.c | 14 ++++++++++++++ 8 files changed, 45 insertions(+)