Re: [PATCH bpf-next v3 6/7] selftests/bpf: detach a struct_ops link from the subsystem managing it.

[Amery Hung <ameryhung@gmail.com>]

On Thu, May 09, 2024 at 05:29:41PM -0700, Kui-Feng Lee wrote:
> Not only a user space program can detach a struct_ops link, the subsystem
> managing a link can also detach the link. This patch adds a kfunc to
> simulate detaching a link by the subsystem managing it and makes sure user
> space programs get notified through epoll.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 42 ++++++++++++
>  .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  1 +
>  .../bpf/prog_tests/test_struct_ops_module.c   | 67 +++++++++++++++++++
>  .../selftests/bpf/progs/struct_ops_detach.c   |  7 ++
>  4 files changed, 117 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 1150e758e630..1f347eed6c18 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -741,6 +741,38 @@ __bpf_kfunc int bpf_kfunc_call_kernel_getpeername(struct addr_args *args)
>  	return err;
>  }
>  
> +static DEFINE_SPINLOCK(detach_lock);
> +static struct bpf_link *link_to_detach;
> +
> +__bpf_kfunc int bpf_dummy_do_link_detach(void)
> +{
> +	struct bpf_link *link;
> +	int ret = -ENOENT;
> +
> +	/* A subsystem must ensure that a link is valid when detaching the
> +	 * link. In order to achieve that, the subsystem may need to obtain
> +	 * a lock to safeguard a table that holds the pointer to the link
> +	 * being detached. However, the subsystem cannot invoke
> +	 * link->ops->detach() while holding the lock because other tasks
> +	 * may be in the process of unregistering, which could lead to
> +	 * acquiring the same lock and causing a deadlock. This is why
> +	 * bpf_link_inc_not_zero() is used to maintain the link's validity.
> +	 */
> +	spin_lock(&detach_lock);
> +	link = link_to_detach;
> +	/* Make sure the link is still valid by increasing its refcnt */
> +	if (link && IS_ERR(bpf_link_inc_not_zero(link)))
> +		link = NULL;
> +	spin_unlock(&detach_lock);
> +

I know it probably doesn't matter in this example, but where would you set
link_to_detach to NULL if reg and unreg can be called multiple times?

> +	if (link) {
> +		ret = link->ops->detach(link);
> +		bpf_link_put(link);
> +	}
> +
> +	return ret;
> +}

[...]