linux-kernel - Re: [PATCH v5 3/8] arm: fixmap: implement __set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140911161636.GU6158@arm.com>
Date:	Thu, 11 Sep 2014 17:16:36 +0100
From:	Will Deacon <will.deacon@....com>
To:	Kees Cook <keescook@...omium.org>
Cc:	Rabin Vincent <rabin@....in>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Laura Abbott <lauraa@...eaurora.org>,
	Rob Herring <robh@...nel.org>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	"msalter@...hat.com" <msalter@...hat.com>,
	Liu hua <sdu.liu@...wei.com>,
	Nikolay Borisov <Nikolay.Borisov@....com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Doug Anderson <dianders@...gle.com>,
	Jason Wessel <jason.wessel@...driver.com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 3/8] arm: fixmap: implement __set_fixmap()

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.

Is there any way to you can WARN_ON(!called_under_stop_machine) in
set_fixmap before doing the local TLBI?

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 ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/