Perf BPF prologue generator unconditionally fetches 8 bytes for function
parameters, which causes problem on big endian machine. Thomas gives a
detail analysis for this problem:
http://lkml.kernel.org/r/[email protected]
This patch parses the type of each argument and converts data from
memory to expected type.
Signed-off-by: Wang Nan <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Thomas Richter <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Hendrik Brueckner <[email protected]>
Cc: Li Zefan <[email protected]>
---
tools/perf/tests/bpf-script-test-prologue.c | 4 ++-
tools/perf/util/bpf-prologue.c | 49 +++++++++++++++++++++++++++--
2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c
index b4ebc75..43f1e16 100644
--- a/tools/perf/tests/bpf-script-test-prologue.c
+++ b/tools/perf/tests/bpf-script-test-prologue.c
@@ -26,9 +26,11 @@ static void (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
(void *) 6;
SEC("func=null_lseek file->f_mode offset orig")
-int bpf_func__null_lseek(void *ctx, int err, unsigned long f_mode,
+int bpf_func__null_lseek(void *ctx, int err, unsigned long _f_mode,
unsigned long offset, unsigned long orig)
{
+ fmode_t f_mode = (fmode_t)_f_mode;
+
if (err)
return 0;
if (f_mode & FMODE_WRITE)
diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
index 6cdbee1..ce28993 100644
--- a/tools/perf/util/bpf-prologue.c
+++ b/tools/perf/util/bpf-prologue.c
@@ -57,6 +57,46 @@ check_pos(struct bpf_insn_pos *pos)
return 0;
}
+/*
+ * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
+ * Documentation/trace/kprobetrace.txt) to size field of BPF_LDX_MEM
+ * instruction (BPF_{B,H,W,DW}).
+ */
+static int
+argtype_to_ldx_size(const char *type)
+{
+ int arg_size = type ? atoi(&type[1]) : 64;
+
+ switch (arg_size) {
+ case 8:
+ return BPF_B;
+ case 16:
+ return BPF_H;
+ case 32:
+ return BPF_W;
+ case 64:
+ default:
+ return BPF_DW;
+ }
+}
+
+static const char *
+insn_sz_to_str(int insn_sz)
+{
+ switch (insn_sz) {
+ case BPF_B:
+ return "BPF_B";
+ case BPF_H:
+ return "BPF_H";
+ case BPF_W:
+ return "BPF_W";
+ case BPF_DW:
+ return "BPF_DW";
+ default:
+ return "UNKNOWN";
+ }
+}
+
/* Give it a shorter name */
#define ins(i, p) append_insn((i), (p))
@@ -257,9 +297,14 @@ gen_prologue_slowpath(struct bpf_insn_pos *pos,
}
/* Final pass: read to registers */
- for (i = 0; i < nargs; i++)
- ins(BPF_LDX_MEM(BPF_DW, BPF_PROLOGUE_START_ARG_REG + i,
+ for (i = 0; i < nargs; i++) {
+ int insn_sz = argtype_to_ldx_size(args[i].type);
+
+ pr_debug("prologue: load arg %d, insn_sz is %s\n",
+ i, insn_sz_to_str(insn_sz));
+ ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
+ }
ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
--
2.10.1
Hi Thomas,
Please try this patch on your machine and give me the result.
Thank you.
On 2017/8/13 2:49, Wang Nan wrote:
> Perf BPF prologue generator unconditionally fetches 8 bytes for function
> parameters, which causes problem on big endian machine. Thomas gives a
> detail analysis for this problem:
>
> http://lkml.kernel.org/r/[email protected]
>
> This patch parses the type of each argument and converts data from
> memory to expected type.
>
> Signed-off-by: Wang Nan <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Thomas Richter <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Hendrik Brueckner <[email protected]>
> Cc: Li Zefan <[email protected]>
> ---
> tools/perf/tests/bpf-script-test-prologue.c | 4 ++-
> tools/perf/util/bpf-prologue.c | 49 +++++++++++++++++++++++++++--
> 2 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c
> index b4ebc75..43f1e16 100644
> --- a/tools/perf/tests/bpf-script-test-prologue.c
> +++ b/tools/perf/tests/bpf-script-test-prologue.c
> @@ -26,9 +26,11 @@ static void (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
> (void *) 6;
>
> SEC("func=null_lseek file->f_mode offset orig")
> -int bpf_func__null_lseek(void *ctx, int err, unsigned long f_mode,
> +int bpf_func__null_lseek(void *ctx, int err, unsigned long _f_mode,
> unsigned long offset, unsigned long orig)
> {
> + fmode_t f_mode = (fmode_t)_f_mode;
> +
> if (err)
> return 0;
> if (f_mode & FMODE_WRITE)
> diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
> index 6cdbee1..ce28993 100644
> --- a/tools/perf/util/bpf-prologue.c
> +++ b/tools/perf/util/bpf-prologue.c
> @@ -57,6 +57,46 @@ check_pos(struct bpf_insn_pos *pos)
> return 0;
> }
>
> +/*
> + * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
> + * Documentation/trace/kprobetrace.txt) to size field of BPF_LDX_MEM
> + * instruction (BPF_{B,H,W,DW}).
> + */
> +static int
> +argtype_to_ldx_size(const char *type)
> +{
> + int arg_size = type ? atoi(&type[1]) : 64;
> +
> + switch (arg_size) {
> + case 8:
> + return BPF_B;
> + case 16:
> + return BPF_H;
> + case 32:
> + return BPF_W;
> + case 64:
> + default:
> + return BPF_DW;
> + }
> +}
> +
> +static const char *
> +insn_sz_to_str(int insn_sz)
> +{
> + switch (insn_sz) {
> + case BPF_B:
> + return "BPF_B";
> + case BPF_H:
> + return "BPF_H";
> + case BPF_W:
> + return "BPF_W";
> + case BPF_DW:
> + return "BPF_DW";
> + default:
> + return "UNKNOWN";
> + }
> +}
> +
> /* Give it a shorter name */
> #define ins(i, p) append_insn((i), (p))
>
> @@ -257,9 +297,14 @@ gen_prologue_slowpath(struct bpf_insn_pos *pos,
> }
>
> /* Final pass: read to registers */
> - for (i = 0; i < nargs; i++)
> - ins(BPF_LDX_MEM(BPF_DW, BPF_PROLOGUE_START_ARG_REG + i,
> + for (i = 0; i < nargs; i++) {
> + int insn_sz = argtype_to_ldx_size(args[i].type);
> +
> + pr_debug("prologue: load arg %d, insn_sz is %s\n",
> + i, insn_sz_to_str(insn_sz));
> + ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
> BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
> + }
>
> ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
>
Em Fri, Aug 11, 2017 at 06:47:56PM +0800, Wangnan (F) escreveu:
> Hi Thomas,
>
> Please try this patch on your machine and give me the result.
Right, I'm waiting for test results for the last two patches from Wang:
(3.0K) [PATCH] perf bpf: Fix endianness problem when loading parameters in prologue
(1.2K) [PATCH] perf test llvm: Fix f_mode endianness problem
Thanks,
- Arnaldo
> Thank you.
>
> On 2017/8/13 2:49, Wang Nan wrote:
> > Perf BPF prologue generator unconditionally fetches 8 bytes for function
> > parameters, which causes problem on big endian machine. Thomas gives a
> > detail analysis for this problem:
> >
> > http://lkml.kernel.org/r/[email protected]
> >
> > This patch parses the type of each argument and converts data from
> > memory to expected type.
> >
> > Signed-off-by: Wang Nan <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Thomas Richter <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Hendrik Brueckner <[email protected]>
> > Cc: Li Zefan <[email protected]>
> > ---
> > tools/perf/tests/bpf-script-test-prologue.c | 4 ++-
> > tools/perf/util/bpf-prologue.c | 49 +++++++++++++++++++++++++++--
> > 2 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c
> > index b4ebc75..43f1e16 100644
> > --- a/tools/perf/tests/bpf-script-test-prologue.c
> > +++ b/tools/perf/tests/bpf-script-test-prologue.c
> > @@ -26,9 +26,11 @@ static void (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
> > (void *) 6;
> > SEC("func=null_lseek file->f_mode offset orig")
> > -int bpf_func__null_lseek(void *ctx, int err, unsigned long f_mode,
> > +int bpf_func__null_lseek(void *ctx, int err, unsigned long _f_mode,
> > unsigned long offset, unsigned long orig)
> > {
> > + fmode_t f_mode = (fmode_t)_f_mode;
> > +
> > if (err)
> > return 0;
> > if (f_mode & FMODE_WRITE)
> > diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
> > index 6cdbee1..ce28993 100644
> > --- a/tools/perf/util/bpf-prologue.c
> > +++ b/tools/perf/util/bpf-prologue.c
> > @@ -57,6 +57,46 @@ check_pos(struct bpf_insn_pos *pos)
> > return 0;
> > }
> > +/*
> > + * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
> > + * Documentation/trace/kprobetrace.txt) to size field of BPF_LDX_MEM
> > + * instruction (BPF_{B,H,W,DW}).
> > + */
> > +static int
> > +argtype_to_ldx_size(const char *type)
> > +{
> > + int arg_size = type ? atoi(&type[1]) : 64;
> > +
> > + switch (arg_size) {
> > + case 8:
> > + return BPF_B;
> > + case 16:
> > + return BPF_H;
> > + case 32:
> > + return BPF_W;
> > + case 64:
> > + default:
> > + return BPF_DW;
> > + }
> > +}
> > +
> > +static const char *
> > +insn_sz_to_str(int insn_sz)
> > +{
> > + switch (insn_sz) {
> > + case BPF_B:
> > + return "BPF_B";
> > + case BPF_H:
> > + return "BPF_H";
> > + case BPF_W:
> > + return "BPF_W";
> > + case BPF_DW:
> > + return "BPF_DW";
> > + default:
> > + return "UNKNOWN";
> > + }
> > +}
> > +
> > /* Give it a shorter name */
> > #define ins(i, p) append_insn((i), (p))
> > @@ -257,9 +297,14 @@ gen_prologue_slowpath(struct bpf_insn_pos *pos,
> > }
> > /* Final pass: read to registers */
> > - for (i = 0; i < nargs; i++)
> > - ins(BPF_LDX_MEM(BPF_DW, BPF_PROLOGUE_START_ARG_REG + i,
> > + for (i = 0; i < nargs; i++) {
> > + int insn_sz = argtype_to_ldx_size(args[i].type);
> > +
> > + pr_debug("prologue: load arg %d, insn_sz is %s\n",
> > + i, insn_sz_to_str(insn_sz));
> > + ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
> > BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
> > + }
> > ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
>
On 08/11/2017 09:23 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Aug 11, 2017 at 06:47:56PM +0800, Wangnan (F) escreveu:
>> Hi Thomas,
>>
>> Please try this patch on your machine and give me the result.
>
> Right, I'm waiting for test results for the last two patches from Wang:
>
> (3.0K) [PATCH] perf bpf: Fix endianness problem when loading parameters in prologue
This patch does not solve the issue completely. It solves load of parameter 1 which is
loaded from 132(%r2):x32 from memory via function BPF_FUNC_probe_read.
How another issue is introduced with register r3:
The probe_events: p:perf_bpf_probe/func _text+8769704 f_mode=+132(%r2):x32
offset=%r3:s64 orig=%r4:s32
Now r4 does not have any offset at all and is not read from memory via
function BPF_FUNC_probe_read. The value is instead stored in a 8 byte register.
When I look into the generated eBPF bytes code I see
(1) r3 = *(r6 + 56) <-- R3 loaded from ctx pointer + 56
which is value of register r3 (origin of lseek)
....
(2) *(r10 - 24) = r3 <-- R3 is stored on stack position for function call)
...
(3) r3 = *(r10 - 8) <-- load f_mode into r3
(4) r4 = *(r10 - 16) <-- load offset into r4
(5) r5 = *(r10 - 24) <-- load origin into r5
I have provided a reworked patch to solve this issue. Please review it because
I do not know if it will work in all cases.
See below.
> (1.2K) [PATCH] perf test llvm: Fix f_mode endianness problem
>
> Thanks,
>
> - Arnaldo
>
>> Thank you.
>>
>> On 2017/8/13 2:49, Wang Nan wrote:
>>> Perf BPF prologue generator unconditionally fetches 8 bytes for function
>>> parameters, which causes problem on big endian machine. Thomas gives a
>>> detail analysis for this problem:
>>>
>>> http://lkml.kernel.org/r/[email protected]
>>>
>>> This patch parses the type of each argument and converts data from
>>> memory to expected type.
>>>
>>> Signed-off-by: Wang Nan <[email protected]>
>>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>>> Cc: Thomas Richter <[email protected]>
>>> Cc: Alexei Starovoitov <[email protected]>
>>> Cc: Hendrik Brueckner <[email protected]>
>>> Cc: Li Zefan <[email protected]>
>>> ---
>>> tools/perf/tests/bpf-script-test-prologue.c | 4 ++-
>>> tools/perf/util/bpf-prologue.c | 49 +++++++++++++++++++++++++++--
>>> 2 files changed, 50 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c
>>> index b4ebc75..43f1e16 100644
>>> --- a/tools/perf/tests/bpf-script-test-prologue.c
>>> +++ b/tools/perf/tests/bpf-script-test-prologue.c
>>> @@ -26,9 +26,11 @@ static void (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
>>> (void *) 6;
>>> SEC("func=null_lseek file->f_mode offset orig")
>>> -int bpf_func__null_lseek(void *ctx, int err, unsigned long f_mode,
>>> +int bpf_func__null_lseek(void *ctx, int err, unsigned long _f_mode,
>>> unsigned long offset, unsigned long orig)
>>> {
>>> + fmode_t f_mode = (fmode_t)_f_mode;
>>> +
>>> if (err)
>>> return 0;
>>> if (f_mode & FMODE_WRITE)
>>> diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
>>> index 6cdbee1..ce28993 100644
>>> --- a/tools/perf/util/bpf-prologue.c
>>> +++ b/tools/perf/util/bpf-prologue.c
>>> @@ -57,6 +57,46 @@ check_pos(struct bpf_insn_pos *pos)
>>> return 0;
>>> }
>>> +/*
>>> + * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
>>> + * Documentation/trace/kprobetrace.txt) to size field of BPF_LDX_MEM
>>> + * instruction (BPF_{B,H,W,DW}).
>>> + */
>>> +static int
>>> +argtype_to_ldx_size(const char *type)
>>> +{
>>> + int arg_size = type ? atoi(&type[1]) : 64;
>>> +
>>> + switch (arg_size) {
>>> + case 8:
>>> + return BPF_B;
>>> + case 16:
>>> + return BPF_H;
>>> + case 32:
>>> + return BPF_W;
>>> + case 64:
>>> + default:
>>> + return BPF_DW;
>>> + }
>>> +}
>>> +
>>> +static const char *
>>> +insn_sz_to_str(int insn_sz)
>>> +{
>>> + switch (insn_sz) {
>>> + case BPF_B:
>>> + return "BPF_B";
>>> + case BPF_H:
>>> + return "BPF_H";
>>> + case BPF_W:
>>> + return "BPF_W";
>>> + case BPF_DW:
>>> + return "BPF_DW";
>>> + default:
>>> + return "UNKNOWN";
>>> + }
>>> +}
>>> +
>>> /* Give it a shorter name */
>>> #define ins(i, p) append_insn((i), (p))
>>> @@ -257,9 +297,14 @@ gen_prologue_slowpath(struct bpf_insn_pos *pos,
>>> }
>>> /* Final pass: read to registers */
>>> - for (i = 0; i < nargs; i++)
>>> - ins(BPF_LDX_MEM(BPF_DW, BPF_PROLOGUE_START_ARG_REG + i,
>>> + for (i = 0; i < nargs; i++) {
>>> + int insn_sz = argtype_to_ldx_size(args[i].type);
int insn_sz = (args[i].ref) ? argtype_to_ldx_size(args[i].type) : BPF_DW;
>>> +
>>> + pr_debug("prologue: load arg %d, insn_sz is %s\n",
>>> + i, insn_sz_to_str(insn_sz));
>>> + ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
>>> BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
>>> + }
>>> ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
>>
>
--
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294