Messages in this thread |  | | Subject | Re: [PATCH v2 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32 | From | christophe leroy <> | Date | Sat, 22 Apr 2017 08:58:45 +0200 |
| |
Le 22/04/2017 à 08:08, Michael Ellerman a écrit : > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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.
Christophe
--- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
|  |