Re: [PATCH bpf-next v4 05/13] bpf: Account R2 of register-pair returns in live register analysis
Eduard Zingerman <[email protected]>
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On Mon, 2026-08-10 at 17:09 -0700, Yonghong Song wrote: > A BPF_EXIT of a subprogram returning a value larger than 8 bytes (a > struct/union or an __int128) reads R2 as well as R0, since the second half > of the return value is passed back in R2. compute_insn_live_regs() only > marked R0 used at exit, so a callee's R2 could be considered dead and > cleaned from checkpointed states, which would allow unsound state pruning. > > Mark R2 as read at the BPF_EXIT of a subprogram that does return a register > pair. bpf_compute_live_registers() walks the instructions in order and > env->subprog_info[] is sorted by subprogram start, so the containing > subprogram is tracked with a running index and its return convention is > queried once per subprogram through bpf_ret_reg_pair(). > > Marking R2 at every exit instead would be simpler, but R2 would then stay > live backwards across any call that is not followed by a write to R2, which > is nearly every program, and would needlessly hurt state pruning. > > Signed-off-by: Yonghong Song <[email protected]> > --- Acked-by: Eduard Zingerman <[email protected]> > kernel/bpf/liveness.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > index ef9a5a922887..451edd74fa6f 100644 > --- a/kernel/bpf/liveness.c > +++ b/kernel/bpf/liveness.c > @@ -2062,10 +2062,15 @@ static inline u32 mask_widen(u32 m) { return m | (m << 16); } > static inline u16 mask_lo(u32 m) { return (u16)m; } > static inline u16 mask_hi(u32 m) { return (u16)(m >> 16); } > > -/* Compute info->{use,def} fields for the instruction */ > +/* > + * Compute info->{use,def} fields for the instruction. @ret_reg_pair tells > + * whether the subprogram containing @insn returns its value in the R0:R2 > + * register pair, which matters for BPF_EXIT. > + */ Nit: this comment change is unnecessary. > static void compute_insn_live_regs(struct bpf_verifier_env *env, > struct bpf_insn *insn, > - struct insn_live_regs *info) > + struct insn_live_regs *info, > + bool ret_reg_pair) > { > struct bpf_call_summary cs; > const u8 class = BPF_CLASS(insn->code); > @@ -2196,7 +2201,7 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env, > break; > case BPF_EXIT: > def = 0; > - use = r0; > + use = ret_reg_pair ? (r0 | reg64_mask(BPF_REG_2)) : r0; Nit: let's add an r2 variable. > break; > case BPF_CALL: > def = ALL_CALLER_SAVED_REGS; ...