[v1] bpf: fix unpopulated name_len field in perf_event link info

[PATCH bpf] bpf: fix unpopulated name_len field in perf_event link info

Posted by tyrone-wu 1 month, 4 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name. The
 relevant selftests have also been updated to assert that `name_len`
contains the correct size rather than 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
---
 kernel/bpf/syscall.c                          | 25 +++++++++++++------
 .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++---
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..90b6add4d0c9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,7 +3565,7 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
@@ -3574,18 +3574,20 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	len = strlen(buf);
+	*ulen = len + 1;
 	if (!uname)
 		return 0;
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, *ulen, len);
 		if (err)
 			return err;
 	} else {
@@ -3609,7 +3611,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3619,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3641,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3650,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3676,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..59077f260404 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

Re: [PATCH bpf] bpf: fix unpopulated name_len field in perf_event link info

Posted by Jiri Olsa 1 month, 3 weeks ago

On Mon, Sep 30, 2024 at 11:59:20PM +0000, tyrone-wu wrote:
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
> 
> This patch fills `name_len` with the actual size of the string name. The
>  relevant selftests have also been updated to assert that `name_len`
> contains the correct size rather than 0.
> 
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> ---
>  kernel/bpf/syscall.c                          | 25 +++++++++++++------
>  .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++---
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..90b6add4d0c9 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,7 +3565,7 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>  
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -				     char __user *uname, u32 ulen,
> +				     char __user *uname, u32 *ulen,
>  				     u64 *probe_offset, u64 *probe_addr,
>  				     u32 *fd_type, unsigned long *missed)
>  {
> @@ -3574,18 +3574,20 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
>  	size_t len;
>  	int err;
>  
> -	if (!ulen ^ !uname)
> +	if (!(*ulen) ^ !uname)
>  		return -EINVAL;
>  
>  	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>  				      probe_offset, probe_addr, missed);
>  	if (err)
>  		return err;
> +
> +	len = strlen(buf);
> +	*ulen = len + 1;

I think before you overwrite *ulen with actual name lenth you should
store its value and use it in bpf_copy_to_user, otherwise we could
overwrite user space memory that we are not supposed to

jirka

>  	if (!uname)
>  		return 0;
>  	if (buf) {
> -		len = strlen(buf);
> -		err = bpf_copy_to_user(uname, buf, ulen, len);
> +		err = bpf_copy_to_user(uname, buf, *ulen, len);
>  		if (err)
>  			return err;
>  	} else {
> @@ -3609,7 +3611,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>  
>  	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
>  	ulen = info->perf_event.kprobe.name_len;
> -	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>  					&type, &missed);
>  	if (err)
>  		return err;

SNIP

[PATCH bpf v2] bpf: fix unpopulated name_len field in perf_event link info

Posted by tyrone-wu 1 month, 3 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name. The
 relevant selftests have also been updated to assert that `name_len`
contains the correct size rather than 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
---
V1 -> V2:
Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c                          | 29 +++++++++++++------
 .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++--
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..26cc18693924 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
 	u32 prog_id;
-	size_t len;
+	size_t len, name_len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	name_len = *ulen;
+	len = strlen(buf);
+	*ulen = len + 1;
 	if (!uname)
 		return 0;
