Re: [PATCH bpf-next v9 04/23] bpf: Keep a call site's landing pad in insn_aux_data, add lookups

[Yonghong Song <yonghong.song@linux.dev>]

On 10/8/26 10:01 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] bpf: adjust_insn_aux_data() fails to shift cleanup_pad for BPF_PATCH_MOVE_TARGET
> --
>
> commit 1bdc64eca7dc9ba53a20b6b6c4d0e4448bdd4aa8
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Keep a call site's landing pad in insn_aux_data, add lookups
>
> A call site's landing pad, if any, is kept in insn_aux_data as
> cleanup_pad, so the three places that move instructions --
> bpf_patch_insn_data(), verifier_remove_insns() and bpf_opt_remove_nops()
> -- keep it in step. Add lookups for it and for calls to bpf_unwind() and
> bpf_unwind_resume(); their users come in later patches.
>
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> index 206cc9a614a95..64baee1a37e60 100644
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
>> @@ -244,11 +244,18 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>>   			data[i].non_stack_access =
>>   				data[off + cnt - 1].non_stack_access;
>>   			data[off + cnt - 1].non_stack_access = false;
>> +			data[i].cleanup_pad = data[off + cnt - 1].cleanup_pad;
>> +			data[off + cnt - 1].cleanup_pad = 0;
>>   		} else if (bpf_is_mem_insn(insn + i)) {
>>   			data[i].non_stack_access = true;
>>   		}
>>   	}
>>   
>> +	if (env->cleanup_info_cnt)
>> +		for (i = 0; i < prog_len; i++)
>> +			if (data[i].cleanup_pad > off + 1)
>> +				data[i].cleanup_pad += cnt - 1;
> [Severity: Medium]
> Does this logic fail to adjust the landing pad when mode is
> BPF_PATCH_MOVE_TARGET?
>
> By unconditionally using off + 1, this appears to assume
> BPF_PATCH_KEEP_TARGET semantics.
>
> If bpf_patch_insn_data() is called with BPF_PATCH_MOVE_TARGET and the landing
> pad is located exactly at the patched instruction, data[i].cleanup_pad would
> be equal to off + 1 (the 1-based index). The condition
> data[i].cleanup_pad > off + 1 evaluates to false, meaning the landing pad
> index is not shifted.
>
> As a result, the landing pad remains pointing at the newly inserted
> instructions instead of following the original instruction shifted to
> off + cnt - 1.
>
> Could this cause the unwinder to jump to the wrong instruction offset,
> potentially leading to state corruption during an exception unwind?

This won't happen. The below is v7 reply to sashiko. v8 does not pop
up this issue. and now v9 it comes back.

====

Okay, commit "bpf: Retarget indirect jump targets across prologue prepends"
introduced BPF_PATCH_MOVE_TARGET and merged last Friday. It solved three
cases for ops->gen_epilogue, ops->gen_prologue || env->seen_direct_write, and
stack slots for subprogs. ops->gen_epilogue has been rejected in patch 5.
We cannot allow ops->gen_epilogue since it may silently exit.
For other cases in "bpf: Retarget indirect jump targets across prologue prepends",
The above commit should already handle this.

For the other two, cleanup_pad == off + 1 cannot happen, because MOVE only
patches an entry insn and a landing pad cannot start at insn 0: the entry is
always walked outside a pad first, so reaching it again from an unwind
fails bpf_exc_check_insn() with "insn %u runs both inside and outside a
landing pad". Pads after off are shifted by the existing `> off + 1`, and a
covered call at off moves with its insn_aux_data.

====