Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Apr 2022 09:37:33 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, mingo@redhat.com,
        Kim Phillips <kim.phillips@amd.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] x86/unwind/orc: recheck address range after stack info
 was updated V2
Message-ID: <20220418163733.hmee24hzlt4zyixm@treble>
References: <1650201791-356270-1-git-send-email-dmtrmonakhov@yandex-team.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1650201791-356270-1-git-send-email-dmtrmonakhov@yandex-team.ru>
Precedence: bulk

In the subject, the patch version should be in the [PATCH] field which
gets discarded by git.  Also "recheck" should be capitalized:

  [PATCH v2] x86/unwind/orc: Recheck address range after stack info was updated

> get_stack_info() detects stack type only by begin address, so we must
> check that address range in question is fully covered by detected stack

Try to use good grammar/punctuation.  This sentence doesn't even have a
period.

And, it should mention the fact that the root cause actually seems to be
a bug in the IBS handling code which is passing invalid regs to the
unwinder.

> 
> Otherwise following crash is possible:
> -> unwind_next_frame
>    case ORC_TYPE_REGS:
>      if (!deref_stack_regs(state, sp, &state->ip, &state->sp))
>      -> deref_stack_regs
>        -> stack_access_ok  <- addr is ok, but addr+len-1 is not, exit with success
>      *ip = READ_ONCE_NOCHECK(regs->ip); <- Here we hit stack guard fault
> 
> Original OOPS log:
> BUG: stack guard page was hit at 000000000dd984a2 (stack is 00000000d1caafca..00000000613712f0)
> kernel stack overflow (page fault): 0000 [#1] SMP NOPTI
> CPU: 93 PID: 23787 Comm: context_switch1 Not tainted 5.4.145 #1
> RIP: 0010:unwind_next_frame
> Call Trace:
>  <NMI>
>  perf_callchain_kernel
>  ..
>  get_perf_callchain
>  perf_callchain
>  perf_prepare_sample
>  perf_event_output_forward
>  ...
>  __perf_event_overflow
>  perf_ibs_handle_irq
>  ....
>  perf_ibs_nmi_handler

This is definitely more readable than before, though I think all the
'...' can be removed, since you only trimmed the '?' entries which are
just bread crumbs on the stack and not actually part of the call path.

>  nmi_handle
>  default_do_nmi
>  do_nmi
>  end_repeat_nmi
> 
> Changes since v1:
>  - Do not call on_stack() twice for valid range.

This changelog shouldn't be a part of the patch description itself.
Instead it should be below the "---" line.

Also, a diffstat would be helpful.

See "The canonical patch format" in
Documentation/process/submitting-patches.rst.

> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 794fdef2501a..38185aedf7d1 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -339,11 +339,11 @@ static bool stack_access_ok(struct unwind_state *state, unsigned long _addr,
>  	struct stack_info *info = &state->stack_info;
>  	void *addr = (void *)_addr;
>  
> -	if (!on_stack(info, addr, len) &&
> -	    (get_stack_info(addr, state->task, info, &state->stack_mask)))
> -		return false;
> +	if (on_stack(info, addr, len))
> +		return true;
>  
> -	return true;
> +	return !get_stack_info(addr, state->task, info, &state->stack_mask) &&
> +		on_stack(info, addr, len);

The change itself looks ok.

-- 
Josh