+
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, name_len, len);
 		if (err)
 			return err;
 	} else {
@@ -3609,7 +3613,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3621,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3643,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3652,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3678,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..59077f260404 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

Re: [PATCH bpf v2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Jiri Olsa 1 month, 3 weeks ago

On Wed, Oct 02, 2024 at 09:38:39PM +0000, tyrone-wu wrote:
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
> 
> This patch fills `name_len` with the actual size of the string name. The
>  relevant selftests have also been updated to assert that `name_len`
> contains the correct size rather than 0.
> 
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> ---
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
> 
>  kernel/bpf/syscall.c                          | 29 +++++++++++++------
>  .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++--
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..26cc18693924 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>  
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -				     char __user *uname, u32 ulen,
> +				     char __user *uname, u32 *ulen,
>  				     u64 *probe_offset, u64 *probe_addr,
>  				     u32 *fd_type, unsigned long *missed)
>  {
>  	const char *buf;
>  	u32 prog_id;
> -	size_t len;
> +	size_t len, name_len;

>  	int err;
>  
> -	if (!ulen ^ !uname)
> +	if (!(*ulen) ^ !uname)
>  		return -EINVAL;
>  
>  	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>  				      probe_offset, probe_addr, missed);
>  	if (err)
>  		return err;
> +
> +	name_len = *ulen;
> +	len = strlen(buf);
> +	*ulen = len + 1;
>  	if (!uname)
>  		return 0;
> +
>  	if (buf) {
> -		len = strlen(buf);
> -		err = bpf_copy_to_user(uname, buf, ulen, len);
> +		err = bpf_copy_to_user(uname, buf, name_len, len);
>  		if (err)
>  			return err;
>  	} else {

small nit.. up to you but I'd suggest bit different naming like below,
otherwise looks good

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

thanks,
jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..b637e9dced5a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,26 +3565,28 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulenp,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, ulen;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulenp) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+	ulen = *ulenp;
+	len = strlen(buf);
+	*ulenp = len + 1;
 	if (!uname)
 		return 0;
 	if (buf) {
-		len = strlen(buf);
 		err = bpf_copy_to_user(uname, buf, ulen, len);
 		if (err)
 			return err;

[PATCH bpf v3] bpf: fix unpopulated name_len field in perf_event link info

Posted by tyrone-wu 1 month, 3 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name. The
 relevant selftests have also been updated to assert that `name_len`
contains the correct size rather than 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
---
V2 -> V3:
Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
- Use clearer variable name for user set/inputted name len (name_len -> input_len)
- Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

V1 -> V2:
Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c                          | 29 +++++++++++++------
 .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++--
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..56c556fcf325 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, input_len;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	input_len = *ulen;
+	len = strlen(buf);
+	*ulen = len + 1;
 	if (!uname)
 		return 0;
+
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, input_len, len);
 		if (err)
 			return err;
 	} else {
@@ -3609,7 +3613,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3621,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3643,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3652,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3678,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..59077f260404 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

Re: [PATCH bpf v3] bpf: fix unpopulated name_len field in perf_event link info

Posted by Jiri Olsa 1 month, 3 weeks ago

On Thu, Oct 03, 2024 at 08:23:00PM +0000, tyrone-wu wrote:
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
> 
> This patch fills `name_len` with the actual size of the string name. The
>  relevant selftests have also been updated to assert that `name_len`
> contains the correct size rather than 0.
> 
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> ---
> V2 -> V3:
> Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
> - Use clearer variable name for user set/inputted name len (name_len -> input_len)
> - Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

looks good, I checked with Daniel and it's better to separate
kernel and selftest changes, so it's easier to get this applied
in stable branches

could you split the change in 2 changes and repost? please keep my ack

thanks,
jirka

> 
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
> 
>  kernel/bpf/syscall.c                          | 29 +++++++++++++------
>  .../selftests/bpf/prog_tests/fill_link_info.c |  6 ++--
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..56c556fcf325 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>  
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -				     char __user *uname, u32 ulen,
> +				     char __user *uname, u32 *ulen,
>  				     u64 *probe_offset, u64 *probe_addr,
>  				     u32 *fd_type, unsigned long *missed)
>  {
>  	const char *buf;
> -	u32 prog_id;
> +	u32 prog_id, input_len;
>  	size_t len;
>  	int err;
>  
> -	if (!ulen ^ !uname)
> +	if (!(*ulen) ^ !uname)
>  		return -EINVAL;
>  
>  	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>  				      probe_offset, probe_addr, missed);
>  	if (err)
>  		return err;
> +
> +	input_len = *ulen;
> +	len = strlen(buf);
> +	*ulen = len + 1;
>  	if (!uname)
>  		return 0;
> +
>  	if (buf) {
> -		len = strlen(buf);
> -		err = bpf_copy_to_user(uname, buf, ulen, len);
> +		err = bpf_copy_to_user(uname, buf, input_len, len);
>  		if (err)
>  			return err;
>  	} else {
> @@ -3609,7 +3613,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>  
>  	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
>  	ulen = info->perf_event.kprobe.name_len;
> -	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>  					&type, &missed);
>  	if (err)
>  		return err;
> @@ -3617,7 +3621,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>  		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
>  	else
>  		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> -
> +	info->perf_event.kprobe.name_len = ulen;
>  	info->perf_event.kprobe.offset = offset;
>  	info->perf_event.kprobe.missed = missed;
>  	if (!kallsyms_show_value(current_cred()))
> @@ -3639,7 +3643,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>  
>  	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
>  	ulen = info->perf_event.uprobe.name_len;
> -	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>  					&type, NULL);
>  	if (err)
>  		return err;
> @@ -3648,6 +3652,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>  		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
>  	else
>  		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
> +	info->perf_event.uprobe.name_len = ulen;
>  	info->perf_event.uprobe.offset = offset;
>  	info->perf_event.uprobe.cookie = event->bpf_cookie;
>  	return 0;
> @@ -3673,12 +3678,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
>  {
>  	char __user *uname;
>  	u32 ulen;
> +	int err;
>  
>  	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
>  	ulen = info->perf_event.tracepoint.name_len;
> +	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
> +	if (err)
> +		return err;
> +
>  	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
> +	info->perf_event.tracepoint.name_len = ulen;
>  	info->perf_event.tracepoint.cookie = event->bpf_cookie;
> -	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
> +	return 0;
>  }
>  
>  static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
> diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> index f3932941bbaa..59077f260404 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> @@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>  
>  		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
>  
> +		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
>  		if (!info.perf_event.kprobe.func_name) {
> -			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
>  			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
>  			info.perf_event.kprobe.name_len = sizeof(buf);
>  			goto again;
> @@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>  		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
>  		break;
>  	case BPF_PERF_EVENT_TRACEPOINT:
> +		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
>  		if (!info.perf_event.tracepoint.tp_name) {
> -			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
>  			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
>  			info.perf_event.tracepoint.name_len = sizeof(buf);
>  			goto again;
> @@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>  	case BPF_PERF_EVENT_URETPROBE:
>  		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
>  
> +		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
>  		if (!info.perf_event.uprobe.file_name) {
> -			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
>  			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
>  			info.perf_event.uprobe.name_len = sizeof(buf);
>  			goto again;
> -- 
> 2.43.0
>

[PATCH bpf v4 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by tyrone-wu 1 month, 3 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

V2 -> V3:
Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
- Use clearer variable name for user set/inputted name len (name_len -> input_len)
- Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

V1 -> V2:
Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..56c556fcf325 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, input_len;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	input_len = *ulen;
+	len = strlen(buf);
+	*ulen = len + 1;
 	if (!uname)
 		return 0;
+
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, input_len, len);
 		if (err)
 			return err;
 	} else {
@@ -3609,7 +3613,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3621,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3643,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3652,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3678,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
-- 
2.43.0

Re: [PATCH bpf v4 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Yafang Shao 1 month, 3 weeks ago

On Fri, Oct 4, 2024 at 11:40 PM tyrone-wu <wudevelops@gmail.com> wrote:
>
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
>
> This patch fills `name_len` with the actual size of the string name.
>
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
> V3 -> V4:
> Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
> - Split patch into separate kernel and selftest change
>
> V2 -> V3:
> Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
> - Use clearer variable name for user set/inputted name len (name_len -> input_len)
> - Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32
>
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
>
>  kernel/bpf/syscall.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..56c556fcf325 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -                                    char __user *uname, u32 ulen,
> +                                    char __user *uname, u32 *ulen,
>                                      u64 *probe_offset, u64 *probe_addr,
>                                      u32 *fd_type, unsigned long *missed)
>  {
>         const char *buf;
> -       u32 prog_id;
> +       u32 prog_id, input_len;
>         size_t len;
>         int err;
>
> -       if (!ulen ^ !uname)
> +       if (!(*ulen) ^ !uname)
>                 return -EINVAL;
>
>         err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>                                       probe_offset, probe_addr, missed);
>         if (err)
>                 return err;
> +
> +       input_len = *ulen;
> +       len = strlen(buf);

The buf might be NULL, so we should check it.

> +       *ulen = len + 1;
>         if (!uname)
>                 return 0;
> +
>         if (buf) {
> -               len = strlen(buf);
> -               err = bpf_copy_to_user(uname, buf, ulen, len);
> +               err = bpf_copy_to_user(uname, buf, input_len, len);
>                 if (err)
>                         return err;
>         } else {
> @@ -3609,7 +3613,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
>         ulen = info->perf_event.kprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, &missed);
>         if (err)
>                 return err;
> @@ -3617,7 +3621,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> -
> +       info->perf_event.kprobe.name_len = ulen;
>         info->perf_event.kprobe.offset = offset;
>         info->perf_event.kprobe.missed = missed;
>         if (!kallsyms_show_value(current_cred()))
> @@ -3639,7 +3643,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
>         ulen = info->perf_event.uprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, NULL);
>         if (err)
>                 return err;
> @@ -3648,6 +3652,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_UPROBE;
> +       info->perf_event.uprobe.name_len = ulen;
>         info->perf_event.uprobe.offset = offset;
>         info->perf_event.uprobe.cookie = event->bpf_cookie;
>         return 0;
> @@ -3673,12 +3678,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
>  {
>         char __user *uname;
>         u32 ulen;
> +       int err;
>
>         uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
>         ulen = info->perf_event.tracepoint.name_len;
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
> +       if (err)
> +               return err;
> +
>         info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
> +       info->perf_event.tracepoint.name_len = ulen;
>         info->perf_event.tracepoint.cookie = event->bpf_cookie;
> -       return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
> +       return 0;
>  }
>
>  static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
> --
> 2.43.0
>


--
Regards


Yafang

[PATCH bpf v5 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by tyrone-wu 1 month, 3 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
V4 -> V5:
Link: https://lore.kernel.org/bpf/CALOAHbC5xm7Cbfhau3z5X2PqUhiHECNWAPtJCWiOVqTKmdZp-Q@mail.gmail.com/
- Check that buf is not NULL before retrieving/using its length

V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

V2 -> V3:
Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
- Use clearer variable name for user set/inputted name len (name_len -> input_len)
- Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

V1 -> V2:
Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..3df192a6bdcc 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,32 +3565,35 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, input_len;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
-	if (!uname)
-		return 0;
+
 	if (buf) {
+		input_len = *ulen;
 		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
-		if (err)
-			return err;
-	} else {
-		char zero = '\0';
+		*ulen = len + 1;
 
+		if (uname) {
+			err = bpf_copy_to_user(uname, buf, input_len, len);
+			if (err)
+				return err;
+		}
+	} else if (uname) {
+		char zero = '\0';
 		if (put_user(zero, uname))
 			return -EFAULT;
 	}
@@ -3609,7 +3612,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3620,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3642,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3651,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3677,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
-- 
2.43.0

Re: [PATCH bpf v5 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Jiri Olsa 1 month, 3 weeks ago

On Sun, Oct 06, 2024 at 07:51:30PM +0000, tyrone-wu wrote:
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
> 
> This patch fills `name_len` with the actual size of the string name.
> 
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
> V4 -> V5:
> Link: https://lore.kernel.org/bpf/CALOAHbC5xm7Cbfhau3z5X2PqUhiHECNWAPtJCWiOVqTKmdZp-Q@mail.gmail.com/
> - Check that buf is not NULL before retrieving/using its length
> 
> V3 -> V4:
> Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
> - Split patch into separate kernel and selftest change
> 
> V2 -> V3:
> Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
> - Use clearer variable name for user set/inputted name len (name_len -> input_len)
> - Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32
> 
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
> 
>  kernel/bpf/syscall.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..3df192a6bdcc 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,32 +3565,35 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>  
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -				     char __user *uname, u32 ulen,
> +				     char __user *uname, u32 *ulen,
>  				     u64 *probe_offset, u64 *probe_addr,
>  				     u32 *fd_type, unsigned long *missed)
>  {
>  	const char *buf;
> -	u32 prog_id;
> +	u32 prog_id, input_len;
>  	size_t len;
>  	int err;
>  
> -	if (!ulen ^ !uname)
> +	if (!(*ulen) ^ !uname)
>  		return -EINVAL;
>  
>  	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>  				      probe_offset, probe_addr, missed);
>  	if (err)
>  		return err;
> -	if (!uname)
> -		return 0;
> +
>  	if (buf) {
> +		input_len = *ulen;
>  		len = strlen(buf);
> -		err = bpf_copy_to_user(uname, buf, ulen, len);
> -		if (err)
> -			return err;
> -	} else {
> -		char zero = '\0';
> +		*ulen = len + 1;
>  
> +		if (uname) {
> +			err = bpf_copy_to_user(uname, buf, input_len, len);
> +			if (err)
> +				return err;
> +		}
> +	} else if (uname) {
> +		char zero = '\0';
>  		if (put_user(zero, uname))
>  			return -EFAULT;
>  	}

hm, why not just simple check buf for and keep the rest? seems less complicated..

jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..e393b94b90ec 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,27 +3565,31 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, input_len;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+	if (buf) {
+		input_len = *ulen;
+		len = strlen(buf);
+		*ulen = len + 1;
+	}
 	if (!uname)
 		return 0;
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, input_len, len);
 		if (err)
 			return err;
 	} else {

[PATCH bpf v6 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Tyrone Wu 1 month, 3 weeks ago

From: tyrone-wu <wudevelops@gmail.com>

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: Tyrone Wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
---
V5 -> V6:
Link: https://lore.kernel.org/bpf/ZwOWs_XrBtlTGE24@krava/
- Use simpler buf check while keeping V4
- Fix netdev/checkpatch warning for 80 cols exceeded
- Fix Signed-off-by to use real name instead of git username

V4 -> V5:
Link: https://lore.kernel.org/bpf/CALOAHbC5xm7Cbfhau3z5X2PqUhiHECNWAPtJCWiOVqTKmdZp-Q@mail.gmail.com/
- Check that buf is not NULL before retrieving/using its length

V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

V2 -> V3:
Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
- Use clearer variable name for user set/inputted name len (name_len -> input_len)
- Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

V1 -> V2:
Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..b5a7e428ac16 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,27 +3565,33 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulen,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, input_len;
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
+	if (!(*ulen) ^ !uname)
 		return -EINVAL;
 
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	if (buf) {
+		input_len = *ulen;
+		len = strlen(buf);
+		*ulen = len + 1;
+	}
 	if (!uname)
 		return 0;
+
 	if (buf) {
-		len = strlen(buf);
-		err = bpf_copy_to_user(uname, buf, ulen, len);
+		err = bpf_copy_to_user(uname, buf, input_len, len);
 		if (err)
 			return err;
 	} else {
@@ -3609,7 +3615,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3623,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3645,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3654,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3680,19 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL,
+					NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
-- 
2.43.0

Re: [PATCH bpf v6 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Andrii Nakryiko 1 month, 3 weeks ago

On Mon, Oct 7, 2024 at 11:29 AM Tyrone Wu <wudevelops@gmail.com> wrote:
>
> From: tyrone-wu <wudevelops@gmail.com>
>
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
>
> This patch fills `name_len` with the actual size of the string name.
>
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: Tyrone Wu <wudevelops@gmail.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> V5 -> V6:
> Link: https://lore.kernel.org/bpf/ZwOWs_XrBtlTGE24@krava/
> - Use simpler buf check while keeping V4
> - Fix netdev/checkpatch warning for 80 cols exceeded
> - Fix Signed-off-by to use real name instead of git username
>
> V4 -> V5:
> Link: https://lore.kernel.org/bpf/CALOAHbC5xm7Cbfhau3z5X2PqUhiHECNWAPtJCWiOVqTKmdZp-Q@mail.gmail.com/
> - Check that buf is not NULL before retrieving/using its length
>
> V3 -> V4:
> Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
> - Split patch into separate kernel and selftest change
>
> V2 -> V3:
> Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
> - Use clearer variable name for user set/inputted name len (name_len -> input_len)
> - Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32
>
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/

Please drop all the Link: tags, our scripts will accumulate all of
them in the final commit and it becomes very confusing. Our script
will add the final Link: for your latest applied version anyways.

Also, git still shows:

Author: tyrone-wu <wudevelops@gmail.com>

So please try to fix that.

But generally, looks good and I almost applied, but see one small
issue below, which I think needs fixing.

pw-bot: cr

> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
>
>  kernel/bpf/syscall.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..b5a7e428ac16 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,27 +3565,33 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -                                    char __user *uname, u32 ulen,
> +                                    char __user *uname, u32 *ulen,

nit: I'd do what Jiri suggested, use `u32 *ulenp` here

>                                      u64 *probe_offset, u64 *probe_addr,
>                                      u32 *fd_type, unsigned long *missed)
>  {
>         const char *buf;
> -       u32 prog_id;
> +       u32 prog_id, input_len;

and then just `u32 ulen;` here.

>         size_t len;
>         int err;
>

start with just

ulen = *ulenp;

> -       if (!ulen ^ !uname)
> +       if (!(*ulen) ^ !uname)

and remove all the `*ulen` uses except the final assignment

>                 return -EINVAL;
>
>         err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>                                       probe_offset, probe_addr, missed);
>         if (err)
>                 return err;
> +
> +       if (buf) {
> +               input_len = *ulen;
> +               len = strlen(buf);
> +               *ulen = len + 1;
> +       }

don't we need

} else {
    *ulen = 1;
}

for cases when we don't have buf returned from
bpf_get_perf_event_info()? Though I don't think it can happen
currently, existing code is clearly ready to handle that case, so
let's keep it consistent.

>         if (!uname)
>                 return 0;
> +
>         if (buf) {
> -               len = strlen(buf);
> -               err = bpf_copy_to_user(uname, buf, ulen, len);
> +               err = bpf_copy_to_user(uname, buf, input_len, len);
>                 if (err)
>                         return err;
>         } else {
> @@ -3609,7 +3615,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
>         ulen = info->perf_event.kprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, &missed);
>         if (err)
>                 return err;
> @@ -3617,7 +3623,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> -
> +       info->perf_event.kprobe.name_len = ulen;
>         info->perf_event.kprobe.offset = offset;
>         info->perf_event.kprobe.missed = missed;
>         if (!kallsyms_show_value(current_cred()))
> @@ -3639,7 +3645,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
>         ulen = info->perf_event.uprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, NULL);
>         if (err)
>                 return err;
> @@ -3648,6 +3654,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_UPROBE;
> +       info->perf_event.uprobe.name_len = ulen;
>         info->perf_event.uprobe.offset = offset;
>         info->perf_event.uprobe.cookie = event->bpf_cookie;
>         return 0;
> @@ -3673,12 +3680,19 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
>  {
>         char __user *uname;
>         u32 ulen;
> +       int err;
>
>         uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
>         ulen = info->perf_event.tracepoint.name_len;
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL,
> +                                       NULL);

nit: keep it on a single line

> +       if (err)
> +               return err;
> +
>         info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
> +       info->perf_event.tracepoint.name_len = ulen;
>         info->perf_event.tracepoint.cookie = event->bpf_cookie;
> -       return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
> +       return 0;
>  }
>
>  static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
> --
> 2.43.0
>

[PATCH bpf v7 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Tyrone Wu 1 month, 2 weeks ago

Previously when retrieving `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
kernel, leaving it to reflect the value initially set by the user. This
behavior was inconsistent with how other input/output string buffer
fields function (e.g. `raw_tracepoint.tp_name_len`).

This patch fills `name_len` with the actual size of the string name.

Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Signed-off-by: Tyrone Wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
---
V6 -> V7:
- Use *ulenp suggestion from Jiri for user name_len
- Set user name_len to 1 when buf returned from bpf_get_perf_event_info() is NULL

V5 -> V6:
- Use simpler buf check while keeping V4
- Fix netdev/checkpatch warning for 80 cols exceeded
- Fix Signed-off-by to use real name instead of git username

V4 -> V5:
- Check that buf is not NULL before retrieving/using its length

V3 -> V4:
- Split patch into separate kernel and selftest change

V2 -> V3:
- Use clearer variable name for user set/inputted name len (name_len -> input_len)
- Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32

V1 -> V2:
- Use user set *ulen in bpf_copy_to_user before overwriting *ulen

 kernel/bpf/syscall.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..8cfa7183d2ef 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3565,15 +3565,16 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 }
 
 static int bpf_perf_link_fill_common(const struct perf_event *event,
-				     char __user *uname, u32 ulen,
+				     char __user *uname, u32 *ulenp,
 				     u64 *probe_offset, u64 *probe_addr,
 				     u32 *fd_type, unsigned long *missed)
 {
 	const char *buf;
-	u32 prog_id;
+	u32 prog_id, ulen;
 	size_t len;
 	int err;
 
+	ulen = *ulenp;
 	if (!ulen ^ !uname)
 		return -EINVAL;
 
@@ -3581,10 +3582,17 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
 				      probe_offset, probe_addr, missed);
 	if (err)
 		return err;
+
+	if (buf) {
+		len = strlen(buf);
+		*ulenp = len + 1;
+	} else {
+		*ulenp = 1;
+	}
 	if (!uname)
 		return 0;
+
 	if (buf) {
-		len = strlen(buf);
 		err = bpf_copy_to_user(uname, buf, ulen, len);
 		if (err)
 			return err;
@@ -3609,7 +3617,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
 	ulen = info->perf_event.kprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, &missed);
 	if (err)
 		return err;
@@ -3617,7 +3625,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_KPROBE;
-
+	info->perf_event.kprobe.name_len = ulen;
 	info->perf_event.kprobe.offset = offset;
 	info->perf_event.kprobe.missed = missed;
 	if (!kallsyms_show_value(current_cred()))
@@ -3639,7 +3647,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 
 	uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
 	ulen = info->perf_event.uprobe.name_len;
-	err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
+	err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
 					&type, NULL);
 	if (err)
 		return err;
@@ -3648,6 +3656,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
 		info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
 	else
 		info->perf_event.type = BPF_PERF_EVENT_UPROBE;
+	info->perf_event.uprobe.name_len = ulen;
 	info->perf_event.uprobe.offset = offset;
 	info->perf_event.uprobe.cookie = event->bpf_cookie;
 	return 0;
@@ -3673,12 +3682,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
 {
 	char __user *uname;
 	u32 ulen;
+	int err;
 
 	uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
 	ulen = info->perf_event.tracepoint.name_len;
+	err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
+	if (err)
+		return err;
+
 	info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
+	info->perf_event.tracepoint.name_len = ulen;
 	info->perf_event.tracepoint.cookie = event->bpf_cookie;
-	return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
+	return 0;
 }
 
 static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
-- 
2.43.0

[PATCH bpf v7 2/2] selftests/bpf: fix perf_event link info name_len assertion

Posted by Tyrone Wu 1 month, 2 weeks ago

Fix `name_len` field assertions in `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint to validate correct name size instead of 0.

Fixes: 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")
Signed-off-by: Tyrone Wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
---
V6 -> V7: no change

V5 -> V6:
- Fix netdev/checkpatch warning for 80 cols exceeded
- Fix Signed-off-by to use real name instead of git username

V4 -> V5: no change

V3 -> V4:
- Split patch into separate kernel and selftest change

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

diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..745c5ada4c4b 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1,
+				  "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +80,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1,
+				  "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +98,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1,
+				  "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

[PATCH bpf v6 2/2] selftests/bpf: fix perf_event link info name_len assertion

Posted by Tyrone Wu 1 month, 3 weeks ago

From: tyrone-wu <wudevelops@gmail.com>

Fix `name_len` field assertions in `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint to validate correct name size instead of 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")
Signed-off-by: Tyrone Wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
---
V5 -> V6:
- Fix netdev/checkpatch warning for 80 cols exceeded
- Fix Signed-off-by to use real name instead of git username

V4 -> V5: no change

V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

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

diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..745c5ada4c4b 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1,
+				  "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +80,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1,
+				  "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +98,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1,
+				  "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

Re: [PATCH bpf v5 1/2] bpf: fix unpopulated name_len field in perf_event link info

Posted by Yafang Shao 1 month, 3 weeks ago

On Mon, Oct 7, 2024 at 3:51 AM tyrone-wu <wudevelops@gmail.com> wrote:
>
> Previously when retrieving `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint, the `name_len` field was not populated by the
> kernel, leaving it to reflect the value initially set by the user. This
> behavior was inconsistent with how other input/output string buffer
> fields function (e.g. `raw_tracepoint.tp_name_len`).
>
> This patch fills `name_len` with the actual size of the string name.
>
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>

LGTM
Acked-by: Yafang Shao <laoar.shao@gmail.com>

> ---
> V4 -> V5:
> Link: https://lore.kernel.org/bpf/CALOAHbC5xm7Cbfhau3z5X2PqUhiHECNWAPtJCWiOVqTKmdZp-Q@mail.gmail.com/
> - Check that buf is not NULL before retrieving/using its length
>
> V3 -> V4:
> Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
> - Split patch into separate kernel and selftest change
>
> V2 -> V3:
> Link: https://lore.kernel.org/bpf/Zv7sISV0yEyGlEM3@krava/
> - Use clearer variable name for user set/inputted name len (name_len -> input_len)
> - Change (name_len -> input_len) type from size_t to u32 since it's only received and used as u32
>
> V1 -> V2:
> Link: https://lore.kernel.org/bpf/Zv0wl-S13WJnIkb_@krava/
> - Use user set *ulen in bpf_copy_to_user before overwriting *ulen
>
>  kernel/bpf/syscall.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a8f1808a1ca5..3df192a6bdcc 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3565,32 +3565,35 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>  }
>
>  static int bpf_perf_link_fill_common(const struct perf_event *event,
> -                                    char __user *uname, u32 ulen,
> +                                    char __user *uname, u32 *ulen,
>                                      u64 *probe_offset, u64 *probe_addr,
>                                      u32 *fd_type, unsigned long *missed)
>  {
>         const char *buf;
> -       u32 prog_id;
> +       u32 prog_id, input_len;
>         size_t len;
>         int err;
>
> -       if (!ulen ^ !uname)
> +       if (!(*ulen) ^ !uname)
>                 return -EINVAL;
>
>         err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>                                       probe_offset, probe_addr, missed);
>         if (err)
>                 return err;
> -       if (!uname)
> -               return 0;
> +
>         if (buf) {
> +               input_len = *ulen;
>                 len = strlen(buf);
> -               err = bpf_copy_to_user(uname, buf, ulen, len);
> -               if (err)
> -                       return err;
> -       } else {
> -               char zero = '\0';
> +               *ulen = len + 1;
>
> +               if (uname) {
> +                       err = bpf_copy_to_user(uname, buf, input_len, len);
> +                       if (err)
> +                               return err;
> +               }
> +       } else if (uname) {
> +               char zero = '\0';
>                 if (put_user(zero, uname))
>                         return -EFAULT;
>         }
> @@ -3609,7 +3612,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
>         ulen = info->perf_event.kprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, &missed);
>         if (err)
>                 return err;
> @@ -3617,7 +3620,7 @@ static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> -
> +       info->perf_event.kprobe.name_len = ulen;
>         info->perf_event.kprobe.offset = offset;
>         info->perf_event.kprobe.missed = missed;
>         if (!kallsyms_show_value(current_cred()))
> @@ -3639,7 +3642,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>
>         uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
>         ulen = info->perf_event.uprobe.name_len;
> -       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, &offset, &addr,
>                                         &type, NULL);
>         if (err)
>                 return err;
> @@ -3648,6 +3651,7 @@ static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
>                 info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
>         else
>                 info->perf_event.type = BPF_PERF_EVENT_UPROBE;
> +       info->perf_event.uprobe.name_len = ulen;
>         info->perf_event.uprobe.offset = offset;
>         info->perf_event.uprobe.cookie = event->bpf_cookie;
>         return 0;
> @@ -3673,12 +3677,18 @@ static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
>  {
>         char __user *uname;
>         u32 ulen;
> +       int err;
>
>         uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
>         ulen = info->perf_event.tracepoint.name_len;
> +       err = bpf_perf_link_fill_common(event, uname, &ulen, NULL, NULL, NULL, NULL);
> +       if (err)
> +               return err;
> +
>         info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
> +       info->perf_event.tracepoint.name_len = ulen;
>         info->perf_event.tracepoint.cookie = event->bpf_cookie;
> -       return bpf_perf_link_fill_common(event, uname, ulen, NULL, NULL, NULL, NULL);
> +       return 0;
>  }
>
>  static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
> --
> 2.43.0
>


