Message ID | 20171019224626.31608-7-jakub.kicinski@netronome.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | tools: bpftool: add a "version" command, and fix several items | expand |
From: Jakub Kicinski > Sent: 19 October 2017 23:46 > The eBPF instruction permitting to load double words (8 bytes) into a > register need 8-byte long "immediate" field, and thus occupy twice the > space of other instructions. bpftool was aware of this and would > increment the instruction counter only once on meeting such instruction, > but it would only print the first four bytes of the immediate value to > load. Make it able to dump the whole 16 byte-long double instruction > instead (as would `llvm-objdump -d <program>`). Guess why most modern instruction sets use a 'load high' instruction to generate big constants... Interestingly, is there anything special in the rest of the second instruction in order to make it an identifiable no-op? ... > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 355c14325622..57edbea2fbe8 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -313,20 +313,29 @@ static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...) > static void dump_xlated(void *buf, unsigned int len, bool opcodes) > { > struct bpf_insn *insn = buf; > + bool double_insn = false; > unsigned int i; > > for (i = 0; i < len / sizeof(*insn); i++) { > + if (double_insn) { > + double_insn = false; > + continue; > + } Why not just: for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) { ... David
On 10/20/2017 12:46 AM, Jakub Kicinski wrote: > From: Quentin Monnet <quentin.monnet@netronome.com> > > The eBPF instruction permitting to load double words (8 bytes) into a > register need 8-byte long "immediate" field, and thus occupy twice the > space of other instructions. bpftool was aware of this and would > increment the instruction counter only once on meeting such instruction, > but it would only print the first four bytes of the immediate value to > load. Make it able to dump the whole 16 byte-long double instruction > instead (as would `llvm-objdump -d <program>`). > > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Hi David, On 20 October 2017 at 10:59, David Laight <David.Laight@aculab.com> wrote: > From: Jakub Kicinski >> Sent: 19 October 2017 23:46 >> The eBPF instruction permitting to load double words (8 bytes) into a >> register need 8-byte long "immediate" field, and thus occupy twice the >> space of other instructions. bpftool was aware of this and would >> increment the instruction counter only once on meeting such instruction, >> but it would only print the first four bytes of the immediate value to >> load. Make it able to dump the whole 16 byte-long double instruction >> instead (as would `llvm-objdump -d <program>`). > > Guess why most modern instruction sets use a 'load high' instruction > to generate big constants... > > Interestingly, is there anything special in the rest of the > second instruction in order to make it an identifiable no-op? The remaining four bytes are taken from the "immediate" field of the second instruction, which leaves the first four fields (offset, source and destination registers, and in particular opcode) unused. As far as I know, these fields remain at zero, and this makes it the only “instruction” to have a null code (although I am not sure this is a strict requirement, because I did not find the code in the verifier that would reject a program having a non-null opcode right after a "load double word immediate" instruction). > […] > Why not just: > for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) { > > ... > > David > Yes, we could use that instead, although I am not sure this makes the code more readable. So I do not believe this is worth a respin, tell me if you think otherwise. Thanks for the review! Quentin
On 10/20/2017 05:50 PM, Quentin Monnet wrote: [...] > The remaining four bytes are taken from the "immediate" field of the second > instruction, which leaves the first four fields (offset, source and destination > registers, and in particular opcode) unused. As far as I know, these fields > remain at zero, and this makes it the only “instruction” to have a null code > (although I am not sure this is a strict requirement, because I did not find > the code in the verifier that would reject a program having a non-null opcode > right after a "load double word immediate" instruction). It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part are rejected there, they have to be otherwise it's not extendable anymore from abi pov; check also 'test1* ld_imm64' in the verifier test cases. Cheers, Daniel
On 20 October 2017 at 17:12, Daniel Borkmann <daniel@iogearbox.net> wrote: > On 10/20/2017 05:50 PM, Quentin Monnet wrote: > [...] >> >> The remaining four bytes are taken from the "immediate" field of the >> second >> instruction, which leaves the first four fields (offset, source and >> destination >> registers, and in particular opcode) unused. As far as I know, these >> fields >> remain at zero, and this makes it the only “instruction” to have a null >> code >> (although I am not sure this is a strict requirement, because I did not >> find >> the code in the verifier that would reject a program having a non-null >> opcode >> right after a "load double word immediate" instruction). > > > It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part > are rejected there, they have to be otherwise it's not extendable anymore > from abi pov; check also 'test1* ld_imm64' in the verifier test cases. > > Cheers, > Daniel Indeed, thanks Daniel! Quentin
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 355c14325622..57edbea2fbe8 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -313,20 +313,29 @@ static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...) static void dump_xlated(void *buf, unsigned int len, bool opcodes) { struct bpf_insn *insn = buf; + bool double_insn = false; unsigned int i; for (i = 0; i < len / sizeof(*insn); i++) { + if (double_insn) { + double_insn = false; + continue; + } + + double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW); + printf("% 4d: ", i); print_bpf_insn(print_insn, NULL, insn + i, true); if (opcodes) { printf(" "); fprint_hex(stdout, insn + i, 8, " "); + if (double_insn && i < len - 1) { + printf(" "); + fprint_hex(stdout, insn + i + 1, 8, " "); + } printf("\n"); } - - if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) - i++; } }