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

[PATCH bpf-next 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 fixes the warning in multiple uprobes attachment and patch #4
is the corresponding test case. Patch #2 fixes the warning for multiple
kprobes attachment and patch #3 is the corresponding test case.

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

Hou Tao (4):
  bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes
  bpf: Use __GFP_NOWARN for kvmalloc_array() when attaching multiple
    kprobes
  selftests/bpf: Add test for abnormal cnt during multi-kprobe
    attachment
  selftests/bpf: Add test for abnormal cnt during multi-uprobe
    attachment

 kernel/trace/bpf_trace.c                      | 10 ++---
 .../bpf/prog_tests/kprobe_multi_test.c        | 14 ++++++
 .../bpf/prog_tests/uprobe_multi_test.c        | 43 ++++++++++++++++++-
 3 files changed, 61 insertions(+), 6 deletions(-)

-- 
2.29.2

[PATCH bpf-next 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching 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 using __GFP_NOWARN when invoking kvzalloc() 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 774cf476a892..07b9b5896d6c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	err = -ENOMEM;
 
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
-	uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
+	uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);
 
 	if (!uprobes || !link)
 		goto error_free;
-- 
2.29.2

[PATCH bpf-next 2/4] bpf: Use __GFP_NOWARN for kvmalloc_array() when attaching 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 using __GFP_NOWARN when invoking kvmalloc_array() 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 07b9b5896d6c..64f200890c19 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2605,11 +2605,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
 	int err = -ENOMEM;
 	unsigned int i;
 
-	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
+	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL | __GFP_NOWARN);
 	if (!syms)
 		goto error;
 
-	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
+	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL | __GFP_NOWARN);
 	if (!buf)
 		goto error;
 
@@ -2972,13 +2972,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		return -EINVAL;
 
 	size = cnt * sizeof(*addrs);
-	addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL);
+	addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL | __GFP_NOWARN);
 	if (!addrs)
 		return -ENOMEM;
 
 	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
 	if (ucookies) {
-		cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL);
+		cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL | __GFP_NOWARN);
 		if (!cookies) {
 			err = -ENOMEM;
 			goto error;
-- 
2.29.2

[PATCH bpf-next 3/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..a340b6047657 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), -ENOMEM, "fail_6_error"))
+		goto cleanup;
+
 cleanup:
 	bpf_link__destroy(link);
 	kprobe_multi__destroy(skel);
-- 
2.29.2

[PATCH bpf-next 4/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        | 43 ++++++++++++++++++-
 1 file changed, 42 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..379ee9cc6221 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,45 @@ 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 *offsets = NULL;
+	const char *syms[3] = {
+		"uprobe_multi_func_1",
+		"uprobe_multi_func_2",
+		"uprobe_multi_func_3",
+	};
+	int link_fd = -1, err;
+
+	err = elf_resolve_syms_offsets(path, 3, syms, (unsigned long **) &offsets, STT_FUNC);
+	if (!ASSERT_OK(err, "elf_resolve_syms_offsets"))
+		return;
+
+	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 = offsets;
+	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, -ENOMEM, "no mem fail"))
+		goto cleanup;
+cleanup:
+	if (link_fd >= 0)
+		close(link_fd);
+	uprobe_multi__destroy(skel);
+	free(offsets);
+}
+
 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 +350,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 +447,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 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:28:40PM +0800, Hou Tao wrote:
> 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 using __GFP_NOWARN when invoking kvzalloc() 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>

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

thanks,
jirka

> ---
>  kernel/trace/bpf_trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 774cf476a892..07b9b5896d6c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	err = -ENOMEM;
>  
>  	link = kzalloc(sizeof(*link), GFP_KERNEL);
> -	uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
> +	uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);
>  
>  	if (!uprobes || !link)
>  		goto error_free;
> -- 
> 2.29.2
> 

