Re: Verification error on bpf_map_lookup_elem with BPF_CORE_READ

[Yonghong Song <yhs@fb.com>]

On 1/5/22 8:36 PM, Andrii Nakryiko wrote:
> On Tue, Jan 4, 2022 at 9:18 PM Yonghong Song <yhs@fb.com> 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.