| Message ID | 20200526163336.63653-5-kpsingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalizing bpf_local_storage | expand |
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? 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 :) > +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)); > + > + [...]
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? - 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)); > > + > > + > > [...]
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? > - 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)); > > > + > > > + > > > > [...]
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)); >>>> + >>>> + >>> [...]
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? Yes, this is a llvm bug. The proposed patch is here. https://reviews.llvm.org/D82041 Will merge into llvm 11 trunk after the review. Not sure whether we can get it into llvm 10.0.1 or not as its release date is also very close. > > > >> - 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)); >>>> + >>>> + >>> [...]
Thanks for sending a fix. Should I keep the patch as it is with a TODO to move to vmlinux.h when LLVM is updated? On Wed, Jun 17, 2020 at 9:19 PM Yonghong Song <yhs@fb.com> wrote: > > > 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? > > Yes, this is a llvm bug. The proposed patch is here. > > https://reviews.llvm.org/D82041 > > Will merge into llvm 11 trunk after the review. Not sure > > whether we can get it into llvm 10.0.1 or not as its release > > date is also very close. > > > > > > > > > >> - 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)); > >>>> + > >>>> + > >>> [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c new file mode 100644 index 000000000000..ee4e27473c1d --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2020 Google LLC. + */ + +#include <test_progs.h> +#include <linux/limits.h> + +#include "local_storage.skel.h" +#include "network_helpers.h" + +int create_and_unlink_file(void) +{ + char fname[PATH_MAX] = "/tmp/fileXXXXXX"; + int fd; + + fd = mkstemp(fname); + if (fd < 0) + return fd; + + close(fd); + unlink(fname); + return 0; +} + +void test_test_local_storage(void) +{ + struct local_storage *skel = NULL; + int err, duration = 0, serv_sk = -1; + + skel = local_storage__open_and_load(); + if (CHECK(!skel, "skel_load", "lsm skeleton failed\n")) + goto close_prog; + + err = local_storage__attach(skel); + if (CHECK(err, "attach", "lsm attach failed: %d\n", err)) + goto close_prog; + + skel->bss->monitored_pid = getpid(); + + err = create_and_unlink_file(); + if (CHECK(err < 0, "exec_cmd", "err %d errno %d\n", err, errno)) + goto close_prog; + + CHECK(!skel->bss->inode_storage_result, "inode_storage_result", + "inode_local_storage not set"); + + serv_sk = start_server(AF_INET6, SOCK_STREAM); + if (CHECK(serv_sk < 0, "start_server", "failed to start server\n")) + goto close_prog; + + CHECK(!skel->bss->sk_storage_result, "sk_storage_result", + "sk_local_storage not set"); + + close(serv_sk); + +close_prog: + local_storage__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/local_storage.c b/tools/testing/selftests/bpf/progs/local_storage.c new file mode 100644 index 000000000000..1aa4ccbe0369 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/local_storage.c @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright 2020 Google LLC. + */ + +#include <errno.h> +#include <linux/bpf.h> +#include <stdbool.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> +#include <bpf/bpf_endian.h> + +char _license[] SEC("license") = "GPL"; +__u32 _version SEC("version") = 1; + +#define DUMMY_STORAGE_VALUE 0xdeadbeef + +int monitored_pid = 0; +bool inode_storage_result = false; +bool sk_storage_result = false; + +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. + */ +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)); + + +SEC("lsm/inode_unlink") +int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct dummy_storage *storage; + + if (pid != monitored_pid) + return 0; + + storage = bpf_inode_storage_get(&inode_storage_map, victim->d_inode, 0, + BPF_SK_STORAGE_GET_F_CREATE); + if (!storage) + return 0; + + if (storage->value == DUMMY_STORAGE_VALUE) + inode_storage_result = true; + + return 0; +} + +SEC("lsm/socket_bind") +int BPF_PROG(socket_bind, struct socket *sock, struct sockaddr *address, + int addrlen) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct dummy_storage *storage; + + if (pid != monitored_pid) + return 0; + + storage = bpf_sk_storage_get(&sk_storage_map, sock->sk, 0, + BPF_SK_STORAGE_GET_F_CREATE); + if (!storage) + return 0; + + if (storage->value == DUMMY_STORAGE_VALUE) + sk_storage_result = true; + + return 0; +} + +SEC("lsm/socket_post_create") +int BPF_PROG(socket_post_create, struct socket *sock, int family, int type, + int protocol, int kern) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct dummy_storage *storage; + + if (pid != monitored_pid) + return 0; + + storage = bpf_sk_storage_get(&sk_storage_map, sock->sk, 0, + BPF_SK_STORAGE_GET_F_CREATE); + if (!storage) + return 0; + + storage->value = DUMMY_STORAGE_VALUE; + + return 0; +} + +SEC("lsm/file_open") +int BPF_PROG(test_int_hook, struct file *file) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct dummy_storage *storage; + + if (pid != monitored_pid) + return 0; + + if (!file->f_inode) + return 0; + + storage = bpf_inode_storage_get(&inode_storage_map, file->f_inode, 0, + BPF_LOCAL_STORAGE_GET_F_CREATE); + if (!storage) + return 0; + + storage->value = DUMMY_STORAGE_VALUE; + return 0; +}