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

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

On Fri, 2020-09-11 at 14:58 +0200, Ilya Leoshkevich wrote:
> 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.

Another possible fix (inspired by helper function call handling) is:

--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4751,6 +4751,7 @@ static int check_func_call(struct
bpf_verifier_env *env, struct bpf_insn *insn,
 
                        /* All global functions return SCALAR_VALUE */
                        mark_reg_unknown(env, caller->regs, BPF_REG_0);
+                       caller->regs[BPF_REG_0].subreg_def =
DEF_NOT_SUBREG;
 
                        /* continue with next insn after call */
                        return 0;

This relies on global functions always returning 64-bit values, which
I believe should always be the case.

If this sounds reasonable, I can send a proper patch.