Re: [PATCH bpf-next 2/4] bpf: Use __GFP_NOWARN for kvmalloc_array() when attaching multiple kprobes

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:28:41PM +0800, Hou Tao 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 using __GFP_NOWARN when invoking kvmalloc_array() in
> bpf_kprobe_multi_link_attach().
> 
> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> Signed-off-by: Hou Tao <houtao1@huawei.com>

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

thanks,
jirka

> ---
>  kernel/trace/bpf_trace.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 07b9b5896d6c..64f200890c19 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2605,11 +2605,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>  	int err = -ENOMEM;
>  	unsigned int i;
>  
> -	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
> +	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL | __GFP_NOWARN);
>  	if (!syms)
>  		goto error;
>  
> -	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
> +	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL | __GFP_NOWARN);
>  	if (!buf)
>  		goto error;
>  
> @@ -2972,13 +2972,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  		return -EINVAL;
>  
>  	size = cnt * sizeof(*addrs);
> -	addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL);
> +	addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL | __GFP_NOWARN);
>  	if (!addrs)
>  		return -ENOMEM;
>  
>  	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>  	if (ucookies) {
> -		cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL);
> +		cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL | __GFP_NOWARN);
>  		if (!cookies) {
>  			err = -ENOMEM;
>  			goto error;
> -- 
> 2.29.2
> 

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

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:28:43PM +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        | 43 ++++++++++++++++++-
>  1 file changed, 42 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..379ee9cc6221 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,45 @@ 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 *offsets = NULL;
> +	const char *syms[3] = {
> +		"uprobe_multi_func_1",
> +		"uprobe_multi_func_2",
> +		"uprobe_multi_func_3",
> +	};
> +	int link_fd = -1, err;
> +
> +	err = elf_resolve_syms_offsets(path, 3, syms, (unsigned long **) &offsets, STT_FUNC);
> +	if (!ASSERT_OK(err, "elf_resolve_syms_offsets"))
> +		return;

we should not need any symbols/offset for this tests right?

the allocation takes place before the offsets are checked,
so I think using just some pointer != NULL should be enough?

thanks,
jirka

> +
> +	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 = offsets;
> +	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, -ENOMEM, "no mem fail"))
> +		goto cleanup;
> +cleanup:
> +	if (link_fd >= 0)
> +		close(link_fd);
> +	uprobe_multi__destroy(skel);
> +	free(offsets);
> +}
> +
>  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 +350,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 +447,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 3/4] selftests/bpf: Add test for abnormal cnt during multi-kprobe attachment

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:28:42PM +0800, Hou Tao 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>

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

thanks,
jirka

> ---
>  .../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..a340b6047657 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), -ENOMEM, "fail_6_error"))
> +		goto cleanup;
> +
>  cleanup:
>  	bpf_link__destroy(link);
>  	kprobe_multi__destroy(skel);
> -- 
> 2.29.2
> 

Re: [PATCH bpf-next 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Alexei Starovoitov]

On Mon, Dec 11, 2023 at 3:27 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> 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 using __GFP_NOWARN when invoking kvzalloc() 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 774cf476a892..07b9b5896d6c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>         err = -ENOMEM;
>
>         link = kzalloc(sizeof(*link), GFP_KERNEL);
> -       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
> +       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);

__GFP_NOWARN will hide actual malloc failures.
Let's limit cnt instead. Both for k and u multi probes.

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

[Hou Tao]

Hi,

On 12/11/2023 8:56 PM, Jiri Olsa wrote:
> On Mon, Dec 11, 2023 at 07:28:43PM +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        | 43 ++++++++++++++++++-
>>  1 file changed, 42 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..379ee9cc6221 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,45 @@ 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 *offsets = NULL;
>> +	const char *syms[3] = {
>> +		"uprobe_multi_func_1",
>> +		"uprobe_multi_func_2",
>> +		"uprobe_multi_func_3",
>> +	};
>> +	int link_fd = -1, err;
>> +
>> +	err = elf_resolve_syms_offsets(path, 3, syms, (unsigned long **) &offsets, STT_FUNC);
>> +	if (!ASSERT_OK(err, "elf_resolve_syms_offsets"))
>> +		return;
> we should not need any symbols/offset for this tests right?
>
> the allocation takes place before the offsets are checked,
> so I think using just some pointer != NULL should be enough?

