Verification error on bpf_map_lookup_elem with BPF_CORE_READ

Verification error on bpf_map_lookup_elem with BPF_CORE_READ

[Tal Lossos]

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
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?

Thanks.

Re: Verification error on bpf_map_lookup_elem with BPF_CORE_READ

[Yonghong Song]

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
> 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.

> 
> Thanks.

Re: Verification error on bpf_map_lookup_elem with BPF_CORE_READ

[Andrii Nakryiko]

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.

>
> >
> > Thanks.

Re: Verification error on bpf_map_lookup_elem with BPF_CORE_READ

[Yonghong Song]

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.