| Message ID | 20200909233439.3100292-6-iii@linux.ibm.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Do not include the original insn in zext patchlet | expand |
Hi, Ilya! Cool, thanks! Shouldn't the rnd patch be done the same way for completeness? Even if it is unlikely there to hit the problem. >>>>> On Thu, 10 Sep 2020 01:34:39 +0200, Ilya Leoshkevich wrote: > If the original insn is a jump, then it is not subjected to branch > adjustment, which is incorrect. As discovered by Yauheni in > https://lore.kernel.org/bpf/20200903140542.156624-1-yauheni.kaliuta@redhat.com/ > this causes `test_progs -t global_funcs` failures on s390. > Most likely, the current code includes the original insn in the > patchlet, because there was no infrastructure to insert new insns, only > to replace the existing ones. Now that bpf_patch_insns_data() can do > insertions, stop including the original insns in zext patchlets. > Fixes: a4b1d3c1ddf6 ("bpf: verifier: insert zero extension according > to analysis result") > Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > kernel/bpf/verifier.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 17c2e926e436..64a04953c631 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -9911,7 +9911,7 @@ static int opt_remove_nops(struct bpf_verifier_env *env) > static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > const union bpf_attr *attr) > { > - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4]; > + struct bpf_insn *patch, zext_patch, rnd_hi32_patch[4]; > struct bpf_insn_aux_data *aux = env->insn_aux_data; > int i, patch_len, delta = 0, len = env->prog->len; > struct bpf_insn *insns = env->prog->insnsi; > @@ -9919,13 +9919,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > bool rnd_hi32; > rnd_hi32 = attr->prog_flags & BPF_F_TEST_RND_HI32; > - zext_patch[1] = BPF_ZEXT_REG(0); > + zext_patch = BPF_ZEXT_REG(0); > rnd_hi32_patch[1] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_AX, 0); > rnd_hi32_patch[2] = BPF_ALU64_IMM(BPF_LSH, BPF_REG_AX, 32); > rnd_hi32_patch[3] = BPF_ALU64_REG(BPF_OR, 0, BPF_REG_AX); > for (i = 0; i < len; i++) { > int adj_idx = i + delta; > struct bpf_insn insn; > + int len_old = 1; > insn = insns[adj_idx]; > if (!aux[adj_idx].zext_dst) { > @@ -9968,20 +9969,21 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > if (!bpf_jit_needs_zext()) > continue; > - zext_patch[0] = insn; > - zext_patch[1].dst_reg = insn.dst_reg; > - zext_patch[1].src_reg = insn.dst_reg; > - patch = zext_patch; > - patch_len = 2; > + zext_patch.dst_reg = insn.dst_reg; > + zext_patch.src_reg = insn.dst_reg; > + patch = &zext_patch; > + patch_len = 1; > + adj_idx++; > + len_old = 0; > apply_patch_buffer: > - new_prog = bpf_patch_insns_data(env, adj_idx, 1, patch, > + new_prog = bpf_patch_insns_data(env, adj_idx, len_old, patch, > patch_len); > if (!new_prog) > return -ENOMEM; env-> prog = new_prog; > insns = new_prog->insnsi; > aux = env->insn_aux_data; > - delta += patch_len - 1; > + delta += patch_len - len_old; > } > return 0; > -- > 2.25.4
On Thu, 2020-09-10 at 09:59 +0300, Yauheni Kaliuta wrote: > Hi, Ilya! > > Cool, thanks! > > Shouldn't the rnd patch be done the same way for completeness? > Even if it is unlikely there to hit the problem. Ah, I haven't noticed that this pattern used elsewhere as well - I just checked and found 4 places. Let's wait and see whether the whole approach is acceptable, if yes, then I'll make patches that clean up these occurrences. > > > > > > > On Thu, 10 Sep 2020 01:34:39 +0200, Ilya > > > > > > Leoshkevich wrote: > > > If the original insn is a jump, then it is not subjected to branch > > adjustment, which is incorrect. As discovered by Yauheni in > > > > https://lore.kernel.org/bpf/20200903140542.156624-1-yauheni.kaliuta@redhat.com/ > > > this causes `test_progs -t global_funcs` failures on s390. > > > Most likely, the current code includes the original insn in the > > patchlet, because there was no infrastructure to insert new insns, > only > > to replace the existing ones. Now that bpf_patch_insns_data() can > do > > insertions, stop including the original insns in zext patchlets. > > > Fixes: a4b1d3c1ddf6 ("bpf: verifier: insert zero extension > according > > to analysis result") > > Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > kernel/bpf/verifier.c | 20 +++++++++++--------- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 17c2e926e436..64a04953c631 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -9911,7 +9911,7 @@ static int opt_remove_nops(struct > bpf_verifier_env *env) > > static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env > *env, > > const union bpf_attr *attr) > > { > > - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4]; > > + struct bpf_insn *patch, zext_patch, rnd_hi32_patch[4]; > > struct bpf_insn_aux_data *aux = env->insn_aux_data; > > int i, patch_len, delta = 0, len = env->prog->len; > > struct bpf_insn *insns = env->prog->insnsi; > > @@ -9919,13 +9919,14 @@ static int > opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > > bool rnd_hi32; > > > rnd_hi32 = attr->prog_flags & BPF_F_TEST_RND_HI32; > > - zext_patch[1] = BPF_ZEXT_REG(0); > > + zext_patch = BPF_ZEXT_REG(0); > > rnd_hi32_patch[1] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_AX, 0); > > rnd_hi32_patch[2] = BPF_ALU64_IMM(BPF_LSH, BPF_REG_AX, 32); > > rnd_hi32_patch[3] = BPF_ALU64_REG(BPF_OR, 0, BPF_REG_AX); > > for (i = 0; i < len; i++) { > > int adj_idx = i + delta; > > struct bpf_insn insn; > > + int len_old = 1; > > > insn = insns[adj_idx]; > > if (!aux[adj_idx].zext_dst) { > > @@ -9968,20 +9969,21 @@ static int > opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > > if (!bpf_jit_needs_zext()) > > continue; > > > - zext_patch[0] = insn; > > - zext_patch[1].dst_reg = insn.dst_reg; > > - zext_patch[1].src_reg = insn.dst_reg; > > - patch = zext_patch; > > - patch_len = 2; > > + zext_patch.dst_reg = insn.dst_reg; > > + zext_patch.src_reg = insn.dst_reg; > > + patch = &zext_patch; > > + patch_len = 1; > > + adj_idx++; > > + len_old = 0; > > apply_patch_buffer: > > - new_prog = bpf_patch_insns_data(env, adj_idx, 1, patch, > > + new_prog = bpf_patch_insns_data(env, adj_idx, len_old, > patch, > > patch_len); > > if (!new_prog) > > return -ENOMEM; > env-> prog = new_prog; > > insns = new_prog->insnsi; > > aux = env->insn_aux_data; > > - delta += patch_len - 1; > > + delta += patch_len - len_old; > > } > > > return 0; > > -- > > > 2.25.4 > >
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().
Hi, Alexei! >>>>> On Thu, 10 Sep 2020 17:25:43 -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(). Yes, it may be the case, as I mentioned in my analysis, but the patching itself looks much more clear with Ilya's changes.
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.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 17c2e926e436..64a04953c631 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -9911,7 +9911,7 @@ static int opt_remove_nops(struct bpf_verifier_env *env) static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, const union bpf_attr *attr) { - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4]; + struct bpf_insn *patch, zext_patch, rnd_hi32_patch[4]; struct bpf_insn_aux_data *aux = env->insn_aux_data; int i, patch_len, delta = 0, len = env->prog->len; struct bpf_insn *insns = env->prog->insnsi; @@ -9919,13 +9919,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, bool rnd_hi32; rnd_hi32 = attr->prog_flags & BPF_F_TEST_RND_HI32; - zext_patch[1] = BPF_ZEXT_REG(0); + zext_patch = BPF_ZEXT_REG(0); rnd_hi32_patch[1] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_AX, 0); rnd_hi32_patch[2] = BPF_ALU64_IMM(BPF_LSH, BPF_REG_AX, 32); rnd_hi32_patch[3] = BPF_ALU64_REG(BPF_OR, 0, BPF_REG_AX); for (i = 0; i < len; i++) { int adj_idx = i + delta; struct bpf_insn insn; + int len_old = 1; insn = insns[adj_idx]; if (!aux[adj_idx].zext_dst) { @@ -9968,20 +9969,21 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, if (!bpf_jit_needs_zext()) continue; - zext_patch[0] = insn; - zext_patch[1].dst_reg = insn.dst_reg; - zext_patch[1].src_reg = insn.dst_reg; - patch = zext_patch; - patch_len = 2; + zext_patch.dst_reg = insn.dst_reg; + zext_patch.src_reg = insn.dst_reg; + patch = &zext_patch; + patch_len = 1; + adj_idx++; + len_old = 0; apply_patch_buffer: - new_prog = bpf_patch_insns_data(env, adj_idx, 1, patch, + new_prog = bpf_patch_insns_data(env, adj_idx, len_old, patch, patch_len); if (!new_prog) return -ENOMEM; env->prog = new_prog; insns = new_prog->insnsi; aux = env->insn_aux_data; - delta += patch_len - 1; + delta += patch_len - len_old; } return 0;
If the original insn is a jump, then it is not subjected to branch adjustment, which is incorrect. As discovered by Yauheni in https://lore.kernel.org/bpf/20200903140542.156624-1-yauheni.kaliuta@redhat.com/ this causes `test_progs -t global_funcs` failures on s390. Most likely, the current code includes the original insn in the patchlet, because there was no infrastructure to insert new insns, only to replace the existing ones. Now that bpf_patch_insns_data() can do insertions, stop including the original insns in zext patchlets. Fixes: a4b1d3c1ddf6 ("bpf: verifier: insert zero extension according to analysis result") Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- kernel/bpf/verifier.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)