Re: [PATCH bpf-next v4 3/7] bpf: Implement cgroup storage available to non-cgroup-attached bpf progs

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/24/22 5:21 PM, Yosry Ahmed wrote:
> On Mon, Oct 24, 2022 at 2:15 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 10/23/22 11:05 AM, Yonghong Song wrote:
>>> +void bpf_cgrp_storage_free(struct cgroup *cgroup)
>>> +{
>>> +     struct bpf_local_storage *local_storage;
>>> +     struct bpf_local_storage_elem *selem;
>>> +     bool free_cgroup_storage = false;
>>> +     struct hlist_node *n;
>>> +     unsigned long flags;
>>> +
>>> +     rcu_read_lock();
>>> +     local_storage = rcu_dereference(cgroup->bpf_cgrp_storage);
>>> +     if (!local_storage) {
>>> +             rcu_read_unlock();
>>> +             return;
>>> +     }
>>> +
>>> +     /* Neither the bpf_prog nor the bpf_map's syscall
>>> +      * could be modifying the local_storage->list now.
>>> +      * Thus, no elem can be added to or deleted from the
>>> +      * local_storage->list by the bpf_prog or by the bpf_map's syscall.
>>> +      *
>>> +      * It is racing with __bpf_local_storage_map_free() alone
>>> +      * when unlinking elem from the local_storage->list and
>>> +      * the map's bucket->list.
>>> +      */
>>> +     bpf_cgrp_storage_lock();
>>> +     raw_spin_lock_irqsave(&local_storage->lock, flags);
>>> +     hlist_for_each_entry_safe(selem, n, &local_storage->list, snode) {
>>> +             bpf_selem_unlink_map(selem);
>>> +             /* If local_storage list has only one element, the
>>> +              * bpf_selem_unlink_storage_nolock() will return true.
>>> +              * Otherwise, it will return false. The current loop iteration
>>> +              * intends to remove all local storage. So the last iteration
>>> +              * of the loop will set the free_cgroup_storage to true.
>>> +              */
>>> +             free_cgroup_storage =
>>> +                     bpf_selem_unlink_storage_nolock(local_storage, selem, false, false);
>>> +     }
>>> +     raw_spin_unlock_irqrestore(&local_storage->lock, flags);
>>> +     bpf_cgrp_storage_unlock();
>>> +     rcu_read_unlock();
>>> +
>>> +     if (free_cgroup_storage)
>>> +             kfree_rcu(local_storage, rcu);
>>> +}
>>
>> [ ... ]
>>
>>> +/* *gfp_flags* is a hidden argument provided by the verifier */
>>> +BPF_CALL_5(bpf_cgrp_storage_get, struct bpf_map *, map, struct cgroup *, cgroup,
>>> +        void *, value, u64, flags, gfp_t, gfp_flags)
>>> +{
>>> +     struct bpf_local_storage_data *sdata;
>>> +
>>> +     WARN_ON_ONCE(!bpf_rcu_lock_held());
>>> +     if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
>>> +             return (unsigned long)NULL;
>>> +
>>> +     if (!cgroup)
>>> +             return (unsigned long)NULL;
>>> +
>>> +     if (!bpf_cgrp_storage_trylock())
>>> +             return (unsigned long)NULL;
>>> +
>>> +     sdata = cgroup_storage_lookup(cgroup, map, true);
>>> +     if (sdata)
>>> +             goto unlock;
>>> +
>>> +     /* only allocate new storage, when the cgroup is refcounted */
>>> +     if (!percpu_ref_is_dying(&cgroup->self.refcnt) &&
>>> +         (flags & BPF_LOCAL_STORAGE_GET_F_CREATE))
>>> +             sdata = bpf_local_storage_update(cgroup, (struct bpf_local_storage_map *)map,
>>> +                                              value, BPF_NOEXIST, gfp_flags);
>>> +
>>> +unlock:
>>> +     bpf_cgrp_storage_unlock();
>>> +     return IS_ERR_OR_NULL(sdata) ? (unsigned long)NULL : (unsigned long)sdata->data;
>>> +}
>>
>> [ ... ]
>>
>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>> index 764bdd5fd8d1..32145d066a09 100644
>>> --- a/kernel/cgroup/cgroup.c
>>> +++ b/kernel/cgroup/cgroup.c
>>> @@ -5227,6 +5227,10 @@ static void css_free_rwork_fn(struct work_struct *work)
>>>        struct cgroup_subsys *ss = css->ss;
>>>        struct cgroup *cgrp = css->cgroup;
>>>
>>> +#ifdef CONFIG_BPF_SYSCALL
>>> +     bpf_cgrp_storage_free(cgrp);
>>> +#endif
>>
>>
>> After revisiting comment 4bfc0bb2c60e, some of the commit message came to my mind:
>>
>> " ...... it blocks a possibility to implement
>>     the memcg-based memory accounting for bpf objects, because a circular
>>     reference dependency will occur. Charged memory pages are pinning the
>>     corresponding memory cgroup, and if the memory cgroup is pinning
>>     the attached bpf program, nothing will be ever released."
>>
>> Considering the bpf_map_kzalloc() is used in bpf_local_storage_map.c and it can
>> charge the memcg, I wonder if the cgrp_local_storage will have similar refcnt
>> loop issue here.
>>
>> If here is the right place to free the cgrp_local_storage() and enough to break
>> this refcnt loop, it will be useful to add some explanation and its
>> consideration in the commit message.
>>
> 
> I think a similar refcount loop issue can happen here as well. IIUC,
> this function will only be run when the css is released after all
> references are dropped. So if memcg charging is enabled the cgroup
> will never be removed. This one might be trickier to handle though..

How about removing all storage from cgrp->bpf_cgrp_storage in 
cgroup_destroy_locked()?