Re: [PATCH] bpf: fix reference count leak in bpf_prog_test_run_xdp()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2026/01/08 23:01, Toke Høiland-Jørgensen wrote:
>> Hmm, this will end up call bpf_ctx_finish() in the error path, which I'm
>> not sure we want?
>
> Excuse me, but I don't think bpf_ctx_finish() will be called, for
>
> +out_put_dev:
>  	/* We convert the xdp_buff back to an xdp_md before checking the return
>  	 * code so the reference count of any held netdevice will be decremented
>  	 * even if the test run failed.
>  	 */
>  	xdp_convert_buff_to_md(&xdp, ctx);
>  	if (ret) // <== ret was set to non-0 value immediately before the "goto out_put_dev;" line.
>  		goto out;

Oh, right; I think my brain just pattern matched on "if (ret) right
after a function call" and assumed there was an assignment to ret there
as well :D

Okay, not the clearest code flow, but not sure there's a good way to
make it clearer without quite a bit of refactoring.

>  
>  	size = xdp.data_end - xdp.data_meta + sinfo->xdp_frags_size;
>  	ret = bpf_test_finish(kattr, uattr, xdp.data_meta, sinfo, size, sinfo->xdp_frags_size,
>  			      retval, duration);
>  	if (!ret)
>  		ret = bpf_ctx_finish(kattr, uattr, ctx,
>  				     sizeof(struct xdp_md));
>
>> 
>> Could we just move the xdp_convert_md_to_buff() call to after the frags
>> have been copied? Not sure there's technically any dependency there,
>> even though it does look a little off?
>
> Unless
>
> 	xdp_md->data = xdp->data - xdp->data_meta;
> 	xdp_md->data_end = xdp->data_end - xdp->data_meta;
>
> in xdp_convert_buff_to_md() lines do something bad for the error path,
> I think this change will be safe.

Yeah, sure, this should be fine.

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>