Re: [PATCH bpf-next v2 1/2] bpf: Add LINK_DETACH support for perf link

[Yonghong Song <yonghong.song@linux.dev>]

On 3/5/26 7:59 PM, Alexei Starovoitov wrote:
> On Thu, Mar 5, 2026 at 7:39 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>> On Wed, 4 Mar 2026 at 23:01, <bot+bpf-ci@kernel.org> wrote:
>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>> --- a/kernel/bpf/syscall.c
>>>> +++ b/kernel/bpf/syscall.c
>>>> @@ -3863,10 +3863,23 @@ struct bpf_perf_link {
>>>>   static void bpf_perf_link_release(struct bpf_link *link)
>>>>   {
>>>>        struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
>>>> -     struct perf_event *event = perf_link->perf_file->private_data;
>>>> +     struct perf_event *event;
>>>> +     struct file *perf_file;
>>>> +
>>>> +     /* bpf_perf_link_detach() may have already released perf_file */
>>>> +     perf_file = xchg(&perf_link->perf_file, NULL);
>>>> +     if (!perf_file)
>>>> +             return;
>>>> +
>>>> +     event = perf_file->private_data;
>>>>        perf_event_free_bpf_prog(event);
>>>> -     fput(perf_link->perf_file);
>>>> +     fput(perf_file);
>>>> +}
>>> [ ... ]
>>>
>>>> @@ -4075,9 +4088,13 @@ static int bpf_perf_link_fill_link_info(const struct bpf_link *link,
>>>>   {
>>>>        struct bpf_perf_link *perf_link;
>>>>        const struct perf_event *event;
>>>> +     struct file *perf_file;
>>>> +
>>>>        perf_link = container_of(link, struct bpf_perf_link, link);
>>>> -     event = perf_get_event(perf_link->perf_file);
>>>> +     perf_file = READ_ONCE(perf_link->perf_file);
>>>> +     if (!perf_file)
>>>> +             return 0;
>>>> +     event = perf_get_event(perf_file);
>>> Could bpf_perf_link_fill_link_info() race with bpf_perf_link_release()
>>> here?
>>>
>>> After READ_ONCE reads a non-NULL perf_file, no reference is taken on
>>> the file.  A concurrent BPF_LINK_DETACH on another CPU can call
>>> bpf_perf_link_release(), which does xchg() followed by fput().  If
>>> the perf event fd has already been closed by userspace, fput() drops
>>> the last reference.  Once the detaching thread returns to userspace,
>>> task_work runs __fput and frees the file struct.
>>>
>>> At that point perf_get_event() dereferences file->f_op on freed
>>> memory:
>>>
>>>    Thread A (BPF_OBJ_GET_INFO_BY_FD)      Thread B (BPF_LINK_DETACH)
>>>    --------------------------------       --------------------------
>>>    perf_file = READ_ONCE(...)  [non-NULL]
>>>                                           xchg(&..., NULL)
>>>                                           fput(perf_file)
>>>                                           return to userspace
>>>                                           task_work -> __fput -> free
>>>    perf_get_event(perf_file) -> UAF
>>>
>>> The xdp and cgroup link types handle this by serializing
>>> fill_link_info and release under a shared lock (rtnl_lock and
>>> cgroup_lock respectively):
>>>
>>> kernel/bpf/cgroup.c:bpf_cgroup_link_fill_link_info() {
>>>      cgroup_lock();
>>>      if (cg_link->cgroup)
>>>          cg_id = cgroup_id(cg_link->cgroup);
>>>      cgroup_unlock();
>>> }
>>>
>>> net/core/dev.c:bpf_xdp_link_fill_link_info() {
>>>      rtnl_lock();
>>>      if (xdp_link->dev)
>>>          ifindex = xdp_link->dev->ifindex;
>>>      rtnl_unlock();
>>> }
>>>
>>> The same issue applies to bpf_perf_link_show_fdinfo() below.
>>>
>>>> @@ -4143,9 +4160,13 @@ static void bpf_perf_link_show_fdinfo(const struct bpf_link *link,
>>>>   {
>>>>        struct bpf_perf_link *perf_link;
>>>>        const struct perf_event *event;
>>>> +     struct file *perf_file;
>>>> +
>>>>        perf_link = container_of(link, struct bpf_perf_link, link);
>>>> -     event = perf_get_event(perf_link->perf_file);
>>>> +     perf_file = READ_ONCE(perf_link->perf_file);
>>>> +     if (!perf_file)
>>>> +             return;
>>>> +     event = perf_get_event(perf_file);
>>> Same race as bpf_perf_link_fill_link_info() above -- a concurrent
>>> BPF_LINK_DETACH can free perf_file between the READ_ONCE and the
>>> perf_get_event() call.
>> AI is correct, but is not proposing a fix, so I will try to be helpful.
>> I think we can add per-link spin lock, so that operations on the same
>> link are serialized.
>> You can do the perf_file replacement in release under the lock, and
>> call the rest of the stuff (perf_event_free.., fput) outside the lock.
>> On the show_fdinfo and fill_link_info side, you will under the
>> protection of the spin lock, read the perf_file and take a reference
>> on it using get_file.
>> Then you can release the lock and use it for the callback and fput on
>> the way out.
>>
>> Other cases are unaffected since they are already under lock
>> protection. I think a global lock is also an option, but I would lean
>> toward having a 4-byte spin lock in the bpf_link itself.
> I would just do a global mutex and avoid the complexity.
> fdinfo could be doing a bunch of work.

The following is an example using mutex lock:

https://lore.kernel.org/r/20240410043527.3737160-1-yonghong.song@linux.dev