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 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
> 
>