On Tue, Jul 21, 2020 at 07:19:31PM -0500, YiFei Zhu wrote: > On Tue, Jul 21, 2020 at 7:09 PM Martin KaFai Lau <kafai@xxxxxx> wrote: > > I think we are talking about the existing map->aux check in > > bpf_cgroup_storage_assign()? Right, the map is dedicated for > > only one bpf prog which usually has only one expected_attach_type. > > Thus, the attach_type of the key is not really useful since it will > > always be the same. However, it does not mean the exposed > > attach_type (of the key) is meaningless or the value is invalid. > > The exposed value is valid. > > > > I am trying to say attach_type is still part of the key > > and exposed to userspace (e.g. in map dump) but it is sort of > > invalid after this change because I am not sure what that "0" > > means now. > > > > Also, as part of the key, it is intuitive for user to think the storage is > > unique per (cgroup, attach_type). This uniqueness is always true now because > > of the map->aux logic and guaranteed by the verifier. With this patch, this > > "key" intuition is gone where the kernel quietly ignore the attach_type. > > Right. It is indeed non-intuitive for part of the "key" to be > completely ignored in cmp. However, what would a sane solution be > instead? I could at attach time, use the attach type to also perform > the lookup, and store the attach type as the key, if and only if the > size of the key == sizeof(struct cgroup_storage_key). Yes, that makes sense. cgroup_storage_lookup() has the map which has the key_size, so it can decide which part of the key to use for lookup. bpf_cgroup_storages_alloc() does not need to know the details and it just populates everything in bpf_cgroup_storage_key and let cgroup_storage_lookup() to decide. Thus, it should not be a big change on this patch. > This storage > will not be shared across different attach types, only across > different programs of the same attach type. The lifetime will still > bound to the (map, cgroup_bpf) pair, rather than the program, as the > implementation prior to this patch did. Right, the lifetime of the storage should be the same regardless of the key. The major idea of this patch is to make storage sharable across different bpf-progs and this part does not change. How sharable is defined by the map's key. The key of the map should behave like a normal map's key. sharable among (cgroup_id, attach_type) or sharable among (cgroup_id) Then all lookup/update/map-dump will behave like a normal map without an invalid value "0" (BPF_CGROUP_INET_INGRESS) in the attach_type. Thus, I don't expect a big change in this patch. May be restoring the original bpf_cgroup_storage_key_cmp() behavior and a few key_size check in the map_ops's lookup/update/check_btf...etc.
