On 11/15/21 7:36 AM, Miroslav Benes wrote: > On Fri, 12 Nov 2021, Josh Poimboeuf wrote: > >> On Fri, Nov 12, 2021 at 10:33:36AM +0100, Peter Zijlstra wrote: >>> On Thu, Nov 11, 2021 at 05:50:03PM -0800, Josh Poimboeuf wrote: >>> >>>> Hm, I think there is actually a livepatch problem here. >>> >>> I suspected as much, because I couldn't find any code dealing with it >>> when I looked in a hurry.. :/ >>> >>>> Some ideas to fix: >>> >>>> c) Update the reliable stacktrace code to mark the stack unreliable if >>>> it has a function with ".cold" in the name? >>> >>> Why not simply match func.cold as func in the transition thing? Then >>> func won't get patched as long as it (or it's .cold part) is in use. >>> This seems like the natural thing to do. >> >> Well yes, you're basically hinting at my first two options a and b: >> >> a) Add a field to 'klp_func' which allows the patch module to specify a >> function's .cold counterpart? >> >> b) Detect such cold counterparts in klp_enable_patch()? Presumably it >> would require searching kallsyms for "<func>.cold", which is somewhat >> problematic as there might be duplicates. >> >> It's basically a two-step process: 1) match func to .cold if it exists; >> 2) check for both in klp_check_stack_func(). The above two options are >> proposals for the 1st step. The 2nd step was implied. > > This reminded me... one of the things I have on my todo list for a long > time is to add an option for a live patch creator to specify functions > which are not contained in the live patch but their presence on stacks > should be checked for. It could save some space in the final live patch > when one would add functions (callers) just because the consistency > requires it. > Yea, I've used this technique once (adding a nop to a function so kpatch-build would detect and include in klp_funcs[]) to make a set of changes safer with respect to the consistency model. Making it simpler to for the livepatch author to say, "I'm not changing foo(), but I don't want it doing anything while patching a task" sounds reasonable. > I took as a convenience feature with a low priority and forgot about it. > The problem above changes it. So should we take the opportunity and > implement both in one step? I wanted to include a list of functions in > on a patch level (klp_patch structure) and klp_check_stack() would just > have more to check. > As far as I read the original problem, I think it would solve for that, too, so I would say go for it. -- Joe
