From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CE818C61DA4 for ; Mon, 6 Mar 2023 16:53:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LcciKcv0JgRTqQILQocihUsmfFMjMJOP7AdKsfXXhI8=; b=gf7kbFLs796abE 78UbOogo+CDc3uL0dPReN8/aSkOsWlFkQ8D2FJnl+sZgQOszmI9x3QfojtUeLCG9c1hHhzLYL+VHE LgJpvLxe+qvoVL23fEBCyyQfDZPcmlmiU6pibmv6Og8eEKNBLnQnBH//ponDqErFBZ2SD/Twi5s6g c9MiegG/SQ/VHSvks58gDEpYdJQpkBFOULHNviM7A1Ud8HzBkhhIwX5pRtxW5w5SyzNJvglR6P6+7 MbG1WgdCNiKBAkVixNkAqwONgi//Pyi4DZzYsk0CbQcNQX4XKu/NamorRpzjQb0bpHjzOJxNDrGra 0di9g2fHAQn21aZ94A2g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pZE4N-00E0WY-OC; Mon, 06 Mar 2023 16:52:15 +0000 Received: from linux.microsoft.com ([13.77.154.182]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pZE4J-00E0VD-RH for linux-arm-kernel@lists.infradead.org; Mon, 06 Mar 2023 16:52:13 +0000 Received: from [192.168.254.32] (unknown [47.187.203.192]) by linux.microsoft.com (Postfix) with ESMTPSA id E36D620BBF92; Mon, 6 Mar 2023 08:52:09 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com E36D620BBF92 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1678121530; bh=sVOn/vcPfOqwuN4KZzT/D2YzwVerqFmDVpeOIxrv9dc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=fU0wypCRqWz3wIXknA/RYCcVWWyzaHutzCMOyjyl+CX4nLXJLAjZV7We+/G0FncMR dOOdjrkBO9SWpF/ul4tBjl1vhIrkHZ9ycS5sL8vTGl3bCkNZkG3mR2FJyp6/Ju3qEE xbMut7amOsTsiytyHR+OeQ6kkxWjTY1oAJMeAvrY= Message-ID: <56308235-3893-75ac-a19f-497cc203c520@linux.microsoft.com> Date: Mon, 6 Mar 2023 10:52:09 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [RFC PATCH v3 19/22] arm64: unwinder: Add a reliability check in the unwinder based on ORC Content-Language: en-US To: Suraj Jitindar Singh Cc: poimboe@redhat.com, peterz@infradead.org, chenzhongjin@huawei.com, mark.rutland@arm.com, broonie@kernel.org, nobuta.keiya@fujitsu.com, catalin.marinas@arm.com, will@kernel.org, jamorris@linux.microsoft.com, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org References: <0337266cf19f4c98388e3f6d09f590d9de258dc7> <20230202074036.507249-1-madvenka@linux.microsoft.com> <20230202074036.507249-20-madvenka@linux.microsoft.com> <88ab8c8348373e5c7c90c985dd92b5e06f32b16b.camel@gmail.com> From: "Madhavan T. Venkataraman" In-Reply-To: <88ab8c8348373e5c7c90c985dd92b5e06f32b16b.camel@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230306_085211_965436_11BD77DF X-CRM114-Status: GOOD ( 23.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2/22/23 22:07, Suraj Jitindar Singh wrote: > On Thu, 2023-02-02 at 01:40 -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" >> >> Introduce a reliability flag in struct unwind_state. This will be set >> to >> false if the PC does not have a valid ORC or if the frame pointer >> computed >> from the ORC does not match the actual frame pointer. >> >> Now that the unwinder can validate the frame pointer, introduce >> arch_stack_walk_reliable(). >> >> Signed-off-by: Madhavan T. Venkataraman >> >> --- >> arch/arm64/include/asm/stacktrace/common.h | 15 ++ >> arch/arm64/kernel/stacktrace.c | 167 >> ++++++++++++++++++++- >> 2 files changed, 175 insertions(+), 7 deletions(-) >> > > [snip] > >> -static void notrace unwind(struct unwind_state *state, >> +static int notrace unwind(struct unwind_state *state, bool >> need_reliable, >> stack_trace_consume_fn consume_entry, void >> *cookie) >> { >> - while (1) { >> - int ret; >> + int ret = 0; >> >> + while (1) { >> + if (need_reliable && !state->reliable) >> + return -EINVAL; >> if (!consume_entry(cookie, state->pc)) >> break; >> ret = unwind_next(state); >> + if (need_reliable && !ret) >> + unwind_check_reliable(state); >> if (ret < 0) >> break; >> } >> + return ret; > > nit: > > I think you're looking more for comments on the approach and the > correctness of these patches, but from an initial read I'm still > putting it all together in my head. So this comment is on the coding > style. > > The above loop seems to check the current reliability state, then > unwind a frame then check the reliability, and then break based of > something which couldn't have been updated by the line immediately > above. I propose something like: > > unwind(...) { > ret = 0; > > while (!ret) { > if (need_reliable) { > unwind_check_reliable(state); > if (!state->reliable) > return -EINVAL; > } > if (!consume_entry(cookie, state->pc)) > return -EINVAL; > ret = unwind_next(state); > } > > return ret; > } > > This also removes the need for the call to unwind_check_reliable() > before the first unwind() below in arch_stack_walk_reliable(). > OK. Suggestion sounds reasonable. Will do. Madhavan > - Suraj > >> } >> NOKPROBE_SYMBOL(unwind); >> >> @@ -216,5 +337,37 @@ noinline notrace void >> arch_stack_walk(stack_trace_consume_fn consume_entry, >> unwind_init_from_task(&state, task); >> } >> >> - unwind(&state, consume_entry, cookie); >> + unwind(&state, false, consume_entry, cookie); >> +} >> + >> +noinline notrace int arch_stack_walk_reliable( >> + stack_trace_consume_fn consume_entry, >> + void *cookie, struct task_struct *task) >> +{ >> + struct stack_info stacks[] = { >> + stackinfo_get_task(task), >> + STACKINFO_CPU(irq), >> +#if defined(CONFIG_VMAP_STACK) >> + STACKINFO_CPU(overflow), >> +#endif >> +#if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE) >> + STACKINFO_SDEI(normal), >> + STACKINFO_SDEI(critical), >> +#endif >> + }; >> + struct unwind_state state = { >> + .stacks = stacks, >> + .nr_stacks = ARRAY_SIZE(stacks), >> + }; >> + int ret; >> + >> + if (task == current) >> + unwind_init_from_caller(&state); >> + else >> + unwind_init_from_task(&state, task); >> + unwind_check_reliable(&state); >> + >> + ret = unwind(&state, true, consume_entry, cookie); >> + >> + return ret == -ENOENT ? 0 : -EINVAL; >> } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel