Re: [PATCH RFC bpf-next 5/5] bpf: Do not include the original insn in zext patchlet

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Thu, 2020-09-10 at 17:25 -0700, Alexei Starovoitov wrote:
> On Wed, Sep 9, 2020 at 4:37 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > If the original insn is a jump, then it is not subjected to branch
> > adjustment, which is incorrect. As discovered by Yauheni in
> 
> I think the problem is elsewhere.
> Something is wrong with zext logic.
> the branch insn should not have been marked as zext_dst.
> and in the line:
> zext_patch[0] = insn;
> this 'insn' should never be a branch.
> See insn_no_def().

Would it make sense to add a WARN_ON(insn_no_def(&insn)) there?


I believe the root cause is triggered by clear_caller_saved_regs().

This is our prog:

[     0]: BPF_JMP | BPF_CALL | BPF_K, BPF_REG_0, BPF_REG_1, 0x0, 0x1
[     1]: BPF_JMP | BPF_EXIT | BPF_K, BPF_REG_0, BPF_REG_0, 0x0, 0x0
[     2]: BPF_JMP | BPF_CALL | BPF_K, BPF_REG_0, BPF_REG_1, 0x0, 0x1
[     3]: BPF_JMP | BPF_EXIT | BPF_K, BPF_REG_0, BPF_REG_0, 0x0, 0x0
...

and env->insn_idx is 2. clear_caller_saved_regs() calls

	check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);

for register 0, and then inside check_reg_arg() we come to

	reg->subreg_def = rw64 ? DEF_NOT_SUBREG : env->insn_idx + 1;

where rw64 is false, because insn 2 is a BPF_PSEUDO_CALL. Having
non-zero subreg_def causes mark_insn_zext() to set zext_dst later on.

Maybe mark_reg_unknown() can do something to prevent this? My knee-jerk
reaction would be to set subreg_def to 0 there, but I'm not sure
whether this would be correct.