On Tue, Jan 4, 2022 at 9:18 PM Yonghong Song <yhs@xxxxxx> wrote: > > > > On 1/4/22 3:35 AM, Tal Lossos wrote: > > Hello! > > I’ve encountered a weird behaviour of verification error regarding > > using bpf_map_lookup_elem (specifically bpf_inode_storage_get in my > > use case) and BPF_CORE_READ as a key. > > For example, if I’m using an inode_storage map, and let’s say that I’m > > using a hook that has a dentry named “dentry” in the context, If I > > will try to use bpf_inode_storage_get, the only way I could do it is > > by passing dentry->d_inode as the key arg, and if I will try to do it > > in the CO-RE way by using BPF_CORE_READ(dentry, d_inode) as the key I > > will fail (because the key is a “inv” (scalar) and not “ptr_” - > > https://elixir.bootlin.com/linux/v5.11/source/kernel/bpf/bpf_inode_storage.c#L266 ): > > struct > > { > > __uint(type, BPF_MAP_TYPE_INODE_STORAGE); > > __uint(map_flags, BPF_F_NO_PREALLOC); > > __type(key, int); > > __type(value, value_t); > > } inode_storage_map SEC(".maps"); > > > > SEC("lsm/inode_rename") > > int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry, > > struct inode *new_dir, struct dentry *new_dentry, > > unsigned int flags) > > { > > struct value_t *storage; > > > > storage = bpf_inode_storage_get(&inode_storage_map, > > old_dentry->d_inode, 0, 0); // this will work > > storage = bpf_inode_storage_get(&inode_storage_map, > > BPF_CORE_READ(old_dentry, d_inode), 0, 0); // this won't work > > ... > > } > > From a quick glimpse into the verifier sources I can assume that the > > BPF_CORE_READ macro (which calls bpf_core_read), returns a “scalar” > > (is it because ebpf helpers counts as “global functions”?) thus > > failing the verification. > > This behaviour is kind of weird because I would expect to be allowed As Yonghong explained, BPF_CORE_READ() always returns unknown scalar that verifier cannot trust. All due to the underlying probe_read_kernel(). BPF_CORE_READ() was never supposed to work for such cases, it's not weird once you realize that BPF_CORE_READ() is able to probe read an arbitrary memory location. > > to call bpf_inode_storage_get with the BPF_CORE_READ (’s output) as > > the key arg. > > May I have some clarification on this please? > > The reason is BPF_CORE_READ macro eventually used > bpf_probe_read_kernel() > to read the the old_dentry->d_inode (adjusted after relocation). > The BTF_ID type information with read-result of bpf_probe_read_kernel() > is lost in verifier and that is why you hit the above verification > failure. CORE predates fentry/fexit so bpf_probe_read_kernel() is > used to relocatable kernel memory accesses. > > But now we have direct memory access. > To resolve the above issue, I think we might need libbpf to > directly modify the offset in the instruction based on > relocation records. For example, the original old_dentry->dinode > code looks like > r1 = *(u64 *)(r2 + 32) > there will be a relocation against offset "32". > libbpf could directly adjust "32" based on relocation information. I think libbpf supports that already. So if you use direct memory reads on dentry type marked with __attribute__((preserve_access_index)) it should work. > > > > > Thanks.
