diff mbox series

[net-next,6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

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

Commit Message

Jakub Kicinski Oct. 19, 2017, 10:46 p.m. UTC 
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>
---
 tools/bpf/bpftool/prog.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

David Laight Oct. 20, 2017, 9:59 a.m. UTC | #1 
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
Daniel Borkmann Oct. 20, 2017, 10:56 a.m. UTC | #2 
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>
Quentin Monnet Oct. 20, 2017, 3:50 p.m. UTC | #3 
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
Daniel Borkmann Oct. 20, 2017, 4:12 p.m. UTC | #4 
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
Quentin Monnet Oct. 20, 2017, 4:41 p.m. UTC | #5 
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 mbox series

Patch

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++;
 	}
 }