From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1044868AbdDWK0g convert rfc822-to-8bit (ORCPT ); Sun, 23 Apr 2017 06:26:36 -0400 Received: from ozlabs.org ([103.22.144.67]:38385 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1044774AbdDWK01 (ORCPT ); Sun, 23 Apr 2017 06:26:27 -0400 From: Michael Ellerman To: christophe leroy , "Naveen N. Rao" , Benjamin Herrenschmidt , 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: References: <97d45054364142af48b8767f9f9e115504d7568b.1492778567.git.christophe.leroy@c-s.fr> <1492781365.w4t118903f.astroid@naverao1-tp.none> <87fuh1ynxd.fsf@concordia.ellerman.id.au> User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Sun, 23 Apr 2017 20:26:20 +1000 Message-ID: <87bmrna08j.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org christophe leroy writes: > Le 22/04/2017 à 08:08, Michael Ellerman a écrit : >> "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; >> } > > Shouldn't we then also have some kind of protection against parallel use > of patch_instruction() like a spin_lock_irqsave(), or is it garantied > not to happen for other reasons ? > > Otherwise, we might end up with one instance setting back the kernel > text to RO while the other one has just put it RW and is about to patch > the instruction. Yes it'd need some locking for sure. "Locking left as an exercise for the reader." ;) cheers