Re: [PATCH bpf-next v4 05/13] bpf: Account R2 of register-pair returns in live register analysis

On 8/12/26 2:21 PM, Eduard Zingerman wrote:
> 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.

Okay, will revert to the original comment.

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

Good point. Will do.

>
>>   			break;
>>   		case BPF_CALL:
>>   			def = ALL_CALLER_SAVED_REGS;
> ...