Re: [PATCH bpf-next 1/2] bpf, x86: Normalize narrow kfunc returns on IA32

> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index 852baf2e4db4d..017cc9e7e4e2b 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c

[ ... ]

> @@ -1620,13 +1620,25 @@ static int emit_kfunc_call(const struct bpf_prog *bpf_prog, u8 *end_addr,
>  		/* add esp,"bytes_in_stack" */
>  		end_addr -= 3;
>
> -	/* mov dword ptr [ebp+off],edx */
> -	if (fm->ret_size > sizeof(u32))
> -		end_addr -= 3;
> -
> -	/* mov dword ptr [ebp+off],eax */
> -	if (fm->ret_size)
> +	switch (fm->ret_size) {
> +	case 0:
> +		break;
> +	case sizeof(u8):
> +	case sizeof(u16):
> +		/* mov[s|z]x eax,[a]x */
>  		end_addr -= 3;
> +		fallthrough;
> +	case sizeof(u32):
> +		/* cdq or xor edx,edx */
> +		end_addr -= fm->ret_flags & BTF_FMODEL_SIGNED_ARG ? 1 : 2;
> +		fallthrough;
> +	case sizeof(u64):
> +		/* Store EDX:EAX in R0. */
> +		end_addr -= 6;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

The comment above this diff hunk reads:

 * address by figuring out how many jit-insn is generated between
 * the call (0xE8) and the end_addr:
 *      - 0-1 jit-insn (3 bytes each) to restore the esp pointer if there
 *        is arg pushed to the stack.
 *      - 0-2 jit-insns (3 bytes each) to handle the return value.

Should this comment be updated? The new code can emit 0-4 instructions
with varying lengths (1, 2, 3 or 6 bytes) to extend and store the
return value, not just "0-2 jit-insns (3 bytes each)".

>
>  	jmp_offset = (u8 *)__bpf_call_base + insn->imm - end_addr;
>  	if (!is_simm32(jmp_offset)) {
> @@ -1637,15 +1649,38 @@ static int emit_kfunc_call(const struct bpf_prog *bpf_prog, u8 *end_addr,
>
>  	EMIT1_off32(0xE8, jmp_offset);
>
> -	if (fm->ret_size)
> +	switch (fm->ret_size) {
> +	case sizeof(u8):
> +		/* mov[s|z]x eax,al */
> +		EMIT3(0x0F, fm->ret_flags & BTF_FMODEL_SIGNED_ARG ? 0xBE : 0xB6,
> +		      add_2reg(0xC0, IA32_EAX, IA32_EAX));
> +		break;
> +	case sizeof(u16):
> +		/* mov[s|z]x eax,ax */
> +		EMIT3(0x0F, fm->ret_flags & BTF_FMODEL_SIGNED_ARG ? 0xBF : 0xB7,
> +		      add_2reg(0xC0, IA32_EAX, IA32_EAX));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (fm->ret_size && fm->ret_size <= sizeof(u32)) {
> +		if (fm->ret_flags & BTF_FMODEL_SIGNED_ARG)
> +			/* cdq */
> +			EMIT1(0x99);
> +		else
> +			/* xor edx,edx */
> +			EMIT2(0x31, add_2reg(0xC0, IA32_EDX, IA32_EDX));
> +	}

Does sign-extending narrow signed returns match the documented kfunc
return contract? In kernel/bpf/fixups.c the comment says:

    /* Zero-extension is done by the caller. */
    if (bpf_pseudo_kfunc_call(&insn))
            continue;

This exempts kfunc calls from the usual zero-extension path because
the JIT is responsible for R0's high half. The verifier's
__mark_btf_func_reg_size() marks narrow returns as subregs
specifically to invoke the zero-extension machinery.

For a concrete example with bpf_strcmp() which returns signed int,
if it returns -1:
  - x86-64 leaves R0 = 0x00000000ffffffff (32-bit write zeros upper half)
  - This patch makes x86-32 produce R0 = 0xffffffffffffffff via cdq

The same BPF program would see different R0 values depending on the
architecture. Is this the intended behavior, or should narrow kfunc
returns be zero-extended unconditionally to match the x86-64 path and
the fixups.c contract?

> +
> +	if (fm->ret_size) {
>  		/* mov dword ptr [ebp+off],eax */
>  		EMIT3(0x89, add_2reg(0x40, IA32_EBP, IA32_EAX),
>  		      STACK_VAR(bpf2ia32[BPF_REG_0][0]));
> -
> -	if (fm->ret_size > sizeof(u32))
>  		/* mov dword ptr [ebp+off],edx */
>  		EMIT3(0x89, add_2reg(0x40, IA32_EBP, IA32_EDX),
>  		      STACK_VAR(bpf2ia32[BPF_REG_0][1]));
> +	}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31699378511