On Tue, Dec 21, 2021 at 4:24 PM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Dec 21, 2021 at 12:16 PM Hao Luo [off-list ref] wrote:

quoted

On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song [off-list ref] wrote:

quoted

On 12/20/21 12:12 PM, Hao Luo wrote:

quoted

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 <redacted>
---
  .../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);

Ack. Will change that.

no need, __destroy() handles NULLs and ERR_PTR just fine, the way you
wrote it is totally correct (that's a deliberate nice feature of
libbpf's "destructor" APIs)

Yep. That's also my understanding.

quoted

I don't know if it's only me: I find it confusing when figuring out
what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
!= NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
returns nothing.

You haven't looked at many other selftests, I presume. All the
ASSERT_xxx() macros return true/false depending whether the assertion
holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which
is NULL and ERR_PTR). If it's not, it returns false. So

if (!ASSERT_ERR_PTR(ptr, "short_descriptor"))
   /* do something if assertion failed */

is a common pattern.

Note also "short_descriptor", it's not supposed to be a long
descriptive sentences, it's sort of a "codename" of the particular
check. It's not illegal to use space-separated sentence, but better to
keep it short and identifier-like.

I see. Thanks for the explanation.

quoted

I noticed that xxx__destroy has a check for NULL, so I put the destroy
function unconditionally.

quoted

quoted

+}
+
+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.

Thanks. Will fix that.

quoted

quoted

+#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.

Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
cast 'active' to 'char *' somewhere, otherwise the compiler will issue
a warning. To combine with your comment, maybe the following:

int *active;
active = (int *)bpf_per_cpu_ptr(...);
...
bpf_d_path(path, (char *)active, sizeof(int));

why not `void *`?

'void *' works. Just haven't thought about that.

quoted

quoted

quoted

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

int *

quoted

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

'active' points to readonly memory.

Ack.

quoted

quoted

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