bpf: fix unpopulated path_size when uprobe_multi fields unset

PATCH Superseded Tyrone Wu 3 participants 6 messages Oct 9, 2024 – Oct 11, 2024

Subsystems (6)

Kselftest BPF Tracing BPF selftests +2 more subsystems

lore

Share & tools

lore

Tyrone Wu

author

2024-10-09 22:07 UTC #0 lore raw

PATCH 1/2 bpf: fix unpopulated path_size when uprobe_multi fields unset

Previously when retrieving bpf_link_info.uprobe_multi with path and path_size fields unset, the path_size field is not populated (remains 0). This behavior was inconsistent with how other input/output string buffer fields work, as the field should be populated in cases when:
- both buffer and length are set (currently works as expected)
- both buffer and length are unset (not working as expected)

This patch now fills the path_size field when path and path_size are unset.

Fixes e56fdbfb06e2 ("bpf: Add link_info support for uprobe multi link")

Signed-off-by Tyrone Wu <[email protected]>

---
 kernel/trace/bpf_trace.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

kernel/trace/bpf_trace.c +17 ~~-18~~

index a582cd25ca87..ba34e4f3fa8f 100644
@@ -3133,7 +3133,8 @@ static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
 	struct bpf_uprobe_multi_link *umulti_link;
 	u32 ucount = info->uprobe_multi.count;
 	int err = 0, i;
-	long left;
+	char *p, *buf;
+	long left = 0;
 
 	if (!upath ^ !upath_size)
 		return -EINVAL;

@@ -3147,26 +3148,24 @@ static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
 	info->uprobe_multi.pid = umulti_link->task ?
 				 task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current)) : 0;
 
