netdev - Re: [PATCH bpf-next] selftests/bpf: Fix test_attach

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] [day] [month] [year] [list]

Message-ID: <YEI0cptuDzUUOaLr@krava>
Date:   Fri, 5 Mar 2021 14:38:58 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>, Yonghong Song <yhs@...com>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Yauheni Kaliuta <ykaliuta@...hat.com>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for
 powerpc uprobes

On Thu, Mar 04, 2021 at 07:04:59AM +0530, Naveen N. Rao wrote:

SNIP

> > > static inline unsigned long ppc_function_entry(void *func)
> > > {
> > > #ifdef PPC64_ELF_ABI_v2
> > > 	u32 *insn = func;
> > > 
> > > 	/*
> > > 	 * A PPC64 ABIv2 function may have a local and a global entry
> > > 	 * point. We need to use the local entry point when patching
> > > 	 * functions, so identify and step over the global entry point
> > > 	 * sequence.
> > 
> > hm, so I need to do the instructions check below as well
> 
> It's a good check, but probably not necessary. In most functions, we 
> expect to be able to probe two instructions later without much of a 
> change to affect function tracing for userspace. For this reason, we 
> just probe at an offset of 8 as a reasonable fallback.
> 
> It is definetely good if we can come up with a better approach though.
> 
> > 
> > > 	 *
> > > 	 * The global entry point sequence is always of the form:
> > > 	 *
> > > 	 * addis r2,r12,XXXX
> > > 	 * addi  r2,r2,XXXX
> > > 	 *
> > > 	 * A linker optimisation may convert the addis to lis:
> > > 	 *
> > > 	 * lis   r2,XXXX
> > > 	 * addi  r2,r2,XXXX
> > > 	 */
> > > 	if ((((*insn & OP_RT_RA_MASK) == ADDIS_R2_R12) ||
> > > 	     ((*insn & OP_RT_RA_MASK) == LIS_R2)) &&
> > > 	    ((*(insn+1) & OP_RT_RA_MASK) == ADDI_R2_R2))
> > 
> > is this check/instructions specific to kernel code?
> > 
> > In the test prog I see following instructions:
> > 
> > Dump of assembler code for function get_base_addr:
> >    0x0000000010034cb0 <+0>:     lis     r2,4256
> >    0x0000000010034cb4 <+4>:     addi    r2,r2,31488
> >    ...
> > 
> > but first instruction does not match the check in kernel code above:
> > 
> > 	1.insn value:	0x3c4010a0
> > 	2.insn value:	0x38427b00
> > 
> > the used defines are:
> > 	#define OP_RT_RA_MASK   0xffff0000UL
> > 	#define LIS_R2          0x3c020000UL
> > 	#define ADDIS_R2_R12    0x3c4c0000UL
> > 	#define ADDI_R2_R2      0x38420000UL
> 
> Good catch! That's wrong, and I suspect we haven't noticed since kernel 
> almost always ends up using the addis variant. I will send a fix for 
> this.

the new macro value from your fix works for the test,
so I'll use it in v2, so we don't just blindly go to
+8 offset.. I'll send it out shortly

> 
> > 
> > 
> > maybe we could skip the check, and run the test twice: first on
> > kallsym address and if the uprobe is not hit we will run it again
> > on address + 8
> 
> Sure, like I mentioned, I'm fine with any approach. Offset'ing into the 
> function by 8 is easy and generally works. Re-trying is fine too. The 
> proper approach will requires us to consult the symbol table and check 
> st_other field [see commit 0b3c2264ae30ed ("perf symbols: Fix kallsyms 
> perf test on ppc64le")]

I think we don't want to complicate this test with symbol table
check. I'll propose the fix with the extra instructions check
for now and we can add symbol table check in future if it's not
enough

thanks for all the info,
jirka

> 
> Thanks,
> - Naveen
>