[RFC,14/19] powerpc: split validate_sp into two functions

Most callers just want to validate an arbitrary kernel stack pointer,
some need a particular size. Make the size case the exceptional one
with an extra function.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/processor.h | 15 ++++++++++++---
 arch/powerpc/kernel/process.c        | 23 ++++++++++++++---------
 arch/powerpc/kernel/stacktrace.c     |  2 +-
 arch/powerpc/perf/callchain.c        |  6 +++---
 4 files changed, 30 insertions(+), 16 deletions(-)

On Mon, 2022-10-31 at 15:54 +1000, Nicholas Piggin wrote:
> Most callers just want to validate an arbitrary kernel stack pointer,
> some need a particular size. Make the size case the exceptional one
> with an extra function.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/processor.h | 15 ++++++++++++---
>  arch/powerpc/kernel/process.c        | 23 ++++++++++++++---------
>  arch/powerpc/kernel/stacktrace.c     |  2 +-
>  arch/powerpc/perf/callchain.c        |  6 +++---
>  4 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/processor.h
> b/arch/powerpc/include/asm/processor.h
> index 631802999d59..e96c9b8c2a60 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -374,9 +374,18 @@ static inline unsigned long __pack_fe01(unsigned
> int fpmode)
>  
>  #endif
>  
> -/* Check that a certain kernel stack pointer is valid in task_struct
> p */
> -int validate_sp(unsigned long sp, struct task_struct *p,
> -                       unsigned long nbytes);
> +/*
> + * Check that a certain kernel stack pointer is a valid (minimum
> sized)
> + * stack frame in task_struct p.
> + */
> +int validate_sp(unsigned long sp, struct task_struct *p);
> +
> +/*
> + * validate the stack frame of a particular minimum size, used for
> when we are
> + * looking at a certain object in the stack beyond the minimum.
> + */
> +int validate_sp_size(unsigned long sp, struct task_struct *p,
> +                    unsigned long nbytes);
>  
>  /*
>   * Prefetch macros.
> diff --git a/arch/powerpc/kernel/process.c
> b/arch/powerpc/kernel/process.c
> index 6cb3982a11ef..b5defea32e75 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2128,9 +2128,12 @@ static inline int
> valid_emergency_stack(unsigned long sp, struct task_struct *p,
>         return 0;
>  }
>  
> -
> -int validate_sp(unsigned long sp, struct task_struct *p,
> -                      unsigned long nbytes)
> +/*
> + * validate the stack frame of a particular minimum size, used for
> when we are
> + * looking at a certain object in the stack beyond the minimum.
> + */
> +int validate_sp_size(unsigned long sp, struct task_struct *p,
> +                    unsigned long nbytes)
>  {
>         unsigned long stack_page = (unsigned long)task_stack_page(p);
>  
> @@ -2146,7 +2149,10 @@ int validate_sp(unsigned long sp, struct
> task_struct *p,
>         return valid_emergency_stack(sp, p, nbytes);
>  }
>  
> -EXPORT_SYMBOL(validate_sp);
> +int validate_sp(unsigned long sp, struct task_struct *p)
> +{
> +       return validate_sp(sp, p, STACK_FRAME_OVERHEAD);

Hi Nick, I assume this supposed to be validate_sp_size()?  Did you get
this to compile?

> +}
>  
>  static unsigned long ___get_wchan(struct task_struct *p)
>  {
> @@ -2154,13 +2160,12 @@ static unsigned long ___get_wchan(struct
> task_struct *p)
>         int count = 0;
>  
>         sp = p->thread.ksp;
> -       if (!validate_sp(sp, p, STACK_FRAME_OVERHEAD))
> +       if (!validate_sp(sp, p))
>                 return 0;
>  
>         do {
>                 sp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
> -               if (!validate_sp(sp, p, STACK_FRAME_OVERHEAD) ||
> -                   task_is_running(p))
> +               if (!validate_sp(sp, p) || task_is_running(p))
>                         return 0;
>                 if (count > 0) {
>                         ip = READ_ONCE_NOCHECK(((unsigned long
> *)sp)[STACK_FRAME_LR_SAVE]);
> @@ -2214,7 +2219,7 @@ void __no_sanitize_address show_stack(struct
> task_struct *tsk,
>         lr = 0;
>         printk("%sCall Trace:\n", loglvl);
>         do {
> -               if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
> +               if (!validate_sp(sp, tsk))
>                         break;
>  
>                 stack = (unsigned long *) sp;
> @@ -2241,7 +2246,7 @@ void __no_sanitize_address show_stack(struct
> task_struct *tsk,
>                  * could hold a pt_regs, if that does not fit then it
> can't
>                  * have regs.
>                  */
> -               if (validate_sp(sp, tsk, STACK_SWITCH_FRAME_SIZE)
> +               if (validate_sp_size(sp, tsk,
> STACK_SWITCH_FRAME_SIZE)
>                     && stack[STACK_INT_FRAME_MARKER_LONGS] ==
> STACK_FRAME_REGS_MARKER) {
>                         struct pt_regs *regs = (struct pt_regs *)
>                                 (sp + STACK_INT_FRAME_REGS);
> diff --git a/arch/powerpc/kernel/stacktrace.c
> b/arch/powerpc/kernel/stacktrace.c
> index 453ac317a6cf..1dbbf30f265e 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -43,7 +43,7 @@ void __no_sanitize_address
> arch_stack_walk(stack_trace_consume_fn consume_entry,
>                 unsigned long *stack = (unsigned long *) sp;
>                 unsigned long newsp, ip;
>  
> -               if (!validate_sp(sp, task, STACK_FRAME_OVERHEAD))
> +               if (!validate_sp(sp, task))
>                         return;
>  
>                 newsp = stack[0];
> diff --git a/arch/powerpc/perf/callchain.c
> b/arch/powerpc/perf/callchain.c
> index b01497ed5173..6b4434dd0ff3 100644
> --- a/arch/powerpc/perf/callchain.c
> +++ b/arch/powerpc/perf/callchain.c
> @@ -27,7 +27,7 @@ static int valid_next_sp(unsigned long sp, unsigned
> long prev_sp)
>  {
>         if (sp & 0xf)
>                 return 0;               /* must be 16-byte aligned */
> -       if (!validate_sp(sp, current, STACK_FRAME_OVERHEAD))
> +       if (!validate_sp(sp, current))
>                 return 0;
>         if (sp >= prev_sp + STACK_FRAME_MIN_SIZE)
>                 return 1;
> @@ -53,7 +53,7 @@ perf_callchain_kernel(struct
> perf_callchain_entry_ctx *entry, struct pt_regs *re
>         sp = regs->gpr[1];
>         perf_callchain_store(entry, perf_instruction_pointer(regs));
>  
> -       if (!validate_sp(sp, current, STACK_FRAME_OVERHEAD))
> +       if (!validate_sp(sp, current))
>                 return;
>  
>         for (;;) {
> @@ -61,7 +61,7 @@ perf_callchain_kernel(struct
> perf_callchain_entry_ctx *entry, struct pt_regs *re
>                 next_sp = fp[0];
>  
>                 if (next_sp == sp + STACK_INT_FRAME_SIZE &&
> -                   validate_sp(sp, current, STACK_INT_FRAME_SIZE) &&
> +                   validate_sp_size(sp, current,
> STACK_INT_FRAME_SIZE) &&
>                     fp[STACK_INT_FRAME_MARKER_LONGS] ==
> STACK_FRAME_REGS_MARKER) {
>                         /*
>                          * This looks like an interrupt frame for an

On Mon Nov 7, 2022 at 10:58 AM AEST, Russell Currey wrote:
> On Mon, 2022-10-31 at 15:54 +1000, Nicholas Piggin wrote:
> > Most callers just want to validate an arbitrary kernel stack pointer,
> > some need a particular size. Make the size case the exceptional one
> > with an extra function.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/include/asm/processor.h | 15 ++++++++++++---
> >  arch/powerpc/kernel/process.c        | 23 ++++++++++++++---------
> >  arch/powerpc/kernel/stacktrace.c     |  2 +-
> >  arch/powerpc/perf/callchain.c        |  6 +++---
> >  4 files changed, 30 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/processor.h
> > b/arch/powerpc/include/asm/processor.h
> > index 631802999d59..e96c9b8c2a60 100644
> > --- a/arch/powerpc/include/asm/processor.h
> > +++ b/arch/powerpc/include/asm/processor.h
> > @@ -374,9 +374,18 @@ static inline unsigned long __pack_fe01(unsigned
> > int fpmode)
> >  
> >  #endif
> >  
> > -/* Check that a certain kernel stack pointer is valid in task_struct
> > p */
> > -int validate_sp(unsigned long sp, struct task_struct *p,
> > -                       unsigned long nbytes);
> > +/*
> > + * Check that a certain kernel stack pointer is a valid (minimum
> > sized)
> > + * stack frame in task_struct p.
> > + */
> > +int validate_sp(unsigned long sp, struct task_struct *p);
> > +
> > +/*
> > + * validate the stack frame of a particular minimum size, used for
> > when we are
> > + * looking at a certain object in the stack beyond the minimum.
> > + */
> > +int validate_sp_size(unsigned long sp, struct task_struct *p,
> > +                    unsigned long nbytes);
> >  
> >  /*
> >   * Prefetch macros.
> > diff --git a/arch/powerpc/kernel/process.c
> > b/arch/powerpc/kernel/process.c
> > index 6cb3982a11ef..b5defea32e75 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -2128,9 +2128,12 @@ static inline int
> > valid_emergency_stack(unsigned long sp, struct task_struct *p,
> >         return 0;
> >  }
> >  
> > -
> > -int validate_sp(unsigned long sp, struct task_struct *p,
> > -                      unsigned long nbytes)
> > +/*
> > + * validate the stack frame of a particular minimum size, used for
> > when we are
> > + * looking at a certain object in the stack beyond the minimum.
> > + */
> > +int validate_sp_size(unsigned long sp, struct task_struct *p,
> > +                    unsigned long nbytes)
> >  {
> >         unsigned long stack_page = (unsigned long)task_stack_page(p);
> >  
> > @@ -2146,7 +2149,10 @@ int validate_sp(unsigned long sp, struct
> > task_struct *p,
> >         return valid_emergency_stack(sp, p, nbytes);
> >  }
> >  
> > -EXPORT_SYMBOL(validate_sp);
> > +int validate_sp(unsigned long sp, struct task_struct *p)
> > +{
> > +       return validate_sp(sp, p, STACK_FRAME_OVERHEAD);
>
> Hi Nick, I assume this supposed to be validate_sp_size()?  Did you get
> this to compile?

Oops, yeah I think I sent a slightly stale version of the series. I
did fix that one.

Thanks,
Nick