On Mon, Nov 20, 2023 at 9:59 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> This patch tests mmapable task_local storage functionality added earlier
> in the series. The success tests focus on verifying correctness of the
> various ways of reading from and writing to mmapable task_local storage:
>
> * Write through mmap'd region should be visible when BPF program
> makes bpf_task_storage_get call
> * If BPF program reads-and-incrs the mapval, the new value should be
> visible when userspace reads from mmap'd region or does
> map_lookup_elem call
> * If userspace does map_update_elem call, new value should be visible
> when userspace reads from mmap'd region or does map_lookup_elem
> call
> * After bpf_map_delete_elem, reading from mmap'd region should still
> succeed, but map_lookup_elem w/o BPF_LOCAL_STORAGE_GET_F_CREATE flag
> should fail
> * After bpf_map_delete_elem, creating a new map_val via mmap call
> should return a different memory region
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> ---
> .../bpf/prog_tests/task_local_storage.c | 177 ++++++++++++++++++
> .../bpf/progs/task_local_storage__mmap.c | 59 ++++++
> .../bpf/progs/task_local_storage__mmap.h | 7 +
> .../bpf/progs/task_local_storage__mmap_fail.c | 39 ++++
> 4 files changed, 282 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/task_local_storage__mmap.c
> create mode 100644 tools/testing/selftests/bpf/progs/task_local_storage__mmap.h
> create mode 100644 tools/testing/selftests/bpf/progs/task_local_storage__mmap_fail.c
>
[...]
> diff --git a/tools/testing/selftests/bpf/progs/task_local_storage__mmap.c b/tools/testing/selftests/bpf/progs/task_local_storage__mmap.c
> new file mode 100644
> index 000000000000..1c8850c8d189
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/task_local_storage__mmap.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "task_local_storage__mmap.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
> + __uint(map_flags, BPF_F_NO_PREALLOC | BPF_F_MMAPABLE);
> + __type(key, int);
> + __type(value, long);
> +} mmapable SEC(".maps");
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
> + __uint(map_flags, BPF_F_NO_PREALLOC | BPF_F_MMAPABLE);
> + __type(key, int);
> + __type(value, struct two_page_struct);
> +} mmapable_two_pages SEC(".maps");
> +
> +long mmaped_mapval = 0;
> +int read_and_incr = 0;
> +int create_flag = 0;
> +int use_big_mapval = 0;
> +
> +SEC("tp_btf/sys_enter")
> +int BPF_PROG(on_enter, struct pt_regs *regs, long id)
> +{
> + struct two_page_struct *big_mapval;
> + struct task_struct *task;
> + long *ptr;
> +
In addition to the note below about idempotency as a desired property,
it might be good to *also* add tid/pid filter here to only execute the
logic for our process?
> + task = bpf_get_current_task_btf();
> + if (!task)
> + return 1;
> +
> + if (use_big_mapval) {
> + big_mapval = bpf_task_storage_get(&mmapable_two_pages, task, 0,
> + create_flag);
> + if (!big_mapval)
> + return 2;
> + ptr = &big_mapval->val;
> + } else {
> + ptr = bpf_task_storage_get(&mmapable, task, 0, create_flag);
> + }
> +
> + if (!ptr)
> + return 3;
> +
> + if (read_and_incr)
> + *ptr = *ptr + 1;
this seems fragile, this program is not idempotent, which means any
extra unexpected syscall might cause problem, right?
> +
> + mmaped_mapval = *ptr;
> + return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/task_local_storage__mmap.h b/tools/testing/selftests/bpf/progs/task_local_storage__mmap.h
> new file mode 100644
> index 000000000000..f4a3264142c2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/task_local_storage__mmap.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +
> +struct two_page_struct {
> + long val;
> + char c[4097];
> +};
> diff --git a/tools/testing/selftests/bpf/progs/task_local_storage__mmap_fail.c b/tools/testing/selftests/bpf/progs/task_local_storage__mmap_fail.c
> new file mode 100644
> index 000000000000..f32c5bfe370a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/task_local_storage__mmap_fail.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
> + __uint(map_flags, BPF_F_NO_PREALLOC | BPF_F_MMAPABLE);
> + __type(key, int);
> + __type(value, long);
> +} mmapable SEC(".maps");
> +
> +__failure __msg("invalid access to map value, value_size=8 off=8 size=8")
> +SEC("tp_btf/sys_enter")
let's keep SEC() first, please move __failure and __msg below it
> +long BPF_PROG(fail_read_past_mapval_end, struct pt_regs *regs, long id)
> +{
> + struct task_struct *task;
> + long *ptr;
> + long res;
> +
> + task = bpf_get_current_task_btf();
> + if (!task)
> + return 1;
> +
> + ptr = bpf_task_storage_get(&mmapable, task, 0, 0);
> + if (!ptr)
> + return 3;
> + /* Although mmapable mapval is given an entire page, verifier shouldn't
> + * allow r/w past end of 'long' type
> + */
> + res = *(ptr + 1);
> +
> + return res;
> +}
> --
> 2.34.1
>