Yes. Will update.
>
> thanks,
> jirka
>
>> +
>> +	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 = offsets;
>> +	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, -ENOMEM, "no mem fail"))
>> +		goto cleanup;
>> +cleanup:
>> +	if (link_fd >= 0)
>> +		close(link_fd);
>> +	uprobe_multi__destroy(skel);
>> +	free(offsets);
>> +}
>> +
>>  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 +350,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 +447,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 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Hou Tao]

Hi,

On 12/12/2023 12:50 AM, Alexei Starovoitov wrote:
> On Mon, Dec 11, 2023 at 3:27 AM Hou Tao <houtao@huaweicloud.com> wrote:
>> 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 using __GFP_NOWARN when invoking kvzalloc() 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 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 774cf476a892..07b9b5896d6c 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>         err = -ENOMEM;
>>
>>         link = kzalloc(sizeof(*link), GFP_KERNEL);
>> -       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
>> +       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);
> __GFP_NOWARN will hide actual malloc failures.
> Let's limit cnt instead. Both for k and u multi probes.

Do you mean there will be no warning messages when the malloc request
can not be fulfilled, right ?  Because kvcalloc() will still return
-ENOMEM when __GFP_NOWARN is used, so the userspace knows the malloc
failed. And I also found out that __GFP_NOWARN only effect the
invocation of vmalloc(), because kvmalloc_node() enable __GFP_NOWARN for
kmalloc() by default when the passed size is greater than PAGE_SIZE.

I also though about fixing the problem by limitation, but I could not
get good reference values for these limitations. For multiple kprobe,
maybe the number of kallsyms can be used as an anchor (e.g, the number
is 207617 on my local dev machine), how about using 
__roundup_pow_of_two(207617 * 4) = 1 << 20 for multiple kprobes ? For
multiple uprobes, maybe (1<<20) is also suitable.

Re: [PATCH bpf-next 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Jiri Olsa]

On Tue, Dec 12, 2023 at 11:44:34AM +0800, Hou Tao wrote:
> Hi,
> 
> On 12/12/2023 12:50 AM, Alexei Starovoitov wrote:
> > On Mon, Dec 11, 2023 at 3:27 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >> 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 using __GFP_NOWARN when invoking kvzalloc() 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 | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index 774cf476a892..07b9b5896d6c 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >>         err = -ENOMEM;
> >>
> >>         link = kzalloc(sizeof(*link), GFP_KERNEL);
> >> -       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
> >> +       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);
> > __GFP_NOWARN will hide actual malloc failures.
> > Let's limit cnt instead. Both for k and u multi probes.
> 
> Do you mean there will be no warning messages when the malloc request
> can not be fulfilled, right ?  Because kvcalloc() will still return
> -ENOMEM when __GFP_NOWARN is used, so the userspace knows the malloc
> failed. And I also found out that __GFP_NOWARN only effect the
> invocation of vmalloc(), because kvmalloc_node() enable __GFP_NOWARN for
> kmalloc() by default when the passed size is greater than PAGE_SIZE.
> 
> I also though about fixing the problem by limitation, but I could not
> get good reference values for these limitations. For multiple kprobe,
> maybe the number of kallsyms can be used as an anchor (e.g, the number
> is 207617 on my local dev machine), how about using 
> __roundup_pow_of_two(207617 * 4) = 1 << 20 for multiple kprobes ? For
> multiple uprobes, maybe (1<<20) is also suitable.

I think available_filter_functions is more relevant, because kallsyms
has everything

