| Message ID | 20210907222339.4130924-14-johan.almbladh@anyfinetworks.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v2,01/13] bpf/tests: Allow different number of runs per test case | expand |
On Wed, 2021-09-08 at 00:23 +0200, Johan Almbladh wrote: > This patch adds a tail call limit test where the program also emits > a BPF_CALL to an external function prior to the tail call. Mainly > testing that JITed programs preserve its internal register state, for > example tail call count, across such external calls. > > Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > --- > lib/test_bpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 48 insertions(+), 3 deletions(-) > > diff --git a/lib/test_bpf.c b/lib/test_bpf.c > index 7475abfd2186..6e45b4da9841 100644 > --- a/lib/test_bpf.c > +++ b/lib/test_bpf.c > @@ -12259,6 +12259,20 @@ static struct tail_call_test tail_call_tests[] > = { > }, > .result = MAX_TAIL_CALL_CNT + 1, > }, > + { > + "Tail call count preserved across function calls", > + .insns = { > + BPF_ALU64_IMM(BPF_ADD, R1, 1), > + BPF_STX_MEM(BPF_DW, R10, R1, -8), > + BPF_CALL_REL(0), > + BPF_LDX_MEM(BPF_DW, R1, R10, -8), > + BPF_ALU32_REG(BPF_MOV, R0, R1), > + TAIL_CALL(0), > + BPF_EXIT_INSN(), > + }, > + .stack_depth = 8, > + .result = MAX_TAIL_CALL_CNT + 1, > + }, > { > "Tail call error path, NULL target", > .insns = { There seems to be a problem with BPF_CALL_REL(0) on s390, since it assumes that test_bpf_func and __bpf_call_base are within +-2G of each other, which is not (yet) the case. I can't think of a good fix, so how about something like this? --- a/lib/test_bpf.c +++ b/lib/test_bpf.c @@ -12257,6 +12257,7 @@ static struct tail_call_test tail_call_tests[] = { }, .result = MAX_TAIL_CALL_CNT + 1, }, +#ifndef __s390__ { "Tail call count preserved across function calls", .insns = { @@ -12271,6 +12272,7 @@ static struct tail_call_test tail_call_tests[] = { .stack_depth = 8, .result = MAX_TAIL_CALL_CNT + 1, }, +#endif { "Tail call error path, NULL target", .insns = { [...]
On Wed, Sep 8, 2021 at 12:10 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Wed, 2021-09-08 at 00:23 +0200, Johan Almbladh wrote: > > This patch adds a tail call limit test where the program also emits > > a BPF_CALL to an external function prior to the tail call. Mainly > > testing that JITed programs preserve its internal register state, for > > example tail call count, across such external calls. > > > > Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > > --- > > lib/test_bpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 48 insertions(+), 3 deletions(-) > > > > diff --git a/lib/test_bpf.c b/lib/test_bpf.c > > index 7475abfd2186..6e45b4da9841 100644 > > --- a/lib/test_bpf.c > > +++ b/lib/test_bpf.c > > @@ -12259,6 +12259,20 @@ static struct tail_call_test tail_call_tests[] > > = { > > }, > > .result = MAX_TAIL_CALL_CNT + 1, > > }, > > + { > > + "Tail call count preserved across function calls", > > + .insns = { > > + BPF_ALU64_IMM(BPF_ADD, R1, 1), > > + BPF_STX_MEM(BPF_DW, R10, R1, -8), > > + BPF_CALL_REL(0), > > + BPF_LDX_MEM(BPF_DW, R1, R10, -8), > > + BPF_ALU32_REG(BPF_MOV, R0, R1), > > + TAIL_CALL(0), > > + BPF_EXIT_INSN(), > > + }, > > + .stack_depth = 8, > > + .result = MAX_TAIL_CALL_CNT + 1, > > + }, > > { > > "Tail call error path, NULL target", > > .insns = { > > There seems to be a problem with BPF_CALL_REL(0) on s390, since it > assumes that test_bpf_func and __bpf_call_base are within +-2G of > each other, which is not (yet) the case. The idea with this test is to mess up a JITed program's internal state if it does not properly save/restore those regs. I would like to keep the test in some form, but I do see the problem here. Another option could perhaps be to skip this test at runtime if the computed offset is outside +-2G. If the offset is greater than that it does not fit into the 32-bit BPF immediate field, and must therefore be skipped. This would work for other archs too. Yet another solution would be call one or several bpf helpers instead. As I understand it, they should always be located within this range, otherwise they would not be callable from a BPF program. The reason I did not do this was because I found helpers that don't require any context to be too simple. Ideally one would want to call something that uses pretty much all available caller-saved CPU registers. I figured snprintf would be complex/nasty enough for this purpose. > > I can't think of a good fix, so how about something like this? > > --- a/lib/test_bpf.c > +++ b/lib/test_bpf.c > @@ -12257,6 +12257,7 @@ static struct tail_call_test tail_call_tests[] > = { > }, > .result = MAX_TAIL_CALL_CNT + 1, > }, > +#ifndef __s390__ > { > "Tail call count preserved across function calls", > .insns = { > @@ -12271,6 +12272,7 @@ static struct tail_call_test tail_call_tests[] > = { > .stack_depth = 8, > .result = MAX_TAIL_CALL_CNT + 1, > }, > +#endif > { > "Tail call error path, NULL target", > .insns = { > > [...] >
On 9/8/21 12:53 PM, Johan Almbladh wrote: > On Wed, Sep 8, 2021 at 12:10 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: >> On Wed, 2021-09-08 at 00:23 +0200, Johan Almbladh wrote: >>> This patch adds a tail call limit test where the program also emits >>> a BPF_CALL to an external function prior to the tail call. Mainly >>> testing that JITed programs preserve its internal register state, for >>> example tail call count, across such external calls. >>> >>> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> >>> --- >>> lib/test_bpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 48 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/test_bpf.c b/lib/test_bpf.c >>> index 7475abfd2186..6e45b4da9841 100644 >>> --- a/lib/test_bpf.c >>> +++ b/lib/test_bpf.c >>> @@ -12259,6 +12259,20 @@ static struct tail_call_test tail_call_tests[] >>> = { >>> }, >>> .result = MAX_TAIL_CALL_CNT + 1, >>> }, >>> + { >>> + "Tail call count preserved across function calls", >>> + .insns = { >>> + BPF_ALU64_IMM(BPF_ADD, R1, 1), >>> + BPF_STX_MEM(BPF_DW, R10, R1, -8), >>> + BPF_CALL_REL(0), >>> + BPF_LDX_MEM(BPF_DW, R1, R10, -8), >>> + BPF_ALU32_REG(BPF_MOV, R0, R1), >>> + TAIL_CALL(0), >>> + BPF_EXIT_INSN(), >>> + }, >>> + .stack_depth = 8, >>> + .result = MAX_TAIL_CALL_CNT + 1, >>> + }, >>> { >>> "Tail call error path, NULL target", >>> .insns = { >> >> There seems to be a problem with BPF_CALL_REL(0) on s390, since it >> assumes that test_bpf_func and __bpf_call_base are within +-2G of >> each other, which is not (yet) the case. > > The idea with this test is to mess up a JITed program's internal state > if it does not properly save/restore those regs. I would like to keep > the test in some form, but I do see the problem here. > > Another option could perhaps be to skip this test at runtime if the > computed offset is outside +-2G. If the offset is greater than that it > does not fit into the 32-bit BPF immediate field, and must therefore > be skipped. This would work for other archs too. Sounds reasonable as a work-around/to move forward. > Yet another solution would be call one or several bpf helpers instead. > As I understand it, they should always be located within this range, > otherwise they would not be callable from a BPF program. The reason I > did not do this was because I found helpers that don't require any > context to be too simple. Ideally one would want to call something > that uses pretty much all available caller-saved CPU registers. I > figured snprintf would be complex/nasty enough for this purpose. Potentially bpf_csum_diff() could also be a candidate, and fairly straight forward to set up from raw asm. >> I can't think of a good fix, so how about something like this? >> >> --- a/lib/test_bpf.c >> +++ b/lib/test_bpf.c >> @@ -12257,6 +12257,7 @@ static struct tail_call_test tail_call_tests[] >> = { >> }, >> .result = MAX_TAIL_CALL_CNT + 1, >> }, >> +#ifndef __s390__ >> { >> "Tail call count preserved across function calls", >> .insns = { >> @@ -12271,6 +12272,7 @@ static struct tail_call_test tail_call_tests[] >> = { >> .stack_depth = 8, >> .result = MAX_TAIL_CALL_CNT + 1, >> }, >> +#endif >> { >> "Tail call error path, NULL target", >> .insns = { >> >> [...] >>
On Wed, Sep 8, 2021 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 9/8/21 12:53 PM, Johan Almbladh wrote: > > On Wed, Sep 8, 2021 at 12:10 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > >> On Wed, 2021-09-08 at 00:23 +0200, Johan Almbladh wrote: > >>> This patch adds a tail call limit test where the program also emits > >>> a BPF_CALL to an external function prior to the tail call. Mainly > >>> testing that JITed programs preserve its internal register state, for > >>> example tail call count, across such external calls. > >>> > >>> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > >>> --- > >>> lib/test_bpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- > >>> 1 file changed, 48 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/lib/test_bpf.c b/lib/test_bpf.c > >>> index 7475abfd2186..6e45b4da9841 100644 > >>> --- a/lib/test_bpf.c > >>> +++ b/lib/test_bpf.c > >>> @@ -12259,6 +12259,20 @@ static struct tail_call_test tail_call_tests[] > >>> = { > >>> }, > >>> .result = MAX_TAIL_CALL_CNT + 1, > >>> }, > >>> + { > >>> + "Tail call count preserved across function calls", > >>> + .insns = { > >>> + BPF_ALU64_IMM(BPF_ADD, R1, 1), > >>> + BPF_STX_MEM(BPF_DW, R10, R1, -8), > >>> + BPF_CALL_REL(0), > >>> + BPF_LDX_MEM(BPF_DW, R1, R10, -8), > >>> + BPF_ALU32_REG(BPF_MOV, R0, R1), > >>> + TAIL_CALL(0), > >>> + BPF_EXIT_INSN(), > >>> + }, > >>> + .stack_depth = 8, > >>> + .result = MAX_TAIL_CALL_CNT + 1, > >>> + }, > >>> { > >>> "Tail call error path, NULL target", > >>> .insns = { > >> > >> There seems to be a problem with BPF_CALL_REL(0) on s390, since it > >> assumes that test_bpf_func and __bpf_call_base are within +-2G of > >> each other, which is not (yet) the case. > > > > The idea with this test is to mess up a JITed program's internal state > > if it does not properly save/restore those regs. I would like to keep > > the test in some form, but I do see the problem here. > > > > Another option could perhaps be to skip this test at runtime if the > > computed offset is outside +-2G. If the offset is greater than that it > > does not fit into the 32-bit BPF immediate field, and must therefore > > be skipped. This would work for other archs too. > > Sounds reasonable as a work-around/to move forward. I'll do this and prepare a v3 then. > > > Yet another solution would be call one or several bpf helpers instead. > > As I understand it, they should always be located within this range, > > otherwise they would not be callable from a BPF program. The reason I > > did not do this was because I found helpers that don't require any > > context to be too simple. Ideally one would want to call something > > that uses pretty much all available caller-saved CPU registers. I > > figured snprintf would be complex/nasty enough for this purpose. > > Potentially bpf_csum_diff() could also be a candidate, and fairly > straight forward to set up from raw asm. Thanks, I will take a look at it. > > >> I can't think of a good fix, so how about something like this? > >> > >> --- a/lib/test_bpf.c > >> +++ b/lib/test_bpf.c > >> @@ -12257,6 +12257,7 @@ static struct tail_call_test tail_call_tests[] > >> = { > >> }, > >> .result = MAX_TAIL_CALL_CNT + 1, > >> }, > >> +#ifndef __s390__ > >> { > >> "Tail call count preserved across function calls", > >> .insns = { > >> @@ -12271,6 +12272,7 @@ static struct tail_call_test tail_call_tests[] > >> = { > >> .stack_depth = 8, > >> .result = MAX_TAIL_CALL_CNT + 1, > >> }, > >> +#endif > >> { > >> "Tail call error path, NULL target", > >> .insns = { > >> > >> [...] > >> >
diff --git a/lib/test_bpf.c b/lib/test_bpf.c index 7475abfd2186..6e45b4da9841 100644 --- a/lib/test_bpf.c +++ b/lib/test_bpf.c @@ -12259,6 +12259,20 @@ static struct tail_call_test tail_call_tests[] = { }, .result = MAX_TAIL_CALL_CNT + 1, }, + { + "Tail call count preserved across function calls", + .insns = { + BPF_ALU64_IMM(BPF_ADD, R1, 1), + BPF_STX_MEM(BPF_DW, R10, R1, -8), + BPF_CALL_REL(0), + BPF_LDX_MEM(BPF_DW, R1, R10, -8), + BPF_ALU32_REG(BPF_MOV, R0, R1), + TAIL_CALL(0), + BPF_EXIT_INSN(), + }, + .stack_depth = 8, + .result = MAX_TAIL_CALL_CNT + 1, + }, { "Tail call error path, NULL target", .insns = { @@ -12281,6 +12295,29 @@ static struct tail_call_test tail_call_tests[] = { }, }; +/* + * A test function to be called from a BPF program, clobbering a lot of + * CPU registers in the process. A JITed BPF program calling this function + * must save and restore any caller-saved registers it uses for internal + * state, for example the current tail call count. + */ +BPF_CALL_1(test_bpf_func, u64, arg) +{ + char buf[64]; + long a = 0; + long b = 1; + long c = 2; + long d = 3; + long e = 4; + long f = 5; + long g = 6; + long h = 7; + + return snprintf(buf, sizeof(buf), + "%ld %lu %lx %ld %lu %lx %ld %lu %x", + a, b, c, d, e, f, g, h, (int)arg); +} + static void __init destroy_tail_call_tests(struct bpf_array *progs) { int i; @@ -12334,16 +12371,17 @@ static __init int prepare_tail_call_tests(struct bpf_array **pprogs) for (i = 0; i < len; i++) { struct bpf_insn *insn = &fp->insnsi[i]; - if (insn->imm != TAIL_CALL_MARKER) - continue; - switch (insn->code) { case BPF_LD | BPF_DW | BPF_IMM: + if (insn->imm != TAIL_CALL_MARKER) + break; insn[0].imm = (u32)(long)progs; insn[1].imm = ((u64)(long)progs) >> 32; break; case BPF_ALU | BPF_MOV | BPF_K: + if (insn->imm != TAIL_CALL_MARKER) + break; if (insn->off == TAIL_CALL_NULL) insn->imm = ntests; else if (insn->off == TAIL_CALL_INVALID) @@ -12351,6 +12389,13 @@ static __init int prepare_tail_call_tests(struct bpf_array **pprogs) else insn->imm = which + insn->off; insn->off = 0; + break; + + case BPF_JMP | BPF_CALL: + if (insn->src_reg != BPF_PSEUDO_CALL) + break; + *insn = BPF_EMIT_CALL(test_bpf_func); + break; } }
This patch adds a tail call limit test where the program also emits a BPF_CALL to an external function prior to the tail call. Mainly testing that JITed programs preserve its internal register state, for example tail call count, across such external calls. Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> --- lib/test_bpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-)