| Message ID | 20200825182919.1118197-1-kpsingh@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Generalizing bpf_local_storage | expand |
On Tue, Aug 25, 2020 at 11:29 AM KP Singh <kpsingh@chromium.org> wrote: > > From: KP Singh <kpsingh@google.com> > > # v9 -> v10 > > - Added NULL check for inode_storage_ptr before calling > bpf_local_storage_update > - Removed an extraneous include > - Rebased and added Acks / Signoff. Hmm. Though it looks good I cannot apply it, because test_progs -t map_ptr is broken: 2225: (18) r2 = 0xffffc900004e5004 2227: (b4) w1 = 58 2228: (63) *(u32 *)(r2 +0) = r1 R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) R1_w=inv58 R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3=inv49 R4=inv63 R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 R7=invP8 R8=map_ptr(id=0,off=0,ks=4,vs=4,imm=0) R10=? ; VERIFY_TYPE(BPF_MAP_TYPE_SK_STORAGE, check_sk_storage); 2229: (18) r1 = 0xffffc900004e5000 2231: (b4) w3 = 24 2232: (63) *(u32 *)(r1 +0) = r3 R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0) R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3_w=inv24 R4=inv63 R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 R7=invP8 R8=map_pt? 2233: (18) r3 = 0xffff8881f03f7000 ; VERIFY(indirect->map_type == direct->map_type); 2235: (85) call unknown#195896080 invalid func unknown#195896080 processed 4678 insns (limit 1000000) max_states_per_insn 9 total_states 240 peak_states 178 mark_read 11 libbpf: -- END LOG -- libbpf: failed to load program 'cgroup_skb/egress' libbpf: failed to load object 'map_ptr_kern' libbpf: failed to load BPF skeleton 'map_ptr_kern': -4007 test_map_ptr:FAIL:skel_open_load open_load failed #43 map_ptr:FAIL Above 'invalid func unknown#195896080' happens when libbpf fails to do a relocation at runtime. Please debug. It's certainly caused by this set, but not sure why.
On Tue, Aug 25, 2020 at 2:05 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 11:29 AM KP Singh <kpsingh@chromium.org> wrote: > > > > From: KP Singh <kpsingh@google.com> > > > > # v9 -> v10 > > > > - Added NULL check for inode_storage_ptr before calling > > bpf_local_storage_update > > - Removed an extraneous include > > - Rebased and added Acks / Signoff. > > Hmm. Though it looks good I cannot apply it, because > test_progs -t map_ptr > is broken: > 2225: (18) r2 = 0xffffc900004e5004 > 2227: (b4) w1 = 58 > 2228: (63) *(u32 *)(r2 +0) = r1 > R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) R1_w=inv58 > R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3=inv49 R4=inv63 > R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 > R7=invP8 R8=map_ptr(id=0,off=0,ks=4,vs=4,imm=0) R10=? > ; VERIFY_TYPE(BPF_MAP_TYPE_SK_STORAGE, check_sk_storage); > 2229: (18) r1 = 0xffffc900004e5000 > 2231: (b4) w3 = 24 > 2232: (63) *(u32 *)(r1 +0) = r3 > R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) > R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0) > R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3_w=inv24 R4=inv63 > R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 > R7=invP8 R8=map_pt? > 2233: (18) r3 = 0xffff8881f03f7000 > ; VERIFY(indirect->map_type == direct->map_type); > 2235: (85) call unknown#195896080 > invalid func unknown#195896080 > processed 4678 insns (limit 1000000) max_states_per_insn 9 > total_states 240 peak_states 178 mark_read 11 > > libbpf: -- END LOG -- > libbpf: failed to load program 'cgroup_skb/egress' > libbpf: failed to load object 'map_ptr_kern' > libbpf: failed to load BPF skeleton 'map_ptr_kern': -4007 > test_map_ptr:FAIL:skel_open_load open_load failed > #43 map_ptr:FAIL > > Above 'invalid func unknown#195896080' happens > when libbpf fails to do a relocation at runtime. > Please debug. > It's certainly caused by this set, but not sure why. So I've ended up bisecting and debugging it. It turned out that the patch 1 was responsible. I've added the following hunk to fix it: diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c index 473665cac67e..982a2d8aa844 100644 --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c @@ -589,7 +589,7 @@ static inline int check_stack(void) return 1; } -struct bpf_sk_storage_map { +struct bpf_local_storage_map { struct bpf_map map; } __attribute__((preserve_access_index)); @@ -602,8 +602,8 @@ struct { static inline int check_sk_storage(void) { - struct bpf_sk_storage_map *sk_storage = - (struct bpf_sk_storage_map *)&m_sk_storage; + struct bpf_local_storage_map *sk_storage = + (struct bpf_local_storage_map *)&m_sk_storage; struct bpf_map *map = (struct bpf_map *)&m_sk_storage; and pushed the whole set. In the future please always run test_progs and test_progs-no_alu32 for every patch and submit patches only if _all_ tests are passing. Do not assume that your change is not responsible for breakage.
On Wed, Aug 26, 2020 at 12:13 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 2:05 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Aug 25, 2020 at 11:29 AM KP Singh <kpsingh@chromium.org> wrote: > > > > > > From: KP Singh <kpsingh@google.com> > > > > > > # v9 -> v10 > > > > > > - Added NULL check for inode_storage_ptr before calling > > > bpf_local_storage_update > > > - Removed an extraneous include > > > - Rebased and added Acks / Signoff. > > > > Hmm. Though it looks good I cannot apply it, because > > test_progs -t map_ptr > > is broken: > > 2225: (18) r2 = 0xffffc900004e5004 > > 2227: (b4) w1 = 58 > > 2228: (63) *(u32 *)(r2 +0) = r1 > > R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) R1_w=inv58 > > R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3=inv49 R4=inv63 > > R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 > > R7=invP8 R8=map_ptr(id=0,off=0,ks=4,vs=4,imm=0) R10=? > > ; VERIFY_TYPE(BPF_MAP_TYPE_SK_STORAGE, check_sk_storage); > > 2229: (18) r1 = 0xffffc900004e5000 > > 2231: (b4) w3 = 24 > > 2232: (63) *(u32 *)(r1 +0) = r3 > > R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) > > R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0) > > R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3_w=inv24 R4=inv63 > > R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0 > > R7=invP8 R8=map_pt? > > 2233: (18) r3 = 0xffff8881f03f7000 > > ; VERIFY(indirect->map_type == direct->map_type); > > 2235: (85) call unknown#195896080 > > invalid func unknown#195896080 > > processed 4678 insns (limit 1000000) max_states_per_insn 9 > > total_states 240 peak_states 178 mark_read 11 > > > > libbpf: -- END LOG -- > > libbpf: failed to load program 'cgroup_skb/egress' > > libbpf: failed to load object 'map_ptr_kern' > > libbpf: failed to load BPF skeleton 'map_ptr_kern': -4007 > > test_map_ptr:FAIL:skel_open_load open_load failed > > #43 map_ptr:FAIL > > > > Above 'invalid func unknown#195896080' happens > > when libbpf fails to do a relocation at runtime. > > Please debug. > > It's certainly caused by this set, but not sure why. > > So I've ended up bisecting and debugging it. > It turned out that the patch 1 was responsible. > I've added the following hunk to fix it: Thanks for fixing and debugging it. > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > index 473665cac67e..982a2d8aa844 100644 > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > @@ -589,7 +589,7 @@ static inline int check_stack(void) > return 1; [...] > and pushed the whole set. > In the future please always run test_progs and test_progs-no_alu32 Noted, I do run them but this test gave me a different error and I always ended up ignoring this: ./test_progs -t map_ptr libbpf: Error in bpf_create_map_xattr(m_array_of_maps):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_hash_of_maps):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_perf_event_array):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_stack_trace):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_cgroup_array):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_devmap):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. libbpf: Error in bpf_create_map_xattr(m_sockmap):Invalid argument(-22). Retrying without BTF. libbpf: map 'm_sockmap': failed to create: Invalid argument(-22) libbpf: failed to load object 'map_ptr_kern' libbpf: failed to load BPF skeleton 'map_ptr_kern': -22 test_map_ptr:FAIL:skel_open_load open_load failed I now realized that I was not sourcing tools/testing/selftests/bpf/config correctly and CONFIG_BPF_STREAM_PARSER was not enabled in my configuration. Nonetheless, no excuses and will ensure these tests pass in the future. - KP > for every patch and submit patches only if _all_ tests are passing. > Do not assume that your change is not responsible for breakage.
From: KP Singh <kpsingh@google.com> # v9 -> v10 - Added NULL check for inode_storage_ptr before calling bpf_local_storage_update - Removed an extraneous include - Rebased and added Acks / Signoff. # v8 -> v9 - Fixed reference count logic for files for inode maps. - Other fixes suggested by Martin - Rebase # v7 -> v8 - Fixed an issue with BTF IDs for helpers and added bpf_<>_storage_delete to selftests to catch this issue. - Update comments about refcounts and grabbed a refcount to the open file for userspace inode helpers. - Rebase. # v6 -> v7 - Updated the series to use Martin's POC patch: https://lore.kernel.org/bpf/20200725013047.4006241-1-kafai@fb.com/ I added a Co-developed-by: tag, but would need Martin's Signoff (was not sure of the procedure here). - Rebase. # v5 -> v6 - Fixed a build warning. - Rebase. # v4 -> v5 - Split non-functional changes into separate commits. - Updated the cache macros to be simpler. - Fixed some bugs noticed by Martin. - Updated the userspace map functions to use an fd for lookups, updates and deletes. - Rebase. # v3 -> v4 - Fixed a missing include to bpf_sk_storage.h in bpf_sk_storage.c - Fixed some functions that were not marked as static which led to W=1 compilation warnings. # v2 -> v3 * Restructured the code as per Martin's suggestions: - Common functionality in bpf_local_storage.c - bpf_sk_storage functionality remains in net/bpf_sk_storage. - bpf_inode_storage is kept separate as it is enabled only with CONFIG_BPF_LSM. * A separate cache for inode and sk storage with macros to define it. * Use the ops style approach as suggested by Martin instead of the enum + switch style. * Added the inode map to bpftool bash completion and docs. * Rebase and indentation fixes. # v1 -> v2 * Use the security blob pointer instead of dedicated member in struct inode. * Better code re-use as suggested by Alexei. * Dropped the inode count arithmetic as pointed out by Alexei. * Minor bug fixes and rebase. bpf_sk_storage can already be used by some BPF program types to annotate socket objects. These annotations are managed with the life-cycle of the object (i.e. freed when the object is freed) which makes BPF programs much simpler and less prone to errors and leaks. This patch series: * Generalizes the bpf_sk_storage infrastructure to allow easy implementation of local storage for other objects * Implements local storage for inodes * Makes both bpf_{sk, inode}_storage available to LSM programs. Local storage is safe to use in LSM programs as the attachment sites are limited and the owning object won't be freed, however, this is not the case for tracing. Usage in tracing is expected to follow a white-list based approach similar to the d_path helper (https://lore.kernel.org/bpf/20200506132946.2164578-1-jolsa@kernel.org). Access to local storage would allow LSM programs to implement stateful detections like detecting the unlink of a running executable from the examples shared as a part of the KRSI series https://lore.kernel.org/bpf/20200329004356.27286-1-kpsingh@chromium.org/ and https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c KP Singh (7): bpf: Renames in preparation for bpf_local_storage bpf: Generalize caching for sk_storage. bpf: Generalize bpf_sk_storage bpf: Split bpf_local_storage to bpf_sk_storage bpf: Implement bpf_local_storage for inodes bpf: Allow local storage to be used from LSM programs bpf: Add selftests for local_storage include/linux/bpf.h | 8 + include/linux/bpf_local_storage.h | 163 ++++ include/linux/bpf_lsm.h | 29 + include/linux/bpf_types.h | 3 + include/net/bpf_sk_storage.h | 14 + include/net/sock.h | 4 +- include/uapi/linux/bpf.h | 55 +- kernel/bpf/Makefile | 2 + kernel/bpf/bpf_inode_storage.c | 273 ++++++ kernel/bpf/bpf_local_storage.c | 600 +++++++++++++ kernel/bpf/bpf_lsm.c | 21 +- kernel/bpf/syscall.c | 3 +- kernel/bpf/verifier.c | 10 + net/core/bpf_sk_storage.c | 830 +++--------------- security/bpf/hooks.c | 6 + .../bpf/bpftool/Documentation/bpftool-map.rst | 2 +- tools/bpf/bpftool/bash-completion/bpftool | 3 +- tools/bpf/bpftool/map.c | 3 +- tools/include/uapi/linux/bpf.h | 55 +- tools/lib/bpf/libbpf_probes.c | 5 +- .../bpf/prog_tests/test_local_storage.c | 60 ++ .../selftests/bpf/progs/local_storage.c | 140 +++ 22 files changed, 1575 insertions(+), 714 deletions(-) create mode 100644 include/linux/bpf_local_storage.h create mode 100644 kernel/bpf/bpf_inode_storage.c create mode 100644 kernel/bpf/bpf_local_storage.c create mode 100644 tools/testing/selftests/bpf/prog_tests/test_local_storage.c create mode 100644 tools/testing/selftests/bpf/progs/local_storage.c