Hi,

On 9/17/2021 7:46 AM, Yonghong Song wrote:

On 9/16/21 6:55 AM, Hou Tao wrote:

quoted

Add a writable bare tracepoint in bpf_testmod module, and
trigger its calling when reading /sys/kernel/bpf_testmod
with a specific buffer length.

The patch cannot be applied cleanly with bpf-next tree.
Please rebase and resubmit.

Will do

quoted

Signed-off-by: Hou Tao <redacted>
---
  .../bpf/bpf_testmod/bpf_testmod-events.h      | 15 +++++++
  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 10 +++++
  .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  5 +++
  .../selftests/bpf/prog_tests/module_attach.c  | 40 ++++++++++++++++++-
  .../selftests/bpf/progs/test_module_attach.c  | 14 +++++++
  5 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h

b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
index 89c6d58e5dd6..11ee801e75e7 100644

--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h

@@ -34,6 +34,21 @@ DECLARE_TRACE(bpf_testmod_test_write_bare,

      TP_ARGS(task, ctx)
  );
  +#undef BPF_TESTMOD_DECLARE_TRACE
+#ifdef DECLARE_TRACE_WRITABLE
+#define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \
+    DECLARE_TRACE_WRITABLE(call, PARAMS(proto), PARAMS(args), size)
+#else
+#define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \
+    DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
+#endif
+
+BPF_TESTMOD_DECLARE_TRACE(bpf_testmod_test_writable_bare,
+    TP_PROTO(struct bpf_testmod_test_writable_ctx *ctx),
+    TP_ARGS(ctx),
+    sizeof(struct bpf_testmod_test_writable_ctx)
+);
+
  #endif /* _BPF_TESTMOD_EVENTS_H */
    #undef TRACE_INCLUDE_PATH

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c

b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 141d8da687d2..3d3fb16eaf8c 100644

--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c

@@ -26,6 +26,16 @@ bpf_testmod_test_read(struct file *file, struct kobject

*kobj,
        trace_bpf_testmod_test_read(current, &ctx);
  +    /* Magic number to enable writable tp */
+    if (len == 1024) {
+        struct bpf_testmod_test_writable_ctx writable = {
+            .val = 1024,
+        };
+        trace_bpf_testmod_test_writable_bare(&writable);
+        if (writable.ret)
+            return snprintf(buf, len, "%d\n", writable.val);
+    }
+
      return -EIO; /* always fail */
  }
  EXPORT_SYMBOL(bpf_testmod_test_read);

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h

b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index b3892dc40111..553d94214aa6 100644

--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h

@@ -17,4 +17,9 @@ struct bpf_testmod_test_write_ctx {

      size_t len;
  };
  +struct bpf_testmod_test_writable_ctx {
+    bool ret;
+    int val;
+};
+
  #endif /* _BPF_TESTMOD_H */

diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c

b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index d85a69b7ce44..5565bcab1531 100644

--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c

@@ -6,11 +6,39 @@

    static int duration;
  +#define BPF_TESTMOD_PATH "/sys/kernel/bpf_testmod"
+
+static int trigger_module_test_writable(int *val)
+{
+    int fd, err;
+    char buf[1025];

Not critical, but do you need such a big stack size?
Maybe smaller one?

65 is also fine.

quoted

+    ssize_t rd;
+
+    fd = open(BPF_TESTMOD_PATH, O_RDONLY);
+    err = -errno;
+    if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
+        return err;
+
+    rd = read(fd, buf, sizeof(buf) - 1);
+    err = rd < 0 ? -errno : -EIO;
+    if (CHECK(rd <= 0, "testmod_file_read", "failed: %d\n", err)) {

Please use ASSERT_* macros. You can take a look at other self tests.

The reason using CHECK instead of ASSERT is we can output the errno
if read() fails.

quoted

+        close(fd);
+        return err;
+    }

Put one blank line here and remove the following three blank lines.

Will do.

quoted

+    buf[rd] = '\0';
+
+    *val = strtol(buf, NULL, 0);
+
+    close(fd);
+
+    return 0;
+}
+
  static int trigger_module_test_read(int read_sz)
  {
      int fd, err;
  -    fd = open("/sys/kernel/bpf_testmod", O_RDONLY);
+    fd = open(BPF_TESTMOD_PATH, O_RDONLY);
      err = -errno;
      if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
          return err;

@@ -32,7 +60,7 @@ static int trigger_module_test_write(int write_sz)

      memset(buf, 'a', write_sz);
      buf[write_sz-1] = '\0';
  -    fd = open("/sys/kernel/bpf_testmod", O_WRONLY);
+    fd = open(BPF_TESTMOD_PATH, O_WRONLY);
      err = -errno;
      if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) {
          free(buf);

@@ -58,6 +86,7 @@ void test_module_attach(void)

      struct test_module_attach__bss *bss;
      struct bpf_link *link;
      int err;
+    int writable_val;
        skel = test_module_attach__open();
      if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))

@@ -90,6 +119,13 @@ void test_module_attach(void)

      ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet");
      ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret");
  +    bss->raw_tp_writable_bare_ret = 1;
+    bss->raw_tp_writable_bare_val = 511;
+    writable_val = 0;
+    ASSERT_OK(trigger_module_test_writable(&writable_val), "trigger_writable");
+    ASSERT_EQ(bss->raw_tp_writable_bare_in_val, 1024, "writable_test");
+    ASSERT_EQ(bss->raw_tp_writable_bare_val, writable_val, "writable_test");
+
      test_module_attach__detach(skel);
        /* attach fentry/fexit and make sure it get's module reference */

diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c

b/tools/testing/selftests/bpf/progs/test_module_attach.c
index bd37ceec5587..4f5c780fcd21 100644

--- a/tools/testing/selftests/bpf/progs/test_module_attach.c
+++ b/tools/testing/selftests/bpf/progs/test_module_attach.c

@@ -27,6 +27,20 @@ int BPF_PROG(handle_raw_tp_bare,

      return 0;
  }
  +int raw_tp_writable_bare_in_val = 0;
+int raw_tp_writable_bare_ret = 0;
+int raw_tp_writable_bare_val = 0;
+
+SEC("raw_tp_writable/bpf_testmod_test_writable_bare")
+int BPF_PROG(handle_raw_tp_writable_bare,
+         struct bpf_testmod_test_writable_ctx *writable)
+{
+    raw_tp_writable_bare_in_val = writable->val;
+    writable->ret = raw_tp_writable_bare_ret;
+    writable->val = raw_tp_writable_bare_val;
+    return 0;
+}
+
  __u32 tp_btf_read_sz = 0;
    SEC("tp_btf/bpf_testmod_test_read")

.