From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1041474AbdDVGIf (ORCPT ); Sat, 22 Apr 2017 02:08:35 -0400 Received: from ozlabs.org ([103.22.144.67]:45207 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1038603AbdDVGId (ORCPT ); Sat, 22 Apr 2017 02:08:33 -0400 From: Michael Ellerman To: "Naveen N. Rao" , Benjamin Herrenschmidt , Christophe Leroy , Scott Wood , Paul Mackerras Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v2 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32 In-Reply-To: <1492781365.w4t118903f.astroid@naverao1-tp.none> References: <97d45054364142af48b8767f9f9e115504d7568b.1492778567.git.christophe.leroy@c-s.fr> <1492781365.w4t118903f.astroid@naverao1-tp.none> User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Sat, 22 Apr 2017 16:08:30 +1000 Message-ID: <87fuh1ynxd.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Naveen N. Rao" writes: > Excerpts from Christophe Leroy's message of April 21, 2017 18:32: >> diff --git a/arch/powerpc/kernel/ftrace.c >> b/arch/powerpc/kernel/ftrace.c >> index 32509de6ce4c..06d2ac53f471 100644 >> --- a/arch/powerpc/kernel/ftrace.c >> +++ b/arch/powerpc/kernel/ftrace.c >> @@ -46,6 +46,7 @@ static int >> @@ -67,10 +68,11 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new) >> } >> >> /* replace the text with the new text */ >> - if (patch_instruction((unsigned int *)ip, new)) >> - return -EPERM; >> + set_kernel_text_rw(ip); >> + err = patch_instruction((unsigned int *)ip, new); >> + set_kernel_text_ro(ip); > > Is there a reason to not put those inside patch_instruction()? Yes and no. patch_instruction() is called quite early from apply_feature_fixups(), I haven't looked closely but I suspect the set_kernel_text_rx() routines won't work that early. But on the other hand patch_instruction() is used by things other than ftrace, like jump labels, so we probably want the rw/ro setting in there so that we don't have to go and fixup jump labels etc. So probably we need a raw_patch_instruction() which does just the patching (what patch_instruction() does now), and can be used early in boot. And then patch_instruction() would have the rw/ro change in it, so that all users of it are OK. eg ~=: int raw_patch_instruction(unsigned int *addr, unsigned int instr) { ... } int patch_instruction(unsigned int *addr, unsigned int instr) { int err; set_kernel_text_rw(ip); err = raw_patch_instruction((unsigned int *)ip, new); set_kernel_text_ro(ip); return err; } cheers