Re: [PATCH bpf] net/sched: cls_bpf: prevent unbounded recursion in offload rollback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The function silently swallows error codes during a rollback, returning success (0) instead of the actual error.
--

commit ed592986d69e913d97df2ff4e0394a1bcd8686ac
Author: Jiayuan Chen <jiayuan.chen@linux.dev>
net/sched: cls_bpf: prevent unbounded recursion in offload rollback

This commit adds an is_rollback flag to cls_bpf_offload_cmd() to prevent
infinite recursion when a driver's tc_setup_cb_replace callback repeatedly
fails during a rollback operation.

Link: https://lore.kernel.org/bpf/ce5a6005-3c5e-4696-9e05-eba9461dc860@std.uestc.edu.cn/T/#u

> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 9a346b6221b35..d71fbf7cb407e 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
[ ... ]
> @@ -176,8 +177,8 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>  					  skip_sw, &oldprog->gen_flags,
>  					  &oldprog->in_hw_count, true);
>  
> -	if (prog && err) {
> -		cls_bpf_offload_cmd(tp, oldprog, prog, extack);
> +	if (prog && err && !is_rollback) {
> +		cls_bpf_offload_cmd(tp, oldprog, prog, extack, true);
>  		return err;
>  	}
>  

[Severity: Low]
Does this silently swallow the error code and return success during a rollback?

If err is non-zero but is_rollback is true, the entire if condition evaluates
to false. The function then falls through and returns 0 at the end instead of
returning the actual error.

While the only caller that sets is_rollback to true ignores the return value,
this still drops the error code. Should the recursion guard only wrap the
nested call so that the error is always returned?

    if (prog && err) {
        if (!is_rollback)
            cls_bpf_offload_cmd(tp, oldprog, prog, extack, true);
        return err;
    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522025854.341647-1-jiayuan.chen@linux.dev?part=1