linux-kernel - Re: [RFC patch 40/41] stacktrace: Remove obsolete functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Apr 2019 22:33:20 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [RFC patch 40/41] stacktrace: Remove obsolete functions

On Wed, Apr 10, 2019 at 12:28:34PM +0200, Thomas Gleixner wrote:
> No more users of the struct stack_trace based interfaces. Remove them.
> 
> Remove the macro stubs for !CONFIG_STACKTRACE as well as they are pointless
> because the storage on the call sites is conditional on CONFIG_STACKTRACE
> already. No point to be 'smart'.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  include/linux/stacktrace.h |   46 +++++++++++++++------------------------------
>  kernel/stacktrace.c        |   14 -------------
>  2 files changed, 16 insertions(+), 44 deletions(-)
> 
> --- a/include/linux/stacktrace.h
> +++ b/include/linux/stacktrace.h
> @@ -8,23 +8,6 @@ struct task_struct;
>  struct pt_regs;
>  
>  #ifdef CONFIG_STACKTRACE
> -struct stack_trace {
> -	unsigned int nr_entries, max_entries;
> -	unsigned long *entries;
> -	int skip;	/* input argument: How many entries to skip */
> -};
> -
> -extern void save_stack_trace(struct stack_trace *trace);
> -extern void save_stack_trace_regs(struct pt_regs *regs,
> -				  struct stack_trace *trace);
> -extern void save_stack_trace_tsk(struct task_struct *tsk,
> -				struct stack_trace *trace);
> -extern int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> -					 struct stack_trace *trace);
> -
> -extern void print_stack_trace(struct stack_trace *trace, int spaces);
> -extern int snprint_stack_trace(char *buf, size_t size,
> -			struct stack_trace *trace, int spaces);
>  
>  extern void stack_trace_print(unsigned long *trace, unsigned int nr_entries,
>  			      int spaces);
> @@ -43,20 +26,23 @@ extern unsigned int stack_trace_save_reg
>  extern unsigned int stack_trace_save_user(unsigned long *store,
>  					  unsigned int size,
>  					  unsigned int skipnr);
> +/*
> + * The below is for stack trace internals and architecture
> + * implementations. Do not use in generic code.
> + */
> +struct stack_trace {
> +	unsigned int nr_entries, max_entries;
> +	unsigned long *entries;
> +	int skip;	/* input argument: How many entries to skip */
> +};

I was a bit surprised to see struct stack_trace still standing at the
end of the patch set, but I guess 41 patches is enough :-)  Do we want
to eventually remove the struct altogether?

I was also hoping to see the fragile "skipnr" go away in favor of
something less dependent on compiler optimizations, but I'm not sure how
feasible that would be.

Regardless, these are very nice cleanups, nice work.

> -#ifdef CONFIG_USER_STACKTRACE_SUPPORT
> +extern void save_stack_trace(struct stack_trace *trace);
> +extern void save_stack_trace_regs(struct pt_regs *regs,
> +				  struct stack_trace *trace);
> +extern void save_stack_trace_tsk(struct task_struct *tsk,
> +				struct stack_trace *trace);
> +extern int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> +					 struct stack_trace *trace);

save_stack_trace_tsk_reliable() is still in use by generic livepatch
code.

Also I wonder if it would make sense to rename these to
__save_stack_trace_*() or arch_save_stack_trace_*() to help discourage
them from being used by generic code.

-- 
Josh