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). 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. YiFei Zhu
