On Tue, May 21, 2024 at 5:31 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 5/21/24 15:56, Amery Hung wrote:
> > 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@xxxxxxxxx>
> >> ---
> >> .../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?
>
> For the same link if there is, reg() can be called only once
> except if unreg() has been called for the previous reg() call on the
> same link. Unreg() can only be called for once after a reg() call on the
> same link.
>
> For struct_ops map with link, unreg() is called by
> bpf_struct_ops_map_link_dealloc() and bpf_struct_ops_map_link_detach().
> The former one is called for a link only if the refcnt of the link has
> dropped to zero. The later one is called for a link only if the refcnt
> is not zero, and it holds update_mutex. Once unreg() has been called,
> link->map will be cleared as well. So, unreg() should not be called
> twice on the same link except it is registered again.
>
> Does that answer your question?
>
Thanks for the detailed explanation. That makes sense to me.
> >
> >> + if (link) {
> >> + ret = link->ops->detach(link);
> >> + bpf_link_put(link);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >
> > [...]
