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

[Yauheni Kaliuta <yauheni.kaliuta@redhat.com>]

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


-- 
WBR,
Yauheni Kaliuta