Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.

[Yonghong Song <yhs@xxxxxx>]

On 12/20/21 12:12 PM, Hao Luo wrote:
> The second parameter of bpf_d_path() can only accept writable
> memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> be passed into bpf_d_path for modification. This patch adds
> a selftest to verify this behavior.
>
> Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> ---
>   .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
>   .../bpf/progs/test_d_path_check_rdonly_mem.c  | 30 +++++++++++++++++++
>   2 files changed, 51 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> index 0a577a248d34..f8d8c5a5dfba 100644
> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> @@ -9,6 +9,7 @@
>   #define MAX_FILES		7
>   
>   #include "test_d_path.skel.h"
> +#include "test_d_path_check_rdonly_mem.skel.h"
>   
>   static int duration;
>   
> @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
>   	return ret;
>   }
>   
> -void test_d_path(void)
> +static void test_d_path_basic(void)
>   {
>   	struct test_d_path__bss *bss;
>   	struct test_d_path *skel;
> @@ -155,3 +156,22 @@ void test_d_path(void)
>   cleanup:
>   	test_d_path__destroy(skel);
>   }
> +
> +static void test_d_path_check_rdonly_mem(void)
> +{
> +	struct test_d_path_check_rdonly_mem *skel;
> +
> +	skel = test_d_path_check_rdonly_mem__open_and_load();
> +	ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> +
> +	test_d_path_check_rdonly_mem__destroy(skel);

You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel 
is an ERR_PTR. Maybe
	if (!ASSERT_ERR_PTR(...))
		test_d_path_check_rdonly_mem__destroy(skel);

> +}
> +
> +void test_d_path(void)
> +{
> +	if (test__start_subtest("basic"))
> +		test_d_path_basic();
> +
> +	if (test__start_subtest("check_rdonly_mem"))
> +		test_d_path_check_rdonly_mem();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> new file mode 100644
> index 000000000000..c7a9655d5850
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Google */
> +
> +#include "vmlinux.h"
> +
> +#include "vmlinux.h"

duplicated vmlinux.h.

> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +extern const int bpf_prog_active __ksym;
> +
> +SEC("fentry/security_inode_getattr")
> +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> +	     __u32 request_mask, unsigned int query_flags)
> +{
> +	char *active;

int *active?
It may not matter since the program is rejected by the kernel but
with making it conforms to kernel definition we have one less thing
to worry about the verification.

> +	__u32 cpu;
> +
> +	cpu = bpf_get_smp_processor_id();
> +	active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);

int *

> +	if (active) {
> +		/* FAIL here! 'active' is a rdonly_mem. bpf helpers that

'active' points to readonly memory.

> +		 * update its arguments can not write into it.
> +		 */
> +		bpf_d_path(path, active, sizeof(int));
> +	}
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";