Andrew Donnellan <a...@linux.ibm.com> writes: > If patch_branch() or patch_instruction() fails while updating a jump > label, we presently fail silently, leading to unpredictable behaviour > later on. > > Change arch_jump_label_transform() to panic on a code patching failure, > matching the existing behaviour of arch_static_call_transform(). > > Reported-by: Erhard Furtner <erhar...@mailbox.org> > Signed-off-by: Andrew Donnellan <a...@linux.ibm.com> > > --- > > Ran into this while debugging an issue that Erhard reported to me about my > PAGE_TABLE_CHECK series on a G4, where updating a static key failed > silently, but only for one call site, leading to an incorrect reference > count later on. This looks to be due to the issue fixed in [0]. A loud > failure would have saved us all considerable debugging time. > > Should I change the return type of arch_jump_label_transform() and handle > this in an arch-independent way? Are there other users of code patching > in powerpc that ought to be hardened? > > Or is this excessive? > > [0] > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4b5e6eb281d7b1ea77619bee17095f905a125168.1757003584.git.christophe.le...@csgroup.eu/ > --- > arch/powerpc/kernel/jump_label.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/jump_label.c > b/arch/powerpc/kernel/jump_label.c > index 2659e1ac8604..80d41ed7ac50 100644 > --- a/arch/powerpc/kernel/jump_label.c > +++ b/arch/powerpc/kernel/jump_label.c > @@ -12,9 +12,14 @@ void arch_jump_label_transform(struct jump_entry *entry, > enum jump_label_type type) > { > u32 *addr = (u32 *)jump_entry_code(entry); > + int err; > > if (type == JUMP_LABEL_JMP) > - patch_branch(addr, jump_entry_target(entry), 0); > + err = patch_branch(addr, jump_entry_target(entry), 0); > else > - patch_instruction(addr, ppc_inst(PPC_RAW_NOP())); > + err = patch_instruction(addr, ppc_inst(PPC_RAW_NOP())); > + > + if (err) > + panic("%s: patching failed, err %d, type %d, addr %pS, target > %pS\n", > + __func__, err, type, addr, (void > *)jump_entry_target(entry)); > }
arch_jump_label_transform() is mainly getting called from __jump_level_update() and it's used for enabling or updating static keys / branch. But static keys can also be used by drivers / module subsystem whose initialization happens late. Although I understand that if the above fails, it might fail much before, from the arch setup code itself, but panic() still feels like a big hammer. Would pr_err() print with WARN_ON_ONCE(1) would suffice in case of an err? Also you said you ran into a problem at just one call site where above was silently failing. With the above change are you able to hit the panic() now? Because from what I see in patch_instruction(), it mainly will boil down to calling __patch_mem() which always returns 0. Although there are other places where there can be an error returned, so I was wondering if that is what you were hitting or something else? -ritesh > -- > 2.51.0
