On 4/9/21 4:37 PM, Josh Poimboeuf wrote: > On Fri, Apr 09, 2021 at 01:09:09PM +0100, Mark Rutland wrote: >> On Mon, Apr 05, 2021 at 03:43:09PM -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote: >>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx> >>> >>> There are a number of places in kernel code where the stack trace is not >>> reliable. Enhance the unwinder to check for those cases and mark the >>> stack trace as unreliable. Once all of the checks are in place, the unwinder >>> can provide a reliable stack trace. But before this can be used for livepatch, >>> some other entity needs to guarantee that the frame pointers are all set up >>> correctly in kernel functions. objtool is currently being worked on to >>> fill that gap. >>> >>> Except for the return address check, all the other checks involve checking >>> the return PC of every frame against certain kernel functions. To do this, >>> implement some infrastructure code: >>> >>> - Define a special_functions[] array and populate the array with >>> the special functions >> >> I'm not too keen on having to manually collate this within the unwinder, >> as it's very painful from a maintenance perspective. > > Agreed. > >> I'd much rather we could associate this information with the >> implementations of these functions, so that they're more likely to >> stay in sync. >> >> Further, I believe all the special cases are assembly functions, and >> most of those are already in special sections to begin with. I reckon >> it'd be simpler and more robust to reject unwinding based on the >> section. If we need to unwind across specific functions in those >> sections, we could opt-in with some metadata. So e.g. we could reject >> all functions in ".entry.text", special casing the EL0 entry functions >> if necessary. > > Couldn't this also end up being somewhat fragile? Saying "certain > sections are deemed unreliable" isn't necessarily obvious to somebody > who doesn't already know about it, and it could be overlooked or > forgotten over time. And there's no way to enforce it stays that way. > Good point! > FWIW, over the years we've had zero issues with encoding the frame > pointer on x86. After you save pt_regs, you encode the frame pointer to > point to it. Ideally in the same macro so it's hard to overlook. > I had the same opinion. In fact, in my encoding scheme, I have additional checks to make absolutely sure that it is a true encoding and not stack corruption. The chances of all of those values accidentally matching are, well, null. > If you're concerned about debuggers getting confused by the encoding - > which debuggers specifically? In my experience, if vmlinux has > debuginfo, gdb and most other debuggers will use DWARF (which is already > broken in asm code) and completely ignore frame pointers. > Yes. I checked gdb actually. It did not show a problem. >> I think there's a lot more code that we cannot unwind, e.g. KVM >> exception code, or almost anything marked with SYM_CODE_END(). > > Just a reminder that livepatch only unwinds blocked tasks (plus the > 'current' task which calls into livepatch). So practically speaking, it > doesn't matter whether the 'unreliable' detection has full coverage. > The only exceptions which really matter are those which end up calling > schedule(), e.g. preemption or page faults. > > Being able to consistently detect *all* possible unreliable paths would > be nice in theory, but it's unnecessary and may not be worth the extra > complexity. > You do have a point. I tried to think of arch_stack_walk_reliable() as something that should be implemented independent of livepatching. But I could not really come up with a single example of where else it would really be useful. So, if we assume that the reliable stack trace is solely for the purpose of livepatching, I agree with your earlier comments as well. Thanks! Madhavan
