On 1/5/22 8:36 PM, Andrii Nakryiko wrote:
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.
Oh, I missed this. If you use vmlinux.h, then CORE for
old_dentry->d_inode is automatically covered since types in
vmlinux.h already have __attribute__((preserve_access_index)).
Otherwise, you need to define your own struct dentry with
added preserve_access_index attribute.
Thanks.
