linux-kernel - Re: [PATCH V2] arch: patch_text: Fixup last cpu should be master

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: <YjH2e4bfTl+0/+yc@arm.com>
Date:   Wed, 16 Mar 2022 14:38:51 +0000
From:   Catalin Marinas <catalin.marinas@....com>
To:     guoren@...nel.org
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-csky@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-xtensa@...ux-xtensa.org, Guo Ren <guoren@...ux.alibaba.com>,
        Max Filippov <jcmvbkbc@...il.com>,
        Will Deacon <will@...nel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Chris Zankel <chris@...kel.net>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH V2] arch: patch_text: Fixup last cpu should be master

On Sun, Mar 13, 2022 at 09:22:21AM +0800, guoren@...nel.org wrote:
> From: Guo Ren <guoren@...ux.alibaba.com>
> 
> These patch_text implementations are using stop_machine_cpuslocked
> infrastructure with atomic cpu_count. The original idea: When the
> master CPU patch_text, the others should wait for it.

I couldn't find the original intent in the commit logs (at least not in
the arm64 logs). Maybe the intention was for the CPUs to wait for the
text patching to complete rather than the master CPU to wait for the
others to enter the cpu_relax() loop before patching.

I think your patch makes sense anyway, the master CPU would wait for all
the others to enter the cpu_relax() loop before patching and releasing
them with another increment. You probably wouldn't see any issues in
practice unless you insert probes in the multi_stop_cpu() function (or
we could mark this function as __kprobes and get rid of the extra loops
entirely).

> --- a/arch/arm64/kernel/patching.c
> +++ b/arch/arm64/kernel/patching.c
> @@ -117,8 +117,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>  	int i, ret = 0;
>  	struct aarch64_insn_patch *pp = arg;
>  
> -	/* The first CPU becomes master */
> -	if (atomic_inc_return(&pp->cpu_count) == 1) {
> +	/* The last CPU becomes master */
> +	if (atomic_inc_return(&pp->cpu_count) == num_online_cpus()) {
>  		for (i = 0; ret == 0 && i < pp->insn_cnt; i++)
>  			ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i],
>  							     pp->new_insns[i]);

For arm64:

Reviewed-by: Catalin Marinas <catalin.marinas@....com>

-- 
Catalin