[PATCH bpf-next v2 0/4] bpf: Fix warnings in kvmalloc_node()

[PATCH bpf-next v2 0/4] bpf: Fix warnings in kvmalloc_node()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The patch set aims to fix the warnings in kvmalloc_node() when passing
an abnormally big cnt during multiple kprobes/uprobes attachment.

Patch #1 and #2 fix the warning by limiting the maximal number of
uprobes/kprobes. Patch #3 and #4 add tests to ensure these warnings are
fixed.

Please see individual patches for more details. Comments are always
welcome.

Change Log:
v2:
  * limit the number of uprobes/kprobes instead of suppressing the
    out-of-memory warning message (Alexei)
  * provide a faked non-zero offsets to simplify the multiple uprobe
    test (Jiri)

v1: https://lore.kernel.org/bpf/20231211112843.4147157-1-houtao@huaweicloud.com/
  
Hou Tao (4):
  bpf: Limit the number of uprobes when attaching program to multiple
    uprobes
  bpf: Limit the number of kprobes when attaching program to multiple
    kprobes
  selftests/bpf: Add test for abnormal cnt during multi-uprobe
    attachment
  selftests/bpf: Add test for abnormal cnt during multi-kprobe
    attachment

 kernel/trace/bpf_trace.c                      |  7 ++--
 .../bpf/prog_tests/kprobe_multi_test.c        | 14 ++++++++
 .../bpf/prog_tests/uprobe_multi_test.c        | 33 ++++++++++++++++++-
 3 files changed, 51 insertions(+), 3 deletions(-)

-- 
2.29.2

[PATCH bpf-next v2 1/4] bpf: Limit the number of uprobes when attaching program to multiple uprobes

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

An abnormally big cnt may be passed to link_create.uprobe_multi.cnt,
and it will trigger the following warning in kvmalloc_node():

	if (unlikely(size > INT_MAX)) {
		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
		return NULL;
	}

Fix the warning by limiting the maximal number of uprobes in
bpf_uprobe_multi_link_attach().

Fixes: 89ae89f53d20 ("bpf: Add multi uprobe link")
Reported-by: xingwei lee <xrivendell7@gmail.com>
Closes: https://lore.kernel.org/bpf/CABOYnLwwJY=yFAGie59LFsUsBAgHfroVqbzZ5edAXbFE3YiNVA@mail.gmail.com
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/trace/bpf_trace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 774cf476a892..2d1201f7b554 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -42,6 +42,8 @@
 #define bpf_event_rcu_dereference(p)					\
 	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
 
