Re: [RFC PATCH v1 08/14] bpf: Compute used callee saved registers for subprogs

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	 Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	 Martin KaFai Lau <martin.lau@kernel.org>,
	David Vernet <void@manifault.com>, Tejun Heo <tj@kernel.org>,
	 Raj Sahu <rjsu26@vt.edu>, Dan Williams <djwillia@vt.edu>,
	Rishabh Iyer <rishabh.iyer@epfl.ch>,
	 Sanidhya Kashyap <sanidhya.kashyap@epfl.ch>
Subject: Re: [RFC PATCH v1 08/14] bpf: Compute used callee saved registers for subprogs
Date: Fri, 16 Feb 2024 23:02:53 +0100	[thread overview]
Message-ID: <CAP01T74yVyxLcW13+gEUs4A7qe4YCz84x7jnw78o9P5Xjtt12A@mail.gmail.com> (raw)
In-Reply-To: <a99c123671efdf4e795d883cb3b0c67ced3884c1.camel@gmail.com>

On Thu, 15 Feb 2024 at 23:12, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-02-01 at 04:21 +0000, Kumar Kartikeya Dwivedi wrote:
>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 942243cba9f1..aeaf97b0a749 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -2942,6 +2942,15 @@ static int check_subprogs(struct bpf_verifier_env *env)
> >                   insn[i].src_reg == 0 &&
> >                   insn[i].imm == BPF_FUNC_tail_call)
> >                       subprog[cur_subprog].has_tail_call = true;
> > +             /* Collect callee regs used in the subprog. */
> > +             if (insn[i].dst_reg == BPF_REG_6 || insn[i].src_reg == BPF_REG_6)
> > +                     subprog[cur_subprog].callee_regs_used[0] = true;
> > +             if (insn[i].dst_reg == BPF_REG_7 || insn[i].src_reg == BPF_REG_7)
> > +                     subprog[cur_subprog].callee_regs_used[1] = true;
> > +             if (insn[i].dst_reg == BPF_REG_8 || insn[i].src_reg == BPF_REG_8)
> > +                     subprog[cur_subprog].callee_regs_used[2] = true;
> > +             if (insn[i].dst_reg == BPF_REG_9 || insn[i].src_reg == BPF_REG_9)
> > +                     subprog[cur_subprog].callee_regs_used[3] = true;
>
> Nit: Maybe move bpf_jit_comp.c:detect_reg_usage() to some place available to
>      both verifier and jit? Just to keep all related code in one place.
>      E.g. technically nothing prevents x86 jit to do this detection in a more
>      precise manner as a "fixed point" computation.
>

Hm, I remember I did this and then decided against it for some reason,
but I can't remember now.
I will make this change though, if I remember why I didn't go ahead
with it, I will reply again.

Also, what did you mean by the final sentence?