On Thu, 2020-09-10 at 17:25 -0700, Alexei Starovoitov wrote: > On Wed, Sep 9, 2020 at 4:37 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> > 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.
