Re: [PATCH bpf-next v2 2/6] bpf: Implement cgroup storage available to non-cgroup-attached bpf progs

Yonghong Song
2022-10-21T22:57:45Z

On 10/21/22 12:57 PM, David Vernet wrote:
> On Fri, Oct 21, 2022 at 10:33:41AM -0700, Yonghong Song wrote:
>
> [...]
>
>>>> /* Note that tracing related programs such as
>>>> @@ -5435,6 +5443,42 @@ union bpf_attr {
>>>> * **-E2BIG** if user-space has tried to publish a sample which is
>>>> * larger than the size of the ring buffer, or which cannot fit
>>>> * within a struct bpf_dynptr.
>>>> + *
>>>> + * void *bpf_cgrp_storage_get(struct bpf_map *map, struct cgroup *cgroup, void *value, u64 flags)
>>>> + * Description
>>>> + * Get a bpf_local_storage from the *cgroup*.
>>>> + *
>>>> + * Logically, it could be thought of as getting the value from
>>>> + * a *map* with *cgroup* as the **key**. From this
>>>> + * perspective, the usage is not much different from
>>>> + * **bpf_map_lookup_elem**\ (*map*, **&**\ *cgroup*) except this
>>>> + * helper enforces the key must be a cgroup struct and the map must also
>>>> + * be a **BPF_MAP_TYPE_CGRP_STORAGE**.
>>>> + *
>>>> + * Underneath, the value is stored locally at *cgroup* instead of
>>>> + * the *map*. The *map* is used as the bpf-local-storage
>>>> + * "type". The bpf-local-storage "type" (i.e. the *map*) is
>>>> + * searched against all bpf_local_storage residing at *cgroup*.
>>>
>>> IMO this paragraph is a bit hard to parse. Please correct me if I'm
>>> wrong, but I think what it's trying to convey is that when an instance
>>> of cgroup bpf-local-storage is accessed by a program in e.g.
>>> bpf_cgrp_storage_get(), all of the cgroup bpf_local_storage entries are
>>> iterated over in the struct cgroup object until this program's local
>>> storage instance is found. Is that right? If so, perhaps something like
>>> this would be more clear:
>>
>> yes. your above interpretation is correct.
>>
>>>
>>> In reality, the local-storage value is embedded directly inside of the
>>> *cgroup* object itself, rather than being located in the
>>> **BPF_MAP_TYPE_CGRP_STORAGE** map. When the local-storage value is
>>> queried for some *map* on a *cgroup* object, the kernel will perform an
>>> O(n) iteration over all of the live local-storage values for that
>>> *cgroup* object until the local-storage value for the *map* is found.
>>
>> Sounds okay. I can change the explanation like the above.
>
> Thanks!
>
>>>> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
>>>> index 341c94f208f4..3a12e6b400a2 100644
>>>> --- a/kernel/bpf/Makefile
>>>> +++ b/kernel/bpf/Makefile
>>>> @@ -25,7 +25,7 @@ ifeq ($(CONFIG_PERF_EVENTS),y)
>>>> obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
>>>> endif
>>>> ifeq ($(CONFIG_CGROUPS),y)
>>>
>>> I assume that you double checked that it's valid to compile the helper
>>> with CONFIG_CGROUPS && !CONFIG_CGROUP_BPF, but I must admit that even if
>>> that's the case, I'm not following why we would want the map to be
>>> compiled with a different kconfig option than the helper that provides
>>> access to it. If theres's a precedent for doing this then I suppose it's
>>> fine, but it does seem wrong and/or at least wasteful to compile these
>>> helpers in if CONFIG_CGROUPS is defined but CONFIG_CGROUP_BPF is not.
>>
>> The following is my understanding.
>> CONFIG_CGROUP_BPF guards kernel/bpf/cgroup.c which contains implementation
>> mostly for cgroup-attached program types, helpers, etc.
>
> Then why are we using it to guard
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGRP_STORAGE, cgrp_storage_map_ops)?
>
>> A lot of other cgroup-related implementation like cgroup_iter, some
>> cgroup related helper (not related to cgroup-attached program types), etc.
>> are guarded with CONFIG_CGROUPS and CONFIG_BPF_SYSCALL.
>>
>> Note that it is totally possible CONFIG_CGROUP_BPF is 'n' while
>> CONFIG_CGROUPS and CONFIG_BPF_SYSCALL are 'y'.
>>
>> So for cgroup local storage implemented in this patch set,
>> using CONFIG_CGROUPS and CONFIG_BPF_SYSCALL seems okay.
>
> I agree that it's fine to use CONFIG_CGROUPS here. What I'm not
> understanding is why we're using CONFIG_CGROUP_BPF to guard defining
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGRP_STORAGE, cgrp_storage_map_ops), and then
> in the Makefile we're using CONFIG_CGROUPS to add bpf_cgrp_storage.o.
>
> In other words, I think there's a mismatch between:
>
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -90,6 +90,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> #ifdef CONFIG_CGROUP_BPF
>
> ^^ why this instead of CONFIG_CGROUPS for BPF_MAP_TYPE_CGRP_STORAGE?
>
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> +BPF_MAP_TYPE(BPF_MAP_TYPE_CGRP_STORAGE, cgrp_storage_map_ops)
> #endif
> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
>
> and
>
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 341c94f208f4..3a12e6b400a2 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -25,7 +25,7 @@ ifeq ($(CONFIG_PERF_EVENTS),y)
> obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
> endif
> ifeq ($(CONFIG_CGROUPS),y)
> -obj-$(CONFIG_BPF_SYSCALL) += cgroup_iter.o
> +obj-$(CONFIG_BPF_SYSCALL) += cgroup_iter.o bpf_cgrp_storage.o
> endif
> obj-$(CONFIG_CGROUP_BPF) += cgroup.o
> ifeq ($(CONFIG_INET),y)

This makes sense. I will guard
BPF_MAP_TYPE(BPF_MAP_TYPE_CGRP_STORAGE, cgrp_storage_map_ops)
with CONFIG_CGROUPS.

>
>>>> -obj-$(CONFIG_BPF_SYSCALL) += cgroup_iter.o
>>>> +obj-$(CONFIG_BPF_SYSCALL) += cgroup_iter.o bpf_cgrp_storage.o
>>>> endif
>>>> obj-$(CONFIG_CGROUP_BPF) += cgroup.o
>>>> ifeq ($(CONFIG_INET),y)
>
> [...]
>
>>>> + * 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);
>>>> + free_cgroup_storage =
>>>> + bpf_selem_unlink_storage_nolock(local_storage, selem, false, false);
>>>
>>> This still requires a comment explaining why it's OK to overwrite
>>> free_cgroup_storage with a previous value from calling
>>> bpf_selem_unlink_storage_nolock(). Even if that is safe, this looks like
>>> a pretty weird programming pattern, and IMO doing this feels more
>>> intentional and future-proof:
>>>
>>> if (bpf_selem_unlink_storage_nolock(local_storage, selem, false, false))
>>> free_cgroup_storage = true;
>>
>> We have a comment a few lines below.
>> /* free_cgroup_storage should always be true as long as
>> * local_storage->list was non-empty.
>> */
>> if (free_cgroup_storage)
>> kfree_rcu(local_storage, rcu);
>
> IMO that comment doesn't provide much useful information -- it states an
> assumption, but doesn't give a reason for it.
>
>> I will add more explanation in the above code like
>>
>> bpf_selem_unlink_map(selem);
>> /* If local_storage list only have 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);
>
> Thanks, this is the type of comment I was looking for.
>
> Also, I realize this was copy-pasted from a number of other possible
> locations in the codebase which are doing the same thing, but I still
> think this pattern is an odd and brittle way to do this. We're relying
> on an abstracted implementation detail of
> bpf_selem_unlink_storage_nolock() for correctness, which IMO is a signal
> that bpf_selem_unlink_storage_nolock() should probably be the one
> invoking kfree_rcu() on behalf of callers in the first place. It looks
> like all of the callers end up calling kfree_rcu() on the struct
> bpf_local_storage * if bpf_selem_unlink_storage_nolock() returns true,
> so can we just move the responsibility of freeing the local storage
> object down into bpf_selem_unlink_storage_nolock() where it's unlinked?

We probably cannot do this. bpf_selem_unlink_storage_nolock()
is inside the rcu_read_lock() region. We do kfree_rcu() outside
the rcu_read_lock() region.

>
> IMO this can be done in a separate patch set, if we decide it's worth
> doing at all.
>
>>>
>>>> + }
>>>> + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
>>>> + bpf_cgrp_storage_unlock();
>>>> + rcu_read_unlock();
>>>> +
>>>> + /* free_cgroup_storage should always be true as long as
>>>> + * local_storage->list was non-empty.
>>>> + */
>>>> + if (free_cgroup_storage)
>>>> + kfree_rcu(local_storage, rcu);
>>>> +}
>>>> +
>>>> +static struct bpf_local_storage_data *
>>>> +cgroup_storage_lookup(struct cgroup *cgroup, struct bpf_map *map, bool cacheit_lockit)
>>>> +{
>>>> + struct bpf_local_storage *cgroup_storage;
>>>> + struct bpf_local_storage_map *smap;
>>>> +
>>>> + cgroup_storage = rcu_dereference_check(cgroup->bpf_cgrp_storage,
>>>> + bpf_rcu_lock_held());
>>>> + if (!cgroup_storage)
>>>> + return NULL;
>>>> +
>>>> + smap = (struct bpf_local_storage_map *)map;
>>>> + return bpf_local_storage_lookup(cgroup_storage, smap, cacheit_lockit);
>>>> +}
>>>> +
>>>> +static void *bpf_cgrp_storage_lookup_elem(struct bpf_map *map, void *key)
>>>> +{
>>>> + struct bpf_local_storage_data *sdata;
>>>> + struct cgroup *cgroup;
>>>> + int fd;
>>>> +
>>>> + fd = *(int *)key;
>>>> + cgroup = cgroup_get_from_fd(fd);
>>>> + if (IS_ERR(cgroup))
>>>> + return ERR_CAST(cgroup);
>>>> +
>>>> + bpf_cgrp_storage_lock();
>>>> + sdata = cgroup_storage_lookup(cgroup, map, true);
>>>> + bpf_cgrp_storage_unlock();
>>>> + cgroup_put(cgroup);
>>>> + return sdata ? sdata->data : NULL;
>>>> +}
>>>
>>> Stanislav pointed out in the v1 revision that there's a lot of very
>>> similar logic in task storage, and I think you'd mentioned that you were
>>> going to think about generalizing some of that. Have you had a chance to
>>> consider?
>>
>> It is hard to have a common function for
>> lookup_elem/update_elem/delete_elem(). They are quite different as each
>> heavily involves
>> task/cgroup-specific functions.
>
> Yes agreed, each implementation is acquiring their own references, and
> finding the backing element in whatever way it was implemented, etc.
>
>> but map_alloc and map_free could have common helpers.
>
> Agreed, and many of the static functions that are invoked on those paths
> such as bpf_cgrp_storage_free(), bpf_cgrp_storage_lock(), etc possibly
> as well. In general this feels like something we could pretty easily
> simplify using something like a structure with callbacks to implement
> the pieces of logic that are specific to each local storage type, such
> as getting the struct bpf_local_storage __rcu
> * pointer from some context (e.g. cgroup_storage_ptr()). It doesn't
> necessarily need to block this change, but IMO we should clean this up
> soon because a lot of this is nearly a 100% copy-paste of other local
> storage implementations.

Further refactoring is possible. Martin is working to simplify the
locking mechanism. We can wait for that done before doing refactoring.

>
> Thanks,
> David