-- 
Regards
Yafang

[PATCH bpf v5 2/2] selftests/bpf: fix perf_event link info name_len assertion

Posted by tyrone-wu 1 month, 3 weeks ago

Fix `name_len` field assertions in `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint to validate correct name size instead of 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
V4 -> V5: no change

V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

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

diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..59077f260404 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0

Re: [PATCH bpf v5 2/2] selftests/bpf: fix perf_event link info name_len assertion

Posted by Yafang Shao 1 month, 3 weeks ago

On Mon, Oct 7, 2024 at 3:51 AM tyrone-wu <wudevelops@gmail.com> wrote:
>
> Fix `name_len` field assertions in `bpf_link_info.perf_event` for
> kprobe/uprobe/tracepoint to validate correct name size instead of 0.
>
> Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
> Fixes: 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")
> Signed-off-by: tyrone-wu <wudevelops@gmail.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Yafang Shao <laoar.shao@gmail.com>

> ---
> V4 -> V5: no change
>
> V3 -> V4:
> Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
> - Split patch into separate kernel and selftest change
>
>  tools/testing/selftests/bpf/prog_tests/fill_link_info.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> index f3932941bbaa..59077f260404 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> @@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>
>                 ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
>
> +               ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
>                 if (!info.perf_event.kprobe.func_name) {
> -                       ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
>                         info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
>                         info.perf_event.kprobe.name_len = sizeof(buf);
>                         goto again;
> @@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>                 ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
>                 break;
>         case BPF_PERF_EVENT_TRACEPOINT:
> +               ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
>                 if (!info.perf_event.tracepoint.tp_name) {
> -                       ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
>                         info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
>                         info.perf_event.tracepoint.name_len = sizeof(buf);
>                         goto again;
> @@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
>         case BPF_PERF_EVENT_URETPROBE:
>                 ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
>
> +               ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
>                 if (!info.perf_event.uprobe.file_name) {
> -                       ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
>                         info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
>                         info.perf_event.uprobe.name_len = sizeof(buf);
>                         goto again;
> --
> 2.43.0
>


-- 
Regards
Yafang

[PATCH bpf v4 2/2] selftests/bpf: fix perf_event link info name_len assertion

Posted by tyrone-wu 1 month, 3 weeks ago

Fix `name_len` field assertions in `bpf_link_info.perf_event` for
kprobe/uprobe/tracepoint to validate correct name size instead of 0.

Link: https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/
Fixes: 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")
Signed-off-by: tyrone-wu <wudevelops@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
V3 -> V4:
Link: https://lore.kernel.org/bpf/Zv_PP6Gs5cq3W2Ey@krava/
- Split patch into separate kernel and selftest change

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

diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index f3932941bbaa..59077f260404 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 
 		ASSERT_EQ(info.perf_event.kprobe.cookie, PERF_EVENT_COOKIE, "kprobe_cookie");
 
+		ASSERT_EQ(info.perf_event.kprobe.name_len, strlen(KPROBE_FUNC) + 1, "name_len");
 		if (!info.perf_event.kprobe.func_name) {
-			ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
 			info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
 			info.perf_event.kprobe.name_len = sizeof(buf);
 			goto again;
@@ -79,8 +79,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 		ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
 		break;
 	case BPF_PERF_EVENT_TRACEPOINT:
+		ASSERT_EQ(info.perf_event.tracepoint.name_len, strlen(TP_NAME) + 1, "name_len");
 		if (!info.perf_event.tracepoint.tp_name) {
-			ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
 			info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
 			info.perf_event.tracepoint.name_len = sizeof(buf);
 			goto again;
@@ -96,8 +96,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long add
 	case BPF_PERF_EVENT_URETPROBE:
 		ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
 
+		ASSERT_EQ(info.perf_event.uprobe.name_len, strlen(UPROBE_FILE) + 1, "name_len");
 		if (!info.perf_event.uprobe.file_name) {
-			ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
 			info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
 			info.perf_event.uprobe.name_len = sizeof(buf);
 			goto again;
-- 
2.43.0