linux-kernel - Re: [PATCH 6/7] arch: __get_wchan() || ARCH

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]

Message-ID: <20211022175236.GJ86184@C02TD0UTHF1T.local>
Date:   Fri, 22 Oct 2021 18:52:36 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     keescook@...omium.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com, akpm@...ux-foundation.org,
        zhengqi.arch@...edance.com, linux@...linux.org.uk,
        catalin.marinas@....com, will@...nel.org, mpe@...erman.id.au,
        paul.walmsley@...ive.com, palmer@...belt.com, hca@...ux.ibm.com,
        gor@...ux.ibm.com, borntraeger@...ibm.com,
        linux-arch@...r.kernel.org, ardb@...nel.org,
        "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>,
        broonie@...nel.org
Subject: Re: [PATCH 6/7] arch: __get_wchan() || ARCH_STACKWALK

On Fri, Oct 22, 2021 at 05:09:39PM +0200, Peter Zijlstra wrote:
> Use ARCH_STACKWALK to implement a generic __get_wchan().
> 
> STACKTRACE should be possible, but the various implementations of
> stack_trace_save_tsk() are not consistent enough for this to work.
> ARCH_STACKWALK is a smaller set of architectures with a better defined
> interface.
> 
> Since get_wchan() pins the task in a blocked state, it is not
> necessary to take a reference on the task stack, the task isn't going
> anywhere.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  arch/arm/include/asm/processor.h     |    2 -
>  arch/arm/kernel/process.c            |   22 --------------------
>  arch/arm64/include/asm/processor.h   |    2 -
>  arch/arm64/kernel/process.c          |   26 ------------------------
>  arch/powerpc/include/asm/processor.h |    2 -
>  arch/powerpc/kernel/process.c        |   37 -----------------------------------
>  arch/riscv/include/asm/processor.h   |    3 --
>  arch/riscv/kernel/stacktrace.c       |   21 -------------------
>  arch/s390/include/asm/processor.h    |    1 
>  arch/s390/kernel/process.c           |   29 ---------------------------
>  arch/x86/include/asm/processor.h     |    2 -
>  arch/x86/kernel/process.c            |   25 -----------------------
>  kernel/sched/core.c                  |   24 ++++++++++++++++++++++
>  13 files changed, 24 insertions(+), 172 deletions(-)

Nice!

> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -528,32 +528,6 @@ struct task_struct *__switch_to(struct t
>  	return last;
>  }
>  
> -unsigned long __get_wchan(struct task_struct *p)
> -{
> -	struct stackframe frame;
> -	unsigned long stack_page, ret = 0;
> -	int count = 0;
> -
> -	stack_page = (unsigned long)try_get_task_stack(p);
> -	if (!stack_page)
> -		return 0;
> -
> -	start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
> -
> -	do {
> -		if (unwind_frame(p, &frame))
> -			goto out;
> -		if (!in_sched_functions(frame.pc)) {
> -			ret = frame.pc;
> -			goto out;
> -		}
> -	} while (count++ < 16);
> -
> -out:
> -	put_task_stack(p);
> -	return ret;
> -}

> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1966,6 +1966,30 @@ bool sched_task_on_rq(struct task_struct
>  	return task_on_rq_queued(p);
>  }
>  
> +#ifdef CONFIG_ARCH_STACKWALK
> +
> +static bool consume_wchan(void *cookie, unsigned long addr)
> +{
> +	unsigned long *wchan = cookie;
> +
> +	if (in_sched_functions(addr))
> +		return true;
> +
> +	*wchan = addr;
> +	return false;
> +}
> +
> +static unsigned long __get_wchan(struct task_struct *p)
> +{
> +	unsigned long wchan = 0;
> +
> +	arch_stack_walk(consume_wchan, &wchan, p, NULL);
> +
> +	return wchan;
> +}

It's amazing how much simpler things become with the right
infrastructure!

I gave this a spin on arm64, and /proc/*/wchan looks as expected. The
code looks "obviously correct" to me given the changes in prior patches,
so FWIW:

Reviewed-by: Mark Rutland <mark.rutland@....com> [arm64]
Tested-by: Mark Rutland <mark.rutland@....com> [arm64]

Thanks,
Mark.