On 11/23/19 7:18 AM, Alexei Starovoitov wrote:
On Fri, Nov 22, 2019 at 09:00:35PM -0800, Andrii Nakryiko wrote:
On Fri, Nov 22, 2019 at 6:28 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
On Fri, Nov 22, 2019 at 3:25 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
+ case BPF_MOD_CALL_TO_NOP:
+ case BPF_MOD_JUMP_TO_NOP:
+ if (old_addr && !new_addr) {
+ memcpy(new_insn, nop_insn, X86_PATCH_SIZE);
+
+ prog = old_insn;
+ ret = emit_patch_fn(&prog, old_addr, ip);
+ if (ret)
+ return ret;
+ break;
+ }
+ return -ENXIO;
+ default:
There is this redundancy between BPF_MOD_xxx enums and
old_addr+new_addr (both encode what kind of transition it is), which
leads to this cumbersome logic. Would it be simpler to have
old_addr/new_addr determine whether it's X-to-NOP, NOP-to-Y, or X-to-Y
transition, while separate bool or simple BPF_MOD_CALL/BPF_MOD_JUMP
enum determining whether it's a call or a jump that we want to update.
Seems like that should be a simpler interface overall and cleaner
implementation?
Right we can probably simplify it further, I kept preserving the original
switch from Alexei's code where my assumption was that having the transition
explicitly spelled out was preferred in here and then based on that doing
the sanity checks to make sure we don't get bad input from any call-site
since we're modifying kernel text, e.g. in the bpf_trampoline_update() as
one example the BPF_MOD_* is a fixed constant input there.
I guess we can try adding one more argument
bpf_arch_text_poke(ip, BPF_MOD_NOP, old_addr, BPF_MOD_INTO_CALL, new_addr);
I was thinking along the lines of:
bpf_arch_text_poke(ip, BPF_MOD_CALL (or BPF_MOD_JMP), old_addr, new_addr);
old_addr/new_addr being possibly NULL determine NOP/not-a-NOP.
I see. Something like:
if (BPF_MOD_CALL) {
if (old_addr)
memcmp(ip, old_call_insn);
else
memcmp(ip, nop_insn);
} else if (BPF_MOD_JMP) {
if (old_addr)
memcmp(ip, old_jmp_insn);
else
memcmp(ip, nop_insn);
}
I guess that can work.
Ok, will see to come up with a clean simplification in the evening.
Thanks,
Daniel
