linux-kernel - Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

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: <Z3NFwKf1FrCk2mWx@google.com>
Date: Tue, 31 Dec 2024 01:15:44 +0000
From: Peilin Ye <yepeilin@...gle.com>
To: Xu Kuohai <xukuohai@...weicloud.com>
Cc: bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Puranjay Mohan <puranjay@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Quentin Monnet <qmo@...nel.org>,
	Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
	Josh Don <joshdon@...gle.com>, Barret Rhoden <brho@...gle.com>,
	Neel Natu <neelnatu@...gle.com>,
	Benjamin Segall <bsegall@...gle.com>,
	David Vernet <dvernet@...a.com>,
	Dave Marchevsky <davemarchevsky@...a.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and
 store-release instructions

On Mon, Dec 30, 2024 at 04:27:21PM +0800, Xu Kuohai wrote:
> > > As explained above, RS and RT2 fields should be fixed to 1s.
> > 
> > I'm already setting Rs and Rt2 to all 1's here, as AARCH64_INSN_REG_ZR
> > is defined as 31 (0b11111):
> > 
> > 	AARCH64_INSN_REG_ZR = 31,
> 
> I see, but the setting of fixed bits is smomewhat of a waste of jit time.

Fair point, I'll instead make load_acq/store_rel's MASK/VALUE include
those (1) bits.

> > On a related note, I simply grabbed {load,store}_ex's MASK and VALUE,
> > then set their 15th and 23rd bits to make them load-acquire and
> > store-release:
> > 
> >    +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
> >    +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
> >     __AARCH64_INSN_FUNCS(load_ex,   0x3F400000, 0x08400000)
> >     __AARCH64_INSN_FUNCS(store_ex,  0x3F400000, 0x08000000)
> > 
> > My question is, should we extend {load,store}_ex's MASK to make them
> > contain BIT(15) and BIT(23) as well?  As-is, aarch64_insn_is_load_ex()
> > would return true for a load-acquire.
> > 
> > The only user of aarch64_insn_is_load_ex() seems to be this
> > arm64-specific kprobe code in arch/arm64/kernel/probes/decode-insn.c:
> > 
> >    #ifdef CONFIG_KPROBES
> >    static bool __kprobes
> >    is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
> >    {
> >            while (scan_start >= scan_end) {
> >                    /*
> >                     * atomic region starts from exclusive load and ends with
> >                     * exclusive store.
> >                     */
> >                    if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> >                            return false;
> >                    else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
> >                            return true;
> > 
> > But I'm not sure yet if changing {load,store}_ex's MASK would affect the
> > above code.  Do you happen to know the context?
> 
> IIUC, this code prevents kprobe from interrupting the LL-SC loop constructed
> by LDXR/STXR pair, as the kprobe trap causes unexpected memory access that
> prevents the exclusive memory access loop from exiting.
>
> Since load-acquire/store-release instructions are not used to construct LL-SC
> loop, I think it is safe to exclude them from {load,store}_ex.

Ah, I see, thanks!  I'll extend {load,store}_ex's MASK to prevent
aarch64_insn_is_{load,store}_ex() from returning false-positives for
load-acquire/store-release.

Thanks,
Peilin Ye