Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 Sep 2014 09:27:03 -0700 | Subject | Re: [PATCH v5 3/8] arm: fixmap: implement __set_fixmap() | From | Kees Cook <> |
| |
On Thu, Sep 11, 2014 at 9:16 AM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Sep 11, 2014 at 05:05:14PM +0100, Kees Cook wrote: >> Actually, this doesn't make sense. If we're using >> local_flush_tlb_kernel_range() in set_fixmap, we must always run under >> stop_machine. The needs-broadcast case is solved by using >> local_flush_tlb_kernel_range(), and the TLB-miss-on-other-CPU case is >> solved by using stop_machine(). This is how the ftrace case work, >> though not via fixmap. >> >> Since we need to flush the TLB on each fixmap change during >> patch_text(), if we want to make the local_flush_tlb_... optionally >> use flush_tlb_... to avoid calling stop_machine in the >> does't-need-broadcast case, then we'd be checking in multiple places, >> making this code overly complex for this rare operation. Is there a >> good reason to complicate this code to avoid stop_machine()? >> >> I think we should just do this: >> >> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c >> index 07314af47733..5038960e3c55 100644 >> --- a/arch/arm/kernel/patch.c >> +++ b/arch/arm/kernel/patch.c >> @@ -60,16 +125,5 @@ void __kprobes patch_text(void *addr, unsigned int insn) >> .insn = insn, >> }; >> >> - if (cache_ops_need_broadcast()) { >> - stop_machine(patch_text_stop_machine, &patch, cpu_online_mask); >> - } else { >> - bool straddles_word = IS_ENABLED(CONFIG_THUMB2_KERNEL) >> - && __opcode_is_thumb32(insn) >> - && ((uintptr_t)addr & 2); >> - >> - if (straddles_word) >> - stop_machine(patch_text_stop_machine, &patch, NULL); >> - else >> - __patch_text(addr, insn); >> - } >> + stop_machine(patch_text_stop_machine, &patch, NULL); > > The reason not to use stop machine is that it's very expensive and the > architecture does provide some guarantees that mean it's not always > required... > > ... however, as I pointed out in a kprobes thread the other day, the code > you remove above is actually broken, so I wouldn't be against replacing it > all with stop_machine.
Okay, sounds good.
> Is there any way to you can WARN_ON(!called_under_stop_machine) in > set_fixmap before doing the local TLBI?
I'm not sure what the right tests for that would be. As I understand it, this can be called during early boot too, but I'm not sure how to distinguish either being early or being under stop machine. Rabin or Rob, I think you've worked on this from the perspective of early boot, any ideas?
In the meantime, I could add a comment on set_fixmap...
> > Will > > P.S. If you plan on doing an equivalent series for arm64, you should do it > before we have any broken CPUs, then you can use TLBI without worry ;)
Heh. Hopefully I can start testing arm64 soon. I think Laura sent a series for CONFIG_DEBUG_RODATA already, so I'm hoping by the time I get 64-bit hardware, this will already be done. ;)
-Kees
-- Kees Cook Chrome OS Security
| ![\](/images/icornerr.gif) |