on fedora kernel:
  # cat available_filter_functions | wc -l
  80177

anyway to be on the safe side with some other configs and possible
huge kernel modules the '1 << 20' looks good to me, also for uprobe
multi

jirka

Re: [PATCH bpf-next 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Hou Tao]

Hi,

On 12/12/2023 5:54 PM, Jiri Olsa wrote:
> On Tue, Dec 12, 2023 at 11:44:34AM +0800, Hou Tao wrote:
>> Hi,
>>
>> On 12/12/2023 12:50 AM, Alexei Starovoitov wrote:
>>> On Mon, Dec 11, 2023 at 3:27 AM Hou Tao <houtao@huaweicloud.com> wrote:
>>>> 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 using __GFP_NOWARN when invoking kvzalloc() 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 | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>> index 774cf476a892..07b9b5896d6c 100644
>>>> --- a/kernel/trace/bpf_trace.c
>>>> +++ b/kernel/trace/bpf_trace.c
>>>> @@ -3378,7 +3378,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>>>         err = -ENOMEM;
>>>>
>>>>         link = kzalloc(sizeof(*link), GFP_KERNEL);
>>>> -       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
>>>> +       uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL | __GFP_NOWARN);
>>> __GFP_NOWARN will hide actual malloc failures.
>>> Let's limit cnt instead. Both for k and u multi probes.
>> Do you mean there will be no warning messages when the malloc request
>> can not be fulfilled, right ?  Because kvcalloc() will still return
>> -ENOMEM when __GFP_NOWARN is used, so the userspace knows the malloc
>> failed. And I also found out that __GFP_NOWARN only effect the
>> invocation of vmalloc(), because kvmalloc_node() enable __GFP_NOWARN for
>> kmalloc() by default when the passed size is greater than PAGE_SIZE.
>>
>> I also though about fixing the problem by limitation, but I could not
>> get good reference values for these limitations. For multiple kprobe,
>> maybe the number of kallsyms can be used as an anchor (e.g, the number
>> is 207617 on my local dev machine), how about using 
>> __roundup_pow_of_two(207617 * 4) = 1 << 20 for multiple kprobes ? For
>> multiple uprobes, maybe (1<<20) is also suitable.
> I think available_filter_functions is more relevant, because kallsyms
> has everything
>
> on fedora kernel:
>   # cat available_filter_functions | wc -l
>   80177

Agreed. Only functions in available_filter_functions could use kprobe.
>
> anyway to be on the safe side with some other configs and possible
> huge kernel modules the '1 << 20' looks good to me, also for uprobe
> multi

Thanks. Will post v2 if Alexei is also fine with such limitations.
>
> jirka

Re: [PATCH bpf-next 1/4] bpf: Use __GFP_NOWARN for kvcalloc when attaching multiple uprobes

[Alexei Starovoitov]

On Tue, Dec 12, 2023 at 6:06 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> > anyway to be on the safe side with some other configs and possible
> > huge kernel modules the '1 << 20' looks good to me, also for uprobe
> > multi
>
> Thanks. Will post v2 if Alexei is also fine with such limitations.

Yeah. The limit looks fine.

> >> can not be fulfilled, right ?  Because kvcalloc() will still return
> >> -ENOMEM when __GFP_NOWARN is used, so the userspace knows the malloc
> >> failed. And I also found out that __GFP_NOWARN only effect the
> >> invocation of vmalloc(), because kvmalloc_node() enable __GFP_NOWARN for
> >> kmalloc() by default when the passed size is greater than PAGE_SIZE.

Right, because kmalloc of more than a page will likely fail.
But vmalloc() may fail for all kinds of other reasons.
Suppressing them all prevents those messages appearing in the future.
The warn is there by default, so that users think about memory
allocations they request. So it served its exact purpose.
Just returning ENOMEM to user space would have been unnoticed
and cnt > INT_MAX would continue to be acceptable which potentially
opens up DoS, and other abuse.