-	if (upath) {
-		char *p, *buf;
-
-		upath_size = min_t(u32, upath_size, PATH_MAX);
+	upath_size = upath_size ? min_t(u32, upath_size, PATH_MAX) : PATH_MAX;
+	buf = kmalloc(upath_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+	p = d_path(&umulti_link->path, buf, upath_size);
+	if (IS_ERR(p)) {
+		kfree(buf);
+		return PTR_ERR(p);
+	}
+	upath_size = buf + upath_size - p;
 
-		buf = kmalloc(upath_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
-		p = d_path(&umulti_link->path, buf, upath_size);
-		if (IS_ERR(p)) {
-			kfree(buf);
-			return PTR_ERR(p);
-		}
-		upath_size = buf + upath_size - p;
+	if (upath) {
 		left = copy_to_user(upath, p, upath_size);
-		kfree(buf);
-		if (left)
-			return -EFAULT;
-		info->uprobe_multi.path_size = upath_size;
 	}
+	kfree(buf);
+	if (left)
+		return -EFAULT;
+	info->uprobe_multi.path_size = upath_size;
 
 	if (!uoffsets && !ucookies && !uref_ctr_offsets)
 		return 0;

Signature · 1 line

2.43.0

Tyrone Wu

author

2024-10-09 22:07 UTC #1 lore raw

PATCH 2/2 selftests/bpf: assert link info uprobe_multi count & path_size if unset

Add assertions in bpf_link_info.uprobe_multi test to verify that count and path_size fields are correctly populated when the fields are unset.

This tests a previous bug where the path_size field was not populated when path and path_size were unset.

Signed-off-by Tyrone Wu <[email protected]>

---
 tools/testing/selftests/bpf/prog_tests/fill_link_info.c | 7 +++++++
 1 file changed, 7 insertions(+)

tools/testing/selftests/bpf/prog_tests/fill_link_info.c +7 -0

index f3932941bbaa..a38cf2a999fe 100644
@@ -417,6 +417,13 @@ verify_umulti_link_info(int fd, bool retprobe, __u64 *offsets,
 	if (!ASSERT_NEQ(err, -1, "readlink"))
 		return -1;
 
+	memset(&info, 0, sizeof(info));
+	err = bpf_link_get_info_by_fd(fd, &info, &len);
+
+	ASSERT_EQ(info.uprobe_multi.count, 3, "info.uprobe_multi.count");
+	ASSERT_EQ(info.uprobe_multi.path_size, strlen(path) + 1,
+		  "info.uprobe_multi.path_size");
+
 	for (bit = 0; bit < 8; bit++) {
 		memset(&info, 0, sizeof(info));
 		info.uprobe_multi.path = ptr_to_u64(path_buf);

Signature · 1 line

2.43.0

Andrii Nakryiko

2024-10-10 22:01 UTC #2 lore raw

Re: [PATCH bpf v1 2/2] selftests/bpf: assert link info uprobe_multi count & path_size if unset

Quoted 18 lines · Tyrone Wu

Add assertions in bpf_link_info.uprobe_multi test to verify that count and path_size fields are correctly populated when the fields are unset.
This tests a previous bug where the path_size field was not populated when path and path_size were unset.
Signed-off-by Tyrone Wu <[email protected]>
---
 tools/testing/selftests/bpf/prog_tests/fill_link_info.c | 7 +++++++
 1 file changed, 7 insertions(+)
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +0 -0
index f3932941bbaa..a38cf2a999fe 100644

tools/testing/selftests/bpf/prog_tests/fill_link_info.c verify_umulti_link_info(int fd, bool retprobe, __u64 *offsets, · line 421

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/fill_link_info.c +2 -0

@@ -417,6 +417,13 @@ verify_umulti_link_info(int fd, bool retprobe, __u64 *offsets,
        if (!ASSERT_NEQ(err, -1, "readlink"))
                return -1;
 
+       memset(&info, 0, sizeof(info));
+       err = bpf_link_get_info_by_fd(fd, &info, &len);

if (!ASSERT_OK(err, "link_get_info"))

    return -1;

Other than this, LGTM.

pw-bot: cr

Quoted 11 lines

tools/testing/selftests/bpf/prog_tests/fill_link_info.c +5 -1

+
+       ASSERT_EQ(info.uprobe_multi.count, 3, "info.uprobe_multi.count");
+       ASSERT_EQ(info.uprobe_multi.path_size, strlen(path) + 1,
+                 "info.uprobe_multi.path_size");
+
        for (bit = 0; bit < 8; bit++) {
                memset(&info, 0, sizeof(info));
                info.uprobe_multi.path = ptr_to_u64(path_buf);
--

2.43.0

Tyrone Wu

author

2024-10-11 00:08 UTC #3 lore raw

PATCH v2 1/2 bpf: fix unpopulated path_size when uprobe_multi fields unset

This patch now fills the path_size field when path and path_size are unset.

Fixes e56fdbfb06e2 ("bpf: Add link_info support for uprobe multi link")

Signed-off-by Tyrone Wu <[email protected]>

---
V1 -> V2: no change

 kernel/trace/bpf_trace.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

kernel/trace/bpf_trace.c +17 ~~-19~~

index a582cd25ca87..3bd402fa62a4 100644
@@ -3133,7 +3133,8 @@ static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
 	struct bpf_uprobe_multi_link *umulti_link;
 	u32 ucount = info->uprobe_multi.count;
 	int err = 0, i;
-	long left;
+	char *p, *buf;
+	long left = 0;
 
 	if (!upath ^ !upath_size)
 		return -EINVAL;

@@ -3147,26 +3148,23 @@ static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
 	info->uprobe_multi.pid = umulti_link->task ?
 				 task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current)) : 0;
 
-	if (upath) {
-		char *p, *buf;
-
-		upath_size = min_t(u32, upath_size, PATH_MAX);
-
-		buf = kmalloc(upath_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
-		p = d_path(&umulti_link->path, buf, upath_size);
-		if (IS_ERR(p)) {
-			kfree(buf);
-			return PTR_ERR(p);
-		}
-		upath_size = buf + upath_size - p;
-		left = copy_to_user(upath, p, upath_size);
+	upath_size = upath_size ? min_t(u32, upath_size, PATH_MAX) : PATH_MAX;
+	buf = kmalloc(upath_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+	p = d_path(&umulti_link->path, buf, upath_size);
+	if (IS_ERR(p)) {
 		kfree(buf);
-		if (left)
-			return -EFAULT;
-		info->uprobe_multi.path_size = upath_size;
+		return PTR_ERR(p);
 	}
+	upath_size = buf + upath_size - p;
+
+	if (upath)
+		left = copy_to_user(upath, p, upath_size);
+	kfree(buf);
+	if (left)
+		return -EFAULT;
+	info->uprobe_multi.path_size = upath_size;
 
 	if (!uoffsets && !ucookies && !uref_ctr_offsets)
 		return 0;

Signature · 1 line

2.43.0

Tyrone Wu

author

2024-10-11 00:08 UTC #4 lore raw

PATCH v2 2/2 selftests/bpf: assert link info uprobe_multi count & path_size if unset

Add assertions in bpf_link_info.uprobe_multi test to verify that count and path_size fields are correctly populated when the fields are unset.

This tests a previous bug where the path_size field was not populated when path and path_size were unset.

Signed-off-by Tyrone Wu <[email protected]>

---
V1 -> V2:
- Verify bpf_link_get_info_by_fd was successful before continuing with test

 tools/testing/selftests/bpf/prog_tests/fill_link_info.c | 9 +++++++++
 1 file changed, 9 insertions(+)

tools/testing/selftests/bpf/prog_tests/fill_link_info.c +9 -0

index 745c5ada4c4b..d50cbd8040d4 100644
@@ -420,6 +420,15 @@ verify_umulti_link_info(int fd, bool retprobe, __u64 *offsets,
 	if (!ASSERT_NEQ(err, -1, "readlink"))
 		return -1;
 
+	memset(&info, 0, sizeof(info));
+	err = bpf_link_get_info_by_fd(fd, &info, &len);
+	if (!ASSERT_OK(err, "bpf_link_get_info_by_fd"))
+		return -1;
+
+	ASSERT_EQ(info.uprobe_multi.count, 3, "info.uprobe_multi.count");
+	ASSERT_EQ(info.uprobe_multi.path_size, strlen(path) + 1,
+		  "info.uprobe_multi.path_size");
+
 	for (bit = 0; bit < 8; bit++) {
 		memset(&info, 0, sizeof(info));
 		info.uprobe_multi.path = ptr_to_u64(path_buf);

Signature · 1 line

2.43.0

[email protected]

CI Applied

2024-10-11 02:20 UTC #5 lore raw

In reply to Tyrone Wu

This series was applied to bpf/bpf.git (master) by Andrii Nakryiko <[email protected]>:

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <[email protected]>:

Quoted 9 lines · On Fri, 11 Oct 2024 00:08:02 +0000 you wrote:

Previously when retrieving bpf_link_info.uprobe_multi with path and path_size fields unset, the path_size field is not populated (remains 0). This behavior was inconsistent with how other input/output string buffer fields work, as the field should be populated in cases when:
- both buffer and length are set (currently works as expected)
- both buffer and length are unset (not working as expected)
[...]

Here is the summary with links:
- [bpf,v2,1/2] bpf: fix unpopulated path_size when uprobe_multi fields unset

    https://git.kernel.org/bpf/bpf/c/ad6b5b6ea9b7
  - [bpf,v2,2/2] selftests/bpf: assert link info uprobe_multi count & path_size if unset
    https://git.kernel.org/bpf/bpf/c/b836cbdf3b81

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html