Re: [PATCH bpf-next] bpf: Clear subreg_def for global function return values

From: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	bpf@vger.kernel.org, Heiko Carstens <heiko.carstens@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>
Subject: Re: [PATCH bpf-next] bpf: Clear subreg_def for global function return values
Date: Fri, 12 Feb 2021 11:31:19 +0200	[thread overview]
Message-ID: <xunytuqhtyxk.fsf@redhat.com> (raw)
In-Reply-To: <20210212040408.90109-1-iii@linux.ibm.com> (Ilya Leoshkevich's message of "Fri, 12 Feb 2021 05:04:08 +0100")

Hi, Ilya!

>>>>> On Fri, 12 Feb 2021 05:04:08 +0100, Ilya Leoshkevich  wrote:

 > test_global_func4 fails on s390 as reported by Yauheni in [1].
 > The immediate problem is that the zext code includes the instruction,
 > whose result needs to be zero-extended, into the zero-extension
 > patchlet, and if this instruction happens to be a branch, then its
 > delta is not adjusted. As a result, the verifier rejects the program
 > later.

Thank you for addressing that!

 > However, according to [2], as far as the verifier's algorithm is
 > concerned and as specified by the insn_no_def() function, branching
 > insns do not define anything. This includes call insns, even though
 > one might argue that they define %r0.

I still think that the patching code should be fixed as well,
even if it's a separate issue.

But I got the attitude.

 > This means that the real problem is that zero extension kicks in at
 > all. This happens because clear_caller_saved_regs() sets BPF_REG_0's
 > subreg_def after global function calls. This can be fixed in many
 > ways; this patch mimics what helper function call handling already
 > does.

 > [1] https://lore.kernel.org/bpf/20200903140542.156624-1-yauheni.kaliuta@redhat.com/
 > [2]
 > https://lore.kernel.org/bpf/CAADnVQ+2RPKcftZw8d+B1UwB35cpBhpF5u3OocNh90D9pETPwg@mail.gmail.com/

 > Fixes: 51c39bb1d5d1 ("bpf: Introduce function-by-function verification")
 > Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
 > ---
 >  kernel/bpf/verifier.c | 3 ++-
 >  1 file changed, 2 insertions(+), 1 deletion(-)

 > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
 > index beae700bb56e..183fae996ad0 100644
 > --- a/kernel/bpf/verifier.c
 > +++ b/kernel/bpf/verifier.c
 > @@ -5211,8 +5211,9 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 >  					subprog);
 >  			clear_caller_saved_regs(env, caller->regs);

 > -			/* All global functions return SCALAR_VALUE */
 > +			/* All global functions return a 64-bit 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;
 > -- 

 > 2.29.2

-- 
WBR,
Yauheni Kaliuta