Messages in this thread | | | Subject | Re: [PATCH bpf-next 4/4] bpf: Add selftests for local_storage | From | Yonghong Song <> | Date | Tue, 16 Jun 2020 13:40:26 -0700 |
| |
On 6/16/20 12:25 PM, Andrii Nakryiko wrote: > On Tue, Jun 16, 2020 at 8:54 AM KP Singh <kpsingh@chromium.org> wrote: >> On 01-Jun 13:29, Andrii Nakryiko wrote: >>> On Tue, May 26, 2020 at 9:34 AM KP Singh <kpsingh@chromium.org> wrote: >>>> From: KP Singh <kpsingh@google.com> >>>> >>>> inode_local_storage: >>>> >>>> * Hook to the file_open and inode_unlink LSM hooks. >>>> * Create and unlink a temporary file. >>>> * Store some information in the inode's bpf_local_storage during >>>> file_open. >>>> * Verify that this information exists when the file is unlinked. >>>> >>>> sk_local_storage: >>>> >>>> * Hook to the socket_post_create and socket_bind LSM hooks. >>>> * Open and bind a socket and set the sk_storage in the >>>> socket_post_create hook using the start_server helper. >>>> * Verify if the information is set in the socket_bind hook. >>>> >>>> Signed-off-by: KP Singh <kpsingh@google.com> >>>> --- >>>> .../bpf/prog_tests/test_local_storage.c | 60 ++++++++ >>>> .../selftests/bpf/progs/local_storage.c | 139 ++++++++++++++++++ >>>> 2 files changed, 199 insertions(+) >>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/test_local_storage.c >>>> create mode 100644 tools/testing/selftests/bpf/progs/local_storage.c >>>> >>> [...] >>> >>>> +struct dummy_storage { >>>> + __u32 value; >>>> +}; >>>> + >>>> +struct { >>>> + __uint(type, BPF_MAP_TYPE_INODE_STORAGE); >>>> + __uint(map_flags, BPF_F_NO_PREALLOC); >>>> + __type(key, int); >>>> + __type(value, struct dummy_storage); >>>> +} inode_storage_map SEC(".maps"); >>>> + >>>> +struct { >>>> + __uint(type, BPF_MAP_TYPE_SK_STORAGE); >>>> + __uint(map_flags, BPF_F_NO_PREALLOC | BPF_F_CLONE); >>>> + __type(key, int); >>>> + __type(value, struct dummy_storage); >>>> +} sk_storage_map SEC(".maps"); >>>> + >>>> +/* Using vmlinux.h causes the generated BTF to be so big that the object >>>> + * load fails at btf__load. >>>> + */ >>> That's first time I hear about such issue. Do you have an error log >>> from verifier? >> Here's what I get when I do the following change. >> >> --- a/tools/testing/selftests/bpf/progs/local_storage.c >> +++ b/tools/testing/selftests/bpf/progs/local_storage.c >> @@ -4,8 +4,8 @@ >> * Copyright 2020 Google LLC. >> */ >> >> +#include "vmlinux.h" >> #include <errno.h> >> -#include <linux/bpf.h> >> #include <stdbool.h> >> #include <bpf/bpf_helpers.h> >> #include <bpf/bpf_tracing.h> >> @@ -37,24 +37,6 @@ struct { >> __type(value, struct dummy_storage); >> } sk_storage_map SEC(".maps"); >> >> -/* Using vmlinux.h causes the generated BTF to be so big that the object >> - * load fails at btf__load. >> - */ >> -struct sock {} __attribute__((preserve_access_index)); >> -struct sockaddr {} __attribute__((preserve_access_index)); >> -struct socket { >> - struct sock *sk; >> -} __attribute__((preserve_access_index)); >> - >> -struct inode {} __attribute__((preserve_access_index)); >> -struct dentry { >> - struct inode *d_inode; >> -} __attribute__((preserve_access_index)); >> -struct file { >> - struct inode *f_inode; >> -} __attribute__((preserve_access_index)); >> >> ./test_progs -t test_local_storage >> libbpf: Error loading BTF: Invalid argument(22) >> libbpf: magic: 0xeb9f >> version: 1 >> flags: 0x0 >> hdr_len: 24 >> type_off: 0 >> type_len: 4488 >> str_off: 4488 >> str_len: 3012 >> btf_total_size: 7524 >> >> [1] STRUCT (anon) size=32 vlen=4 >> type type_id=2 bits_offset=0 >> map_flags type_id=6 bits_offset=64 >> key type_id=8 bits_offset=128 >> value type_id=9 bits_offset=192 >> [2] PTR (anon) type_id=4 >> [3] INT int size=4 bits_offset=0 nr_bits=32 encoding=SIGNED >> [4] ARRAY (anon) type_id=3 index_type_id=5 nr_elems=28 >> [5] INT __ARRAY_SIZE_TYPE__ size=4 bits_offset=0 nr_bits=32 encoding=(none) >> [6] PTR (anon) type_id=7 >> [7] ARRAY (anon) type_id=3 index_type_id=5 nr_elems=1 >> [8] PTR (anon) type_id=3 >> [9] PTR (anon) type_id=10 >> [10] STRUCT dummy_storage size=4 vlen=1 >> value type_id=11 bits_offset=0 >> [11] TYPEDEF __u32 type_id=12 >> >> [... More BTF Dump ...] >> >> [91] TYPEDEF wait_queue_head_t type_id=175 >> >> [... More BTF Dump ...] >> >> [173] FWD super_block struct >> [174] FWD vfsmount struct >> [175] FWD wait_queue_head struct >> [106] STRUCT socket_wq size=128 vlen=4 >> wait type_id=91 bits_offset=0 Invalid member >> >> libbpf: Error loading .BTF into kernel: -22. >> libbpf: map 'inode_storage_map': failed to create: Invalid argument(-22) >> libbpf: failed to load object 'local_storage' >> libbpf: failed to load BPF skeleton 'local_storage': -22 >> test_test_local_storage:FAIL:skel_load lsm skeleton failed >> #81 test_local_storage:FAIL >> >> The failiure is in: >> >> [106] STRUCT socket_wq size=128 vlen=4 >> wait type_id=91 bits_offset=0 Invalid member >> >>> Clang is smart enough to trim down used types to only those that are >>> actually necessary, so too big BTF shouldn't be a thing. But let's try >>> to dig into this and fix whatever issue it is, before giving up :) >>> >> I was wrong about the size being an issue. The verifier thinks the BTF >> is invalid and more specificially it thinks that the socket_wq's >> member with type_id=91, i.e. typedef wait_queue_head_t is invalid. Am >> I missing some toolchain patches? >> > It is invalid BTF in the sense that we have a struct, embedding a > struct, which is only defined as a forward declaration. There is not > enough information and such situation would have caused compilation > error, because it's impossible to determine the size of the outer > struct. > > Yonghong, it seems like Clang is pruning types too aggressively here? > We should keep types that are embedded, even if they are not used > directly by user code. Could you please take a look?
Sure. Will take a look shortly.
> > > >> - KP >> >> >>>> +struct sock {} __attribute__((preserve_access_index)); >>>> +struct sockaddr {} __attribute__((preserve_access_index)); >>>> +struct socket { >>>> + struct sock *sk; >>>> +} __attribute__((preserve_access_index)); >>>> + >>>> +struct inode {} __attribute__((preserve_access_index)); >>>> +struct dentry { >>>> + struct inode *d_inode; >>>> +} __attribute__((preserve_access_index)); >>>> +struct file { >>>> + struct inode *f_inode; >>>> +} __attribute__((preserve_access_index)); >>>> + >>>> + >>> [...]
| |