linux-kernel - Re: [PATCH 3/3] perf tools: Rework prologue generation code

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: <YhjIOX5BDYh4SRZB@krava>
Date:   Fri, 25 Feb 2022 13:14:49 +0100
From:   Jiri Olsa <olsajiri@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        "linux-perf-use." <linux-perf-users@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 3/3] perf tools: Rework prologue generation code

On Wed, Feb 23, 2022 at 02:29:56PM -0800, Andrii Nakryiko wrote:

SNIP

> > and R3 is loaded in the prologue code (first 15 instructions)
> > and it also sets 'err' (R2) with the result of the reading:
> >
> >            0: (bf) r6 = r1
> >            1: (79) r3 = *(u64 *)(r6 +96)
> >            2: (bf) r7 = r10
> >            3: (07) r7 += -8
> >            4: (7b) *(u64 *)(r10 -8) = r3
> >            5: (b7) r2 = 8
> >            6: (bf) r1 = r7
> >            7: (85) call bpf_probe_read_user#-60848
> >            8: (55) if r0 != 0x0 goto pc+2
> >            9: (61) r3 = *(u32 *)(r10 -8)
> >           10: (05) goto pc+3
> >           11: (b7) r2 = 1
> >           12: (b7) r3 = 0
> >           13: (05) goto pc+1
> >           14: (b7) r2 = 0
> >           15: (bf) r1 = r6
> >
> >           16: (b7) r1 = 100
> >           17: (6b) *(u16 *)(r10 -8) = r1
> >           18: (18) r1 = 0x6c25203a6f697270
> >           20: (7b) *(u64 *)(r10 -16) = r1
> >           21: (bf) r1 = r10
> >           22: (07) r1 += -16
> >           23: (b7) r2 = 10
> >           24: (85) call bpf_trace_printk#-54848
> >           25: (b7) r0 = 1
> >           26: (95) exit
> >
> >
> > I'm still scratching my head how to workaround this.. we do want maps
> > and all the other updates to the code, but verifier won't let it pass
> > without the prologue code
> 
> ugh, perf cornered itself into supporting this crazy scheme and now

well, it just used the interface that was provided at the time

> there is no good solution. I'm still questioning the value of
> supporting this going forward. Is there an evidence that anyone is
> using this functionality at all? Is it worth it trying to carry it on
> just because we have some example that exercises this feature?

yea we discussed this again and I think we can somehow mark this
feature in perf as deprecated and remove it after some time,
because even with the workaround below it'll be pita ;-)

or people will come and scream and we will find some other solution

I already sent the rest of the changes (prog/map priv) separately
and will send some RFC for the deprecation

thanks,
jirka

> 
> Anyways, one way to solve this is to add bpf_program__set_insns() that
> could be called from prog_init_fn callback (which I just realized
> hasn't landed yet, I'll send v4 today) to prepend a simple preamble
> like this:
> 
> r1 = 0;
> r2 = 0;
> r3 = 0;
> f4 = 0;
> r5 = 0; /* how many input arguments we support? */
> 
> This will make all input arguments initialized, libbpf will be able to
> adjust all the relocations and stuff. Once this "prototype program" is
> loaded, perf can grab final instructions and replace first X
> instructions with desired preamble.
> 
> But... ugliness and horror, yeah :(
> 
> 
> >
> > jirka