From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> To: Mark Brown <broonie@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, jpoimboe@redhat.com, ardb@kernel.org, nobuta.keiya@fujitsu.com, catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org, pasha.tatashin@soleen.com, jthierry@redhat.com, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v5 1/2] arm64: Introduce stack trace reliability checks in the unwinder Date: Fri, 25 Jun 2021 12:05:18 -0500 [thread overview] Message-ID: <d9451984-d3fe-405f-f2e6-6571acd518e9@linux.microsoft.com> (raw) In-Reply-To: <20210625155127.GC4492@sirena.org.uk> On 6/25/21 10:51 AM, Mark Brown wrote: > On Fri, Jun 25, 2021 at 10:39:57AM -0500, Madhavan T. Venkataraman wrote: >> On 6/24/21 9:40 AM, Mark Rutland wrote: > >>> At a high-level, I'm on-board with keeping track of this per unwind >>> step, but if we do that then I want to be abel to use this during >>> regular unwinds (e.g. so that we can have a backtrace idicate when a >>> step is not reliable, like x86 does with '?'), and to do that we need to >>> be a little more accurate. > >> The only consumer of frame->reliable is livepatch. So, in retrospect, my >> original per-frame reliability flag was an overkill. I was just trying to >> provide extra per-frame debug information which is not really a requirement >> for livepatch. > > It's not a requirement for livepatch but if it's there a per frame > reliability flag would have other uses - for example Mark has mentioned > the way x86 prints a ? next to unreliable entries in oops output for > example, that'd be handy for people debugging issues and would have the > added bonus of ensuring that there's more constant and widespread > exercising of the reliability stuff than if it's just used for livepatch > which is a bit niche. > I agree. That is why I introduced the per-frame flag. So, let us try a different approach. First, let us get rid of the frame->reliable flag from this patch series. That flag can be implemented when all of the pieces are in place for per-frame debug and tracking. For consumers such as livepatch that don't really care about per-frame stuff, let us solve it more cleanly via the return value of unwind_frame(). Currently, the return value from unwind_frame() is a tri-state return value which is somewhat confusing. 0 means continue unwinding -error means stop unwinding. However, -ENOENT means successful termination Other values mean an error has happened. Instead, let unwind_frame() return one of 3 values: enum { UNWIND_CONTINUE, UNWIND_CONTINUE_WITH_ERRORS, UNWIND_STOP, }; All consumers will stop unwinding upon seeing UNWIND_STOP. Livepatch type consumers will stop unwinding upon seeing anything other than UNWIND_CONTINUE. Debug type consumers can choose to continue upon seeing UNWIND_CONTINUE_WITH_ERRORS. When we eventually implement per-frame stuff, debug consumers can examine the frame for more information when they see UNWIND_CONTINUE_WITH_ERRORS. This way, my patch series does not have a dependency on the per-frame enhancements. >> So, let us separate the two. I will rename frame->reliable to frame->livepatch_safe. >> This will apply to the whole stacktrace and not to every frame. > > I'd rather keep it as reliable, even with only the livepatch usage I > think it's clearer. > See suggestion above. >> Finally, it might be a good idea to perform reliability checks even in >> start_backtrace() so we don't assume that the starting frame is reliable even >> if the caller passes livepatch_safe=true. What do you think? > > That makes sense to me. > Thanks. Madhavan
WARNING: multiple messages have this Message-ID (diff)
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> To: Mark Brown <broonie@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, jpoimboe@redhat.com, ardb@kernel.org, nobuta.keiya@fujitsu.com, catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org, pasha.tatashin@soleen.com, jthierry@redhat.com, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v5 1/2] arm64: Introduce stack trace reliability checks in the unwinder Date: Fri, 25 Jun 2021 12:05:18 -0500 [thread overview] Message-ID: <d9451984-d3fe-405f-f2e6-6571acd518e9@linux.microsoft.com> (raw) In-Reply-To: <20210625155127.GC4492@sirena.org.uk> On 6/25/21 10:51 AM, Mark Brown wrote: > On Fri, Jun 25, 2021 at 10:39:57AM -0500, Madhavan T. Venkataraman wrote: >> On 6/24/21 9:40 AM, Mark Rutland wrote: > >>> At a high-level, I'm on-board with keeping track of this per unwind >>> step, but if we do that then I want to be abel to use this during >>> regular unwinds (e.g. so that we can have a backtrace idicate when a >>> step is not reliable, like x86 does with '?'), and to do that we need to >>> be a little more accurate. > >> The only consumer of frame->reliable is livepatch. So, in retrospect, my >> original per-frame reliability flag was an overkill. I was just trying to >> provide extra per-frame debug information which is not really a requirement >> for livepatch. > > It's not a requirement for livepatch but if it's there a per frame > reliability flag would have other uses - for example Mark has mentioned > the way x86 prints a ? next to unreliable entries in oops output for > example, that'd be handy for people debugging issues and would have the > added bonus of ensuring that there's more constant and widespread > exercising of the reliability stuff than if it's just used for livepatch > which is a bit niche. > I agree. That is why I introduced the per-frame flag. So, let us try a different approach. First, let us get rid of the frame->reliable flag from this patch series. That flag can be implemented when all of the pieces are in place for per-frame debug and tracking. For consumers such as livepatch that don't really care about per-frame stuff, let us solve it more cleanly via the return value of unwind_frame(). Currently, the return value from unwind_frame() is a tri-state return value which is somewhat confusing. 0 means continue unwinding -error means stop unwinding. However, -ENOENT means successful termination Other values mean an error has happened. Instead, let unwind_frame() return one of 3 values: enum { UNWIND_CONTINUE, UNWIND_CONTINUE_WITH_ERRORS, UNWIND_STOP, }; All consumers will stop unwinding upon seeing UNWIND_STOP. Livepatch type consumers will stop unwinding upon seeing anything other than UNWIND_CONTINUE. Debug type consumers can choose to continue upon seeing UNWIND_CONTINUE_WITH_ERRORS. When we eventually implement per-frame stuff, debug consumers can examine the frame for more information when they see UNWIND_CONTINUE_WITH_ERRORS. This way, my patch series does not have a dependency on the per-frame enhancements. >> So, let us separate the two. I will rename frame->reliable to frame->livepatch_safe. >> This will apply to the whole stacktrace and not to every frame. > > I'd rather keep it as reliable, even with only the livepatch usage I > think it's clearer. > See suggestion above. >> Finally, it might be a good idea to perform reliability checks even in >> start_backtrace() so we don't assume that the starting frame is reliable even >> if the caller passes livepatch_safe=true. What do you think? > > That makes sense to me. > Thanks. Madhavan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-25 17:05 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <ea0ef9ed6eb34618bcf468fbbf8bdba99e15df7d> 2021-05-26 21:49 ` [RFC PATCH v5 0/2] arm64: Implement stack trace reliability checks madvenka 2021-05-26 21:49 ` madvenka 2021-05-26 21:49 ` [RFC PATCH v5 1/2] arm64: Introduce stack trace reliability checks in the unwinder madvenka 2021-05-26 21:49 ` madvenka 2021-06-24 14:40 ` Mark Rutland 2021-06-24 14:40 ` Mark Rutland 2021-06-24 16:03 ` Mark Brown 2021-06-24 16:03 ` Mark Brown 2021-06-25 15:39 ` Madhavan T. Venkataraman 2021-06-25 15:39 ` Madhavan T. Venkataraman 2021-06-25 15:51 ` Mark Brown 2021-06-25 15:51 ` Mark Brown 2021-06-25 17:05 ` Madhavan T. Venkataraman [this message] 2021-06-25 17:05 ` Madhavan T. Venkataraman 2021-06-25 17:18 ` Madhavan T. Venkataraman 2021-06-25 17:18 ` Madhavan T. Venkataraman 2021-06-26 15:35 ` Madhavan T. Venkataraman 2021-06-26 15:35 ` Madhavan T. Venkataraman 2021-06-29 16:47 ` Josh Poimboeuf 2021-06-29 16:47 ` Josh Poimboeuf 2021-05-26 21:49 ` [RFC PATCH v5 2/2] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka 2021-05-26 21:49 ` madvenka 2021-06-04 16:24 ` Mark Brown 2021-06-04 16:24 ` Mark Brown 2021-06-04 20:38 ` Madhavan T. Venkataraman 2021-06-04 20:38 ` Madhavan T. Venkataraman 2021-06-04 16:59 ` Mark Brown 2021-06-04 16:59 ` Mark Brown 2021-06-04 20:40 ` Madhavan T. Venkataraman 2021-06-04 20:40 ` Madhavan T. Venkataraman 2021-06-16 1:52 ` Suraj Jitindar Singh 2021-06-16 1:52 ` Suraj Jitindar Singh 2021-06-16 9:15 ` nobuta.keiya 2021-06-16 9:15 ` nobuta.keiya 2021-06-16 11:10 ` Madhavan T. Venkataraman 2021-06-16 11:10 ` Madhavan T. Venkataraman 2021-06-04 15:29 ` [RFC PATCH v5 0/2] arm64: Implement stack trace reliability checks Mark Brown 2021-06-04 15:29 ` Mark Brown 2021-06-04 20:44 ` Madhavan T. Venkataraman 2021-06-04 20:44 ` Madhavan T. Venkataraman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d9451984-d3fe-405f-f2e6-6571acd518e9@linux.microsoft.com \ --to=madvenka@linux.microsoft.com \ --cc=ardb@kernel.org \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=jmorris@namei.org \ --cc=jpoimboe@redhat.com \ --cc=jthierry@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=live-patching@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=nobuta.keiya@fujitsu.com \ --cc=pasha.tatashin@soleen.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.