Re: [PATCH bpf-next v5 5/6] riscv, bpf: Mixing bpf2bpf and tailcalls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Extension programs (BPF_PROG_TYPE_EXT) targeting subprograms incorrectly reset the Tail Call Counter (TCC), bypassing the tail call limit and allowing infinite loops.
--

--- Patch [5]: [PATCH bpf-next v5 5/6] riscv, bpf: Mixing bpf2bpf and tailcalls ---
commit f7082c3640c8a167ff897d3870ee0dd71fc2faab
Author: Pu Lehui <pulehui@huawei.com>

riscv, bpf: Mixing bpf2bpf and tailcalls

This commit implements mixed bpf2bpf and tailcalls for the RV64 JIT.
It changes how the Tail Call Counter (TCC) is passed and preserved,
storing it in a non-callee saved register and explicitly saving/restoring 
it around bpf2bpf calls and tail calls.

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 7c6304e0b846a..823262ca47ebf 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
[ ... ]
> @@ -1836,10 +1829,18 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>  			}
>  		}
>  
> +		/* restore TCC to RV_REG_TCC before bpf2bpf call */
> +		if (aux->tail_call_reachable && insn->src_reg == BPF_PSEUDO_CALL)
> +			emit_ld(RV_REG_TCC, ctx->tcc_offset, RV_REG_SP, ctx);
> +
>  		ret = emit_call(addr, fixed_addr, ctx);
>  		if (ret)
>  			return ret;
>  
> +		/* store updated TCC back to stack after bpf2bpf call */
> +		if (aux->tail_call_reachable && insn->src_reg == BPF_PSEUDO_CALL)
> +			emit_sd(RV_REG_SP, ctx->tcc_offset, RV_REG_TCC, ctx);
> +

[Severity: High]
If the bpf2bpf call jumps to a BPF_PROG_TYPE_EXT program that incorrectly
reset RV_REG_TCC, will the caller now save that clobbered value to its own
stack?

If the TCC is continuously reset to 33 in this manner, could this bypass the
tail call limit and allow an infinite loop resulting in a CPU soft lockup?

>  		if (insn->src_reg != BPF_PSEUDO_CALL)
>  			emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
>  		break;
[ ... ]
> @@ -2038,11 +2038,10 @@ void bpf_jit_build_prologue(struct rv_jit_context *ctx, bool is_subprog)
>  	for (i = 0; i < RV_FENTRY_NINSNS; i++)
>  		emit(rv_nop(), ctx);
>  
> -	/* First instruction is always setting the tail-call-counter
> -	 * (TCC) register. This instruction is skipped for tail calls.
> -	 * Force using a 4-byte (non-compressed) instruction.
> -	 */
> -	emit(rv_addi(RV_REG_TCC, RV_REG_ZERO, MAX_TAIL_CALL_CNT), ctx);
> +	if (!is_subprog)
> +		emit(rv_addi(RV_REG_TCC, RV_REG_ZERO, MAX_TAIL_CALL_CNT), ctx);
> +

[Severity: High]
Does this logic handle BPF_PROG_TYPE_EXT (freplace) programs correctly?

When an extension program is loaded independently and attached to a global
subprogram of another BPF program, is_subprog will be false because it is the
main program of its own object.

Won't this cause the EXT program to unconditionally emit this instruction,
clobbering the inherited TCC passed in RV_REG_TCC from the caller?

> +	/* tailcall starts here, emit insn before it must be fixed */
>  
>  	emit_addi(RV_REG_SP, RV_REG_SP, -stack_adjust, ctx);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707142219.2871758-1-pulehui@huaweicloud.com?part=5