+#define MAX_UPROBE_MULTI_CNT (1U << 20)
+
 #ifdef CONFIG_MODULES
 struct bpf_trace_module {
 	struct module *module;
@@ -3342,7 +3344,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	uoffsets = u64_to_user_ptr(attr->link_create.uprobe_multi.offsets);
 	cnt = attr->link_create.uprobe_multi.cnt;
 
-	if (!upath || !uoffsets || !cnt)
+	if (!upath || !uoffsets || !cnt || cnt > MAX_UPROBE_MULTI_CNT)
 		return -EINVAL;
 
 	uref_ctr_offsets = u64_to_user_ptr(attr->link_create.uprobe_multi.ref_ctr_offsets);
-- 
2.29.2

[PATCH bpf-next v2 2/4] bpf: Limit the number of kprobes when attaching program to multiple kprobes

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

An abnormally big cnt may also be assigned to kprobe_multi.cnt when
attaching multiple kprobes. It will trigger the following warning in
kvmalloc_node():

	if (unlikely(size > INT_MAX)) {
	    WARN_ON_ONCE(!(flags & __GFP_NOWARN));
	    return NULL;
	}

Fix the warning by limiting the maximal number of kprobes in
bpf_kprobe_multi_link_attach().

Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/trace/bpf_trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 2d1201f7b554..944678529f5c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -43,6 +43,7 @@
 	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
 
 #define MAX_UPROBE_MULTI_CNT (1U << 20)
+#define MAX_KPROBE_MULTI_CNT (1U << 20)
 
 #ifdef CONFIG_MODULES
 struct bpf_trace_module {
@@ -2970,7 +2971,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		return -EINVAL;
 
 	cnt = attr->link_create.kprobe_multi.cnt;
-	if (!cnt)
+	if (!cnt || cnt > MAX_KPROBE_MULTI_CNT)
 		return -EINVAL;
 
 	size = cnt * sizeof(*addrs);
-- 
2.29.2

[PATCH bpf-next v2 3/4] selftests/bpf: Add test for abnormal cnt during multi-uprobe attachment

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

If an abnormally huge cnt is used for multi-uprobes attachment, the
following warning will be reported:

  ------------[ cut here ]------------
  WARNING: CPU: 7 PID: 406 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
  Modules linked in: bpf_testmod(O)
  CPU: 7 PID: 406 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
  RIP: 0010:kvmalloc_node+0xd9/0xe0
  ......
  Call Trace:
   <TASK>
   ? __warn+0x89/0x150
   ? kvmalloc_node+0xd9/0xe0
   bpf_uprobe_multi_link_attach+0x14a/0x480
   __sys_bpf+0x14a9/0x2bc0
   do_syscall_64+0x36/0xb0
   entry_SYSCALL_64_after_hwframe+0x6e/0x76
   ......
   </TASK>
  ---[ end trace 0000000000000000 ]---

So add a test to ensure the warning is fixed.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../bpf/prog_tests/uprobe_multi_test.c        | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index ece260cf2c0b..0d2a4510e6cf 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -234,6 +234,35 @@ static void test_attach_api_syms(void)
 	test_attach_api("/proc/self/exe", NULL, &opts);
 }
 
+static void test_failed_link_api(void)
+{
+	LIBBPF_OPTS(bpf_link_create_opts, opts);
+	const char *path = "/proc/self/exe";
+	struct uprobe_multi *skel = NULL;
+	unsigned long offset = 0;
+	int link_fd = -1;
+
+	skel = uprobe_multi__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
+		goto cleanup;
+
+	/* abnormal cnt */
+	opts.uprobe_multi.path = path;
+	opts.uprobe_multi.offsets = &offset;
+	opts.uprobe_multi.cnt = INT_MAX;
+	opts.kprobe_multi.flags = 0;
+	link_fd = bpf_link_create(bpf_program__fd(skel->progs.uprobe), 0,
+				  BPF_TRACE_UPROBE_MULTI, &opts);
+	if (!ASSERT_ERR(link_fd, "link_fd"))
+		goto cleanup;
+	if (!ASSERT_EQ(link_fd, -EINVAL, "invalid cnt"))
+		goto cleanup;
+cleanup:
+	if (link_fd >= 0)
+		close(link_fd);
+	uprobe_multi__destroy(skel);
+}
+
 static void __test_link_api(struct child *child)
 {
 	int prog_fd, link1_fd = -1, link2_fd = -1, link3_fd = -1, link4_fd = -1;
@@ -311,7 +340,7 @@ static void __test_link_api(struct child *child)
 	free(offsets);
 }
 
-void test_link_api(void)
+static void test_link_api(void)
 {
 	struct child *child;
 
@@ -408,6 +437,8 @@ void test_uprobe_multi_test(void)
 		test_attach_api_syms();
 	if (test__start_subtest("link_api"))
 		test_link_api();
+	if (test__start_subtest("failed_link_api"))
+		test_failed_link_api();
 	if (test__start_subtest("bench_uprobe"))
 		test_bench_attach_uprobe();
 	if (test__start_subtest("bench_usdt"))
-- 
2.29.2

[PATCH bpf-next v2 4/4] selftests/bpf: Add test for abnormal cnt during multi-kprobe attachment

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

If an abnormally huge cnt is used for multi-kprobes attachment, the
following warning will be reported:

  ------------[ cut here ]------------
  WARNING: CPU: 1 PID: 392 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
  Modules linked in: bpf_testmod(O)
  CPU: 1 PID: 392 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
  ......
  RIP: 0010:kvmalloc_node+0xd9/0xe0
   ? __warn+0x89/0x150
   ? kvmalloc_node+0xd9/0xe0
   bpf_kprobe_multi_link_attach+0x87/0x670
   __sys_bpf+0x2a28/0x2bc0
   __x64_sys_bpf+0x1a/0x30
   do_syscall_64+0x36/0xb0
   entry_SYSCALL_64_after_hwframe+0x6e/0x76
  RIP: 0033:0x7fbe067f0e0d
  ......
   </TASK>
  ---[ end trace 0000000000000000 ]---

So add a test to ensure the warning is fixed.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/kprobe_multi_test.c   | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 4041cfa670eb..802554d4ee24 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -300,6 +300,20 @@ static void test_attach_api_fails(void)
 	if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_5_error"))
 		goto cleanup;
 
+	/* fail_6 - abnormal cnt */
+	opts.addrs = (const unsigned long *) addrs;
+	opts.syms = NULL;
+	opts.cnt = INT_MAX;
+	opts.cookies = NULL;
+
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
+						     NULL, &opts);
+	if (!ASSERT_ERR_PTR(link, "fail_6"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_6_error"))
+		goto cleanup;
+
 cleanup:
 	bpf_link__destroy(link);
 	kprobe_multi__destroy(skel);
-- 
2.29.2

Re: [PATCH bpf-next v2 0/4] bpf: Fix warnings in kvmalloc_node()

[Jiri Olsa]

On Wed, Dec 13, 2023 at 07:25:27PM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Hi,
> 
> The patch set aims to fix the warnings in kvmalloc_node() when passing
> an abnormally big cnt during multiple kprobes/uprobes attachment.
> 
> Patch #1 and #2 fix the warning by limiting the maximal number of
> uprobes/kprobes. Patch #3 and #4 add tests to ensure these warnings are
> fixed.
> 
> Please see individual patches for more details. Comments are always
> welcome.
> 
> Change Log:
> v2:
>   * limit the number of uprobes/kprobes instead of suppressing the
>     out-of-memory warning message (Alexei)
>   * provide a faked non-zero offsets to simplify the multiple uprobe
>     test (Jiri)
> 
> v1: https://lore.kernel.org/bpf/20231211112843.4147157-1-houtao@huaweicloud.com/
>   
> Hou Tao (4):
>   bpf: Limit the number of uprobes when attaching program to multiple
>     uprobes
>   bpf: Limit the number of kprobes when attaching program to multiple
>     kprobes
>   selftests/bpf: Add test for abnormal cnt during multi-uprobe
>     attachment
>   selftests/bpf: Add test for abnormal cnt during multi-kprobe
>     attachment
> 
>  kernel/trace/bpf_trace.c                      |  7 ++--
>  .../bpf/prog_tests/kprobe_multi_test.c        | 14 ++++++++
>  .../bpf/prog_tests/uprobe_multi_test.c        | 33 ++++++++++++++++++-
>  3 files changed, 51 insertions(+), 3 deletions(-)

with one minor comment, for the patchset

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
> -- 
> 2.29.2
> 

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for abnormal cnt during multi-uprobe attachment

[Jiri Olsa]

On Wed, Dec 13, 2023 at 07:25:30PM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> If an abnormally huge cnt is used for multi-uprobes attachment, the
> following warning will be reported:
> 
>   ------------[ cut here ]------------
>   WARNING: CPU: 7 PID: 406 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
>   Modules linked in: bpf_testmod(O)
>   CPU: 7 PID: 406 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
>   RIP: 0010:kvmalloc_node+0xd9/0xe0
>   ......
>   Call Trace:
>    <TASK>
>    ? __warn+0x89/0x150
>    ? kvmalloc_node+0xd9/0xe0
>    bpf_uprobe_multi_link_attach+0x14a/0x480
>    __sys_bpf+0x14a9/0x2bc0
>    do_syscall_64+0x36/0xb0
>    entry_SYSCALL_64_after_hwframe+0x6e/0x76
>    ......
>    </TASK>
>   ---[ end trace 0000000000000000 ]---
> 
> So add a test to ensure the warning is fixed.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  .../bpf/prog_tests/uprobe_multi_test.c        | 33 ++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> index ece260cf2c0b..0d2a4510e6cf 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -234,6 +234,35 @@ static void test_attach_api_syms(void)
>  	test_attach_api("/proc/self/exe", NULL, &opts);
>  }
>  
> +static void test_failed_link_api(void)
> +{
> +	LIBBPF_OPTS(bpf_link_create_opts, opts);
> +	const char *path = "/proc/self/exe";
> +	struct uprobe_multi *skel = NULL;
> +	unsigned long offset = 0;
> +	int link_fd = -1;
> +
> +	skel = uprobe_multi__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
> +		goto cleanup;
> +
> +	/* abnormal cnt */
> +	opts.uprobe_multi.path = path;
> +	opts.uprobe_multi.offsets = &offset;
> +	opts.uprobe_multi.cnt = INT_MAX;
> +	opts.kprobe_multi.flags = 0;

     s/k/u/  ^^^ .. or best just remove the line

jirka

> +	link_fd = bpf_link_create(bpf_program__fd(skel->progs.uprobe), 0,
> +				  BPF_TRACE_UPROBE_MULTI, &opts);
> +	if (!ASSERT_ERR(link_fd, "link_fd"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(link_fd, -EINVAL, "invalid cnt"))
> +		goto cleanup;
> +cleanup:
> +	if (link_fd >= 0)
> +		close(link_fd);
> +	uprobe_multi__destroy(skel);
> +}
> +
>  static void __test_link_api(struct child *child)
>  {
>  	int prog_fd, link1_fd = -1, link2_fd = -1, link3_fd = -1, link4_fd = -1;
> @@ -311,7 +340,7 @@ static void __test_link_api(struct child *child)
>  	free(offsets);
>  }
>  
> -void test_link_api(void)
> +static void test_link_api(void)
>  {
>  	struct child *child;
>  
> @@ -408,6 +437,8 @@ void test_uprobe_multi_test(void)
>  		test_attach_api_syms();
>  	if (test__start_subtest("link_api"))
>  		test_link_api();
> +	if (test__start_subtest("failed_link_api"))
> +		test_failed_link_api();
>  	if (test__start_subtest("bench_uprobe"))
>  		test_bench_attach_uprobe();
>  	if (test__start_subtest("bench_usdt"))
> -- 
> 2.29.2
> 

Re: [PATCH bpf-next v2 2/4] bpf: Limit the number of kprobes when attaching program to multiple kprobes

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> From: Hou Tao <houtao1@huawei.com>
>
> An abnormally big cnt may also be assigned to kprobe_multi.cnt when
> attaching multiple kprobes. It will trigger the following warning in
> kvmalloc_node():
>
>         if (unlikely(size > INT_MAX)) {
>             WARN_ON_ONCE(!(flags & __GFP_NOWARN));
>             return NULL;
>         }
>
> Fix the warning by limiting the maximal number of kprobes in
> bpf_kprobe_multi_link_attach().
>
> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  kernel/trace/bpf_trace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 2d1201f7b554..944678529f5c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -43,6 +43,7 @@
>         rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
>
>  #define MAX_UPROBE_MULTI_CNT (1U << 20)
> +#define MAX_KPROBE_MULTI_CNT (1U << 20)
>
>  #ifdef CONFIG_MODULES
>  struct bpf_trace_module {
> @@ -2970,7 +2971,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                 return -EINVAL;
>
>         cnt = attr->link_create.kprobe_multi.cnt;
> -       if (!cnt)
> +       if (!cnt || cnt > MAX_KPROBE_MULTI_CNT)
>                 return -EINVAL;

let's return -E2BIG for `cnt > MAX` cases? Same in another patch
>
>         size = cnt * sizeof(*addrs);
> --
> 2.29.2
>

Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add test for abnormal cnt during multi-kprobe attachment

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> From: Hou Tao <houtao1@huawei.com>
>
> If an abnormally huge cnt is used for multi-kprobes attachment, the
> following warning will be reported:
>
>   ------------[ cut here ]------------
>   WARNING: CPU: 1 PID: 392 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
>   Modules linked in: bpf_testmod(O)
>   CPU: 1 PID: 392 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>   ......
>   RIP: 0010:kvmalloc_node+0xd9/0xe0
>    ? __warn+0x89/0x150
>    ? kvmalloc_node+0xd9/0xe0
>    bpf_kprobe_multi_link_attach+0x87/0x670
>    __sys_bpf+0x2a28/0x2bc0
>    __x64_sys_bpf+0x1a/0x30
>    do_syscall_64+0x36/0xb0
>    entry_SYSCALL_64_after_hwframe+0x6e/0x76
>   RIP: 0033:0x7fbe067f0e0d
>   ......
>    </TASK>
>   ---[ end trace 0000000000000000 ]---
>
> So add a test to ensure the warning is fixed.
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  .../selftests/bpf/prog_tests/kprobe_multi_test.c   | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index 4041cfa670eb..802554d4ee24 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -300,6 +300,20 @@ static void test_attach_api_fails(void)
>         if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_5_error"))
>                 goto cleanup;
>
> +       /* fail_6 - abnormal cnt */
> +       opts.addrs = (const unsigned long *) addrs;
> +       opts.syms = NULL;
> +       opts.cnt = INT_MAX;
> +       opts.cookies = NULL;
> +
> +       link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
> +                                                    NULL, &opts);
> +       if (!ASSERT_ERR_PTR(link, "fail_6"))
> +               goto cleanup;
> +
> +       if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_6_error"))

this is unreliable, store errno right after the operation before
ASSERT_xxx() macros

> +               goto cleanup;
> +
>  cleanup:
>         bpf_link__destroy(link);
>         kprobe_multi__destroy(skel);
> --
> 2.29.2
>

Re: [PATCH bpf-next v2 2/4] bpf: Limit the number of kprobes when attaching program to multiple kprobes

[Hou Tao]

Hi,

On 12/14/2023 7:32 AM, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@huaweicloud.com> wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> An abnormally big cnt may also be assigned to kprobe_multi.cnt when
>> attaching multiple kprobes. It will trigger the following warning in
>> kvmalloc_node():
>>
>>         if (unlikely(size > INT_MAX)) {
>>             WARN_ON_ONCE(!(flags & __GFP_NOWARN));
>>             return NULL;
>>         }
>>
>> Fix the warning by limiting the maximal number of kprobes in
>> bpf_kprobe_multi_link_attach().
>>
>> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  kernel/trace/bpf_trace.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 2d1201f7b554..944678529f5c 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -43,6 +43,7 @@
>>         rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
>>
>>  #define MAX_UPROBE_MULTI_CNT (1U << 20)
>> +#define MAX_KPROBE_MULTI_CNT (1U << 20)
>>
>>  #ifdef CONFIG_MODULES
>>  struct bpf_trace_module {
>> @@ -2970,7 +2971,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>                 return -EINVAL;
>>
>>         cnt = attr->link_create.kprobe_multi.cnt;
>> -       if (!cnt)
>> +       if (!cnt || cnt > MAX_KPROBE_MULTI_CNT)
>>                 return -EINVAL;
> let's return -E2BIG for `cnt > MAX` cases? Same in another patch

Good point. Will do in v3.
>>         size = cnt * sizeof(*addrs);
>> --
>> 2.29.2
>>

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add test for abnormal cnt during multi-uprobe attachment

[Hou Tao]

On 12/13/2023 10:43 PM, Jiri Olsa wrote:
> On Wed, Dec 13, 2023 at 07:25:30PM +0800, Hou Tao wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> If an abnormally huge cnt is used for multi-uprobes attachment, the
>> following warning will be reported:
>>
>>   ------------[ cut here ]------------
>>   WARNING: CPU: 7 PID: 406 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
>>   Modules linked in: bpf_testmod(O)
>>   CPU: 7 PID: 406 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
>>   RIP: 0010:kvmalloc_node+0xd9/0xe0
>>   ......
>>   Call Trace:
>>    <TASK>
>>    ? __warn+0x89/0x150
>>    ? kvmalloc_node+0xd9/0xe0
>>    bpf_uprobe_multi_link_attach+0x14a/0x480
>>    __sys_bpf+0x14a9/0x2bc0
>>    do_syscall_64+0x36/0xb0
>>    entry_SYSCALL_64_after_hwframe+0x6e/0x76
>>    ......
>>    </TASK>
>>   ---[ end trace 0000000000000000 ]---
>>
>> So add a test to ensure the warning is fixed.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  .../bpf/prog_tests/uprobe_multi_test.c        | 33 ++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
>> index ece260cf2c0b..0d2a4510e6cf 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
>> @@ -234,6 +234,35 @@ static void test_attach_api_syms(void)
>>  	test_attach_api("/proc/self/exe", NULL, &opts);
>>  }
>>  
>> +static void test_failed_link_api(void)
>> +{
>> +	LIBBPF_OPTS(bpf_link_create_opts, opts);
>> +	const char *path = "/proc/self/exe";
>> +	struct uprobe_multi *skel = NULL;
>> +	unsigned long offset = 0;
>> +	int link_fd = -1;
>> +
>> +	skel = uprobe_multi__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
>> +		goto cleanup;
>> +
>> +	/* abnormal cnt */
>> +	opts.uprobe_multi.path = path;
>> +	opts.uprobe_multi.offsets = &offset;
>> +	opts.uprobe_multi.cnt = INT_MAX;
>> +	opts.kprobe_multi.flags = 0;
>      s/k/u/  ^^^ .. or best just remove the line

My bad. Will remove it. Thanks for the ack.
>
> jirka
>
>> +	link_fd = bpf_link_create(bpf_program__fd(skel->progs.uprobe), 0,
>> +				  BPF_TRACE_UPROBE_MULTI, &opts);
>> +	if (!ASSERT_ERR(link_fd, "link_fd"))
>> +		goto cleanup;
>> +	if (!ASSERT_EQ(link_fd, -EINVAL, "invalid cnt"))
>> +		goto cleanup;
>> +cleanup:
>> +	if (link_fd >= 0)
>> +		close(link_fd);
>> +	uprobe_multi__destroy(skel);
>> +}
>> +
>>  static void __test_link_api(struct child *child)
>>  {
>>  	int prog_fd, link1_fd = -1, link2_fd = -1, link3_fd = -1, link4_fd = -1;
>> @@ -311,7 +340,7 @@ static void __test_link_api(struct child *child)
>>  	free(offsets);
>>  }
>>  
>> -void test_link_api(void)
>> +static void test_link_api(void)
>>  {
>>  	struct child *child;
>>  
>> @@ -408,6 +437,8 @@ void test_uprobe_multi_test(void)
>>  		test_attach_api_syms();
>>  	if (test__start_subtest("link_api"))
>>  		test_link_api();
>> +	if (test__start_subtest("failed_link_api"))
>> +		test_failed_link_api();
>>  	if (test__start_subtest("bench_uprobe"))
>>  		test_bench_attach_uprobe();
>>  	if (test__start_subtest("bench_usdt"))
>> -- 
>> 2.29.2
>>

Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add test for abnormal cnt during multi-kprobe attachment

[Hou Tao]

Hi,

On 12/14/2023 7:33 AM, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@huaweicloud.com> wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> If an abnormally huge cnt is used for multi-kprobes attachment, the
>> following warning will be reported:
>>
>>   ------------[ cut here ]------------
>>   WARNING: CPU: 1 PID: 392 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
>>   Modules linked in: bpf_testmod(O)
>>   CPU: 1 PID: 392 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>   ......
>>   RIP: 0010:kvmalloc_node+0xd9/0xe0
>>    ? __warn+0x89/0x150
>>    ? kvmalloc_node+0xd9/0xe0
>>    bpf_kprobe_multi_link_attach+0x87/0x670
>>    __sys_bpf+0x2a28/0x2bc0
>>    __x64_sys_bpf+0x1a/0x30
>>    do_syscall_64+0x36/0xb0
>>    entry_SYSCALL_64_after_hwframe+0x6e/0x76
>>   RIP: 0033:0x7fbe067f0e0d
>>   ......
>>    </TASK>
>>   ---[ end trace 0000000000000000 ]---
>>
>> So add a test to ensure the warning is fixed.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  .../selftests/bpf/prog_tests/kprobe_multi_test.c   | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> index 4041cfa670eb..802554d4ee24 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> @@ -300,6 +300,20 @@ static void test_attach_api_fails(void)
>>         if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_5_error"))
>>                 goto cleanup;
>>
>> +       /* fail_6 - abnormal cnt */
>> +       opts.addrs = (const unsigned long *) addrs;
>> +       opts.syms = NULL;
>> +       opts.cnt = INT_MAX;
>> +       opts.cookies = NULL;
>> +
>> +       link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
>> +                                                    NULL, &opts);
>> +       if (!ASSERT_ERR_PTR(link, "fail_6"))
>> +               goto cleanup;
>> +
>> +       if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_6_error"))
> this is unreliable, store errno right after the operation before
> ASSERT_xxx() macros

I didn't fully follow the reason why it is unreliable. Do you mean
ASSERT_ERR_PTR() macro may overwrite errno, right ? But _CHECK() already
saves and restores errno before invoking fprintf(), so I think it is OK
to use libbpf_get_error() to get the errno here ?
>
>> +               goto cleanup;
>> +
>>  cleanup:
>>         bpf_link__destroy(link);
>>         kprobe_multi__destroy(skel);
>> --
>> 2.29.2
>>

Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add test for abnormal cnt during multi-kprobe attachment

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 5:44 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 12/14/2023 7:33 AM, Andrii Nakryiko wrote:
> > On Wed, Dec 13, 2023 at 3:24 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >> From: Hou Tao <houtao1@huawei.com>
> >>
> >> If an abnormally huge cnt is used for multi-kprobes attachment, the
> >> following warning will be reported:
> >>
> >>   ------------[ cut here ]------------
> >>   WARNING: CPU: 1 PID: 392 at mm/util.c:632 kvmalloc_node+0xd9/0xe0
> >>   Modules linked in: bpf_testmod(O)
> >>   CPU: 1 PID: 392 Comm: test_progs Tainted: G ...... 6.7.0-rc3+ #32
> >>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> >>   ......
> >>   RIP: 0010:kvmalloc_node+0xd9/0xe0
> >>    ? __warn+0x89/0x150
> >>    ? kvmalloc_node+0xd9/0xe0
> >>    bpf_kprobe_multi_link_attach+0x87/0x670
> >>    __sys_bpf+0x2a28/0x2bc0
> >>    __x64_sys_bpf+0x1a/0x30
> >>    do_syscall_64+0x36/0xb0
> >>    entry_SYSCALL_64_after_hwframe+0x6e/0x76
> >>   RIP: 0033:0x7fbe067f0e0d
> >>   ......
> >>    </TASK>
> >>   ---[ end trace 0000000000000000 ]---
> >>
> >> So add a test to ensure the warning is fixed.
> >>
> >> Signed-off-by: Hou Tao <houtao1@huawei.com>
> >> ---
> >>  .../selftests/bpf/prog_tests/kprobe_multi_test.c   | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >> index 4041cfa670eb..802554d4ee24 100644
> >> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >> @@ -300,6 +300,20 @@ static void test_attach_api_fails(void)
> >>         if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_5_error"))
> >>                 goto cleanup;
> >>
> >> +       /* fail_6 - abnormal cnt */
> >> +       opts.addrs = (const unsigned long *) addrs;
> >> +       opts.syms = NULL;
> >> +       opts.cnt = INT_MAX;
> >> +       opts.cookies = NULL;
> >> +
> >> +       link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
> >> +                                                    NULL, &opts);
> >> +       if (!ASSERT_ERR_PTR(link, "fail_6"))
> >> +               goto cleanup;
> >> +
> >> +       if (!ASSERT_EQ(libbpf_get_error(link), -EINVAL, "fail_6_error"))
> > this is unreliable, store errno right after the operation before
> > ASSERT_xxx() macros
>
> I didn't fully follow the reason why it is unreliable. Do you mean
> ASSERT_ERR_PTR() macro may overwrite errno, right ? But _CHECK() already
> saves and restores errno before invoking fprintf(), so I think it is OK
> to use libbpf_get_error() to get the errno here ?

We shouldn't be using libbpf_get_error() in new code, it's legacy and
not advised to be used. If ASSERT_xxx() macro guarantee to
save/restore errno, then it might be actually fine, but it still feels
better to save errno explicitly right after the operation, just like
you'd do in normal code.

> >
> >> +               goto cleanup;
> >> +
> >>  cleanup:
> >>         bpf_link__destroy(link);
> >>         kprobe_multi__destroy(skel);
> >> --
> >> 2.29.2
> >>
>