Re: [PATCH] bpf,x64: pad NOPs to make images converge more easily

On Fri, Dec 11, 2020 at 06:24:47PM -0800, Alexei Starovoitov wrote:

On Fri, Dec 11, 2020 at 1:13 PM Daniel Borkmann [off-list ref] wrote:

quoted

quoted

quoted

+                       }
  emit_jmp:
                         if (is_imm8(jmp_offset)) {
+                               if (jmp_padding)
+                                       cnt += emit_nops(&prog, INSN_SZ_DIFF - 2);

Could you describe all possible numbers of bytes in padding?
Is it 0, 2, 4 ?
Would be good to add warn_on_once to make sure the number
of nops is expected.

For the conditional jumps, it could be 0 or 4. As for nop jumps, it may be
0, 2, or 5. For the pure jumps, 0 or 3. Will add the warning in the next
version.

quoted

quoted

quoted

  struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
  {
         struct bpf_binary_header *header = NULL;

@@ -1981,6 +1997,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
         struct jit_context ctx = {};
         bool tmp_blinded = false;
         bool extra_pass = false;
+       bool padding = prog->padded;

can this ever be true on assignment? I.e., can the program be jitted twice?

Yes, progs can be passed into the JIT twice, see also jit_subprogs(). In one of
the earlier patches it would still potentially change the image size a second
time which would break subprogs aka bpf2bpf calls.

Right. I think memorized padded flag shouldn't be in sticky bits
of the prog structure.
It's only needed between the last pass and extra pass for bpf2bpf calls.
I think it would be cleaner to keep it in struct x64_jit_data *jit_data.

Okay, jit_data is surely a better place for the flag.

As others have said the selftests are must have.
Especially for bpf2bpf calls where one subprog is padded.

Will try to craft some test cases for this patch in v2.

Gary Lin