bpf_link_info: perf_event link info name_len field returning zero
Subsystems (4)
Hello,
When retrieving bpf_link_info.perf_event kprobe/uprobe/tracepoint data, I noticed that the name_len field always returns 0. After some digging, I see that name_len is never actually populated, which explains the 0 value.
I expected it to function similarly to bpf_link_info.raw_tracepoint.tp_name_len, where that field is filled with the length of tp_name. However, I noticed that the selftest explicitly asserts that name_len should be 0. I was wondering if someone could clarify whether it is intended for the bpf_link_info.perf_event name_len field to not be populated.
I apologize if this is not the appropriate place to ask this question. Thank you for your time!
Quoted 8 lines · Tyrone Wu
Hello,
When retrieving bpf_link_info.perf_event kprobe/uprobe/tracepoint data, I noticed that the
name_lenfield always returns 0. After some digging, I see thatname_lenis never actually populated, which explains the 0 value.
Quoted 6 lines
I expected it to function similarly to bpf_link_info.raw_tracepoint.tp_name_len, where that field is filled with the length of
tp_name. However, I noticed that the selftest explicitly asserts thatname_lenshould be 0. I was wondering if someone could clarify whether it is intended for the bpf_link_info.perf_eventname_lenfield to not be populated.
This sounds like a bug. It should behave consistently with the other users of input/output string buffer size fields: on input we get maximum buffer size, on output we should put an actual size of the string (especially if it was truncated).
Yafang, Jiri, WDYT?
Quoted 4 lines
I apologize if this is not the appropriate place to ask this question. Thank you for your time!
Quoted 17 lines · Andrii Nakryiko
Quoted 8 lines · Tyrone Wu
Hello,
When retrieving bpf_link_info.perf_event kprobe/uprobe/tracepoint data, I noticed that the
name_lenfield always returns 0. After some digging, I see thatname_lenis never actually populated, which explains the 0 value.Quoted 6 lines
I expected it to function similarly to bpf_link_info.raw_tracepoint.tp_name_len, where that field is filled with the length of
tp_name. However, I noticed that the selftest explicitly asserts thatname_lenshould be 0. I was wondering if someone could clarify whether it is intended for the bpf_link_info.perf_eventname_lenfield to not be populated.
Quoted 6 lines
This sounds like a bug. It should behave consistently with the other users of input/output string buffer size fields: on input we get maximum buffer size, on output we should put an actual size of the string (especially if it was truncated).
Yafang, Jiri, WDYT?
The reason name_len is 0 is that the user did not set both the buffer and the length. IOW, this happens when the user buffer is NULL and the input length is 0. However, we should make this behavior consistent by returning the actual size to the user if both the buffer and length are unset.
I will submit a fix.
--
Regards
Yafang
Quoted 3 lines
The reason
name_lenis 0 is that the user did not set both the buffer and the length. IOW, this happens when the user buffer is NULL and the input length is 0.
Thank you both for the follow-up.
Quoted 5 lines
However, we should make this behavior consistent by returning the actual size to the user if both the buffer and length are unset.
I will submit a fix.
I actually made a small patch for this when I was initially exploring this behavior. If it's alright, I can submit this after some clean-up. :)
Quoted 39 lines · Yafang Shao
Quoted 17 lines · Andrii Nakryiko
Quoted 8 lines · Tyrone Wu
Hello,
When retrieving bpf_link_info.perf_event kprobe/uprobe/tracepoint data, I noticed that the
name_lenfield always returns 0. After some digging, I see thatname_lenis never actually populated, which explains the 0 value.Quoted 6 lines
I expected it to function similarly to bpf_link_info.raw_tracepoint.tp_name_len, where that field is filled with the length of
tp_name. However, I noticed that the selftest explicitly asserts thatname_lenshould be 0. I was wondering if someone could clarify whether it is intended for the bpf_link_info.perf_eventname_lenfield to not be populated.Quoted 6 lines
This sounds like a bug. It should behave consistently with the other users of input/output string buffer size fields: on input we get maximum buffer size, on output we should put an actual size of the string (especially if it was truncated).
Yafang, Jiri, WDYT?
The reason
name_lenis 0 is that the user did not set both the buffer and the length. IOW, this happens when the user buffer is NULL and the input length is 0. However, we should make this behavior consistent by returning the actual size to the user if both the buffer and length are unset.I will submit a fix.
--
Regards
Yafang
A message that is not in the archive (sent off-list or not yet received): CABVU1kWDy4vPM-Kw1fGEyFtZqYkBcbB-2hktO2CBxE1P0L350w@mail.gmail.com
Quoted 8 lines · Tyrone Wu
Quoted 5 lines
The reason
name_lenis 0 is that the user did not set both the buffer and the length. IOW, this happens when the user buffer is NULL and the input length is 0.Thank you both for the follow-up.
Quoted 7 lines
Quoted 5 lines
However, we should make this behavior consistent by returning the actual size to the user if both the buffer and length are unset.
I will submit a fix.
I actually made a small patch for this when I was initially exploring this behavior. If it's alright, I can submit this after some clean-up. :)
That's great. Please do it.
Signature · 2 lines
Regards Yafang
Quoted 24 lines · Yafang Shao
Quoted 22 lines · Andrii Nakryiko
Quoted 8 lines · Tyrone Wu
Hello,
When retrieving bpf_link_info.perf_event kprobe/uprobe/tracepoint data, I noticed that the
name_lenfield always returns 0. After some digging, I see thatname_lenis never actually populated, which explains the 0 value.Quoted 6 lines
I expected it to function similarly to bpf_link_info.raw_tracepoint.tp_name_len, where that field is filled with the length of
tp_name. However, I noticed that the selftest explicitly asserts thatname_lenshould be 0. I was wondering if someone could clarify whether it is intended for the bpf_link_info.perf_eventname_lenfield to not be populated.This sounds like a bug. It should behave consistently with the other users of input/output string buffer size fields: on input we get maximum buffer size, on output we should put an actual size of the string (especially if it was truncated).
Quoted 7 lines
Quoted 1 line
Yafang, Jiri, WDYT?
The reason
name_lenis 0 is that the user did not set both the buffer and the length. IOW, this happens when the user buffer is NULL and the input length is 0. However, we should make this behavior consistent by returning the actual size to the user if both the buffer and length are unset.
yep, makes sense the same way rawtp does that
thanks,
jirka
PATCH 1/1 bpf: fix unpopulated name_len field in perf_event link info
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.
--- kernel/bpf/syscall.c | 25 +++++++++++++------ .../selftests/bpf/prog_tests/fill_link_info.c | 6 ++--- 2 files changed, 20 insertions(+), 11 deletions(-)
kernel/bpf/syscall.c +17 -8
@@ -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,
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3 -3
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf] bpf: fix unpopulated name_len field in perf_event link info
Quoted 36 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name. The
relevant selftests have also been updated to assert thatname_len
contains the correct size rather than 0.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>--- kernel/bpf/syscall.c | 25 +++++++++++++------ .../selftests/bpf/prog_tests/fill_link_info.c | 6 ++--- 2 files changed, 20 insertions(+), 11 deletions(-)kernel/bpf/syscall.c +1
-2@@ -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)
kernel/bpf/syscall.c static int bpf_perf_link_fill_common(const struct perf_event *event, · line 3586
Quoted 10 lines
kernel/bpf/syscall.c +4
-0+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
Quoted 18 lines
kernel/bpf/syscall.c +2
-3if (!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 v2 1/1 bpf: fix unpopulated name_len field in perf_event link info
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.
---
V1 -> V2:
- 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(-)
kernel/bpf/syscall.c +20 -9
@@ -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,
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3 -3
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v2] bpf: fix unpopulated name_len field in perf_event link info
Quoted 63 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name. The
relevant selftests have also been updated to assert thatname_len
contains the correct size rather than 0.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>---
V1 -> V2:- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 29 +++++++++++++------ .../selftests/bpf/prog_tests/fill_link_info.c | 6 ++-- 2 files changed, 23 insertions(+), 12 deletions(-)kernel/bpf/syscall.c +9
-5@@ -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;
Quoted 1 line
} else {
small nit.. up to you but I'd suggest bit different naming like below, otherwise looks good
thanks,
jirka
---
kernel/bpf/syscall.c +6 -4
@@ -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 v3 1/1 bpf: fix unpopulated name_len field in perf_event link info
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.
---
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 +++++++++++++------ .../selftests/bpf/prog_tests/fill_link_info.c | 6 ++-- 2 files changed, 23 insertions(+), 12 deletions(-)
kernel/bpf/syscall.c +20 -9
@@ -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,
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3 -3
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v3] bpf: fix unpopulated name_len field in perf_event link info
Quoted 10 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name. The
relevant selftests have also been updated to assert thatname_len
contains the correct size rather than 0.
Quoted 8 lines
Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>---
V2 -> V3:- Use clearer variable name for user set/inputted name len (
name_len->input_len)
- Change (name_len->input_len) type fromsize_tto 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
Quoted 142 lines
V1 -> V2:
- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 29 +++++++++++++------ .../selftests/bpf/prog_tests/fill_link_info.c | 6 ++-- 2 files changed, 23 insertions(+), 12 deletions(-)kernel/bpf/syscall.c +20
-9@@ -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,tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3
-3@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;Signature · 1 line
2.43.0
PATCH v4 1/2 bpf: fix unpopulated name_len field in perf_event link info
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.
---
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, 20 insertions(+), 9 deletions(-)
kernel/bpf/syscall.c +20 -9
@@ -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,
Signature · 1 line
2.43.0
PATCH v4 2/2 selftests/bpf: fix perf_event link info name_len assertion
Fix name_len field assertions in bpf_link_info.perf_event for kprobe/uprobe/tracepoint to validate correct name size instead of 0.
---
V3 -> V4:
- 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(-)
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3 -3
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v4 1/2] bpf: fix unpopulated name_len field in perf_event link info
In reply to tyrone-wuQuoted 50 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>Acked-by Jiri Olsa <[email protected]>---
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 fromsize_tto u32 since it's only received and used as u32V1 -> V2:
- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)kernel/bpf/syscall.c +2
-3@@ -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)
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3586
Quoted 10 lines
kernel/bpf/syscall.c +4
-0+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.
Quoted 67 lines
kernel/bpf/syscall.c +14
-7+ *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, --
Quoted 2 lines
2.43.0
--
Regards
Yafang
PATCH v5 1/2 bpf: fix unpopulated name_len field in perf_event link info
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.
---
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 | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)
kernel/bpf/syscall.c +24 -14
@@ -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,
Signature · 1 line
2.43.0
PATCH v5 2/2 selftests/bpf: fix perf_event link info name_len assertion
Fix name_len field assertions in bpf_link_info.perf_event for kprobe/uprobe/tracepoint to validate correct name size instead of 0.
---
V4 -> V5: no change
V3 -> V4:
- 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(-)
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3 -3
@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v5 2/2] selftests/bpf: fix perf_event link info name_len assertion
Quoted 8 lines · tyrone-wu
Fix
name_lenfield assertions inbpf_link_info.perf_eventfor kprobe/uprobe/tracepoint to validate correct name size instead of 0.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 23cf7aa539dc ("selftests/bpf: Add selftest for fill_link_info")Signed-off-by tyrone-wu <[email protected]>Acked-by Jiri Olsa <[email protected]>
Quoted 47 lines
---
V4 -> V5: no changeV3 -> V4:
- 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(-)tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3
-4@@ -67,8 +67,8 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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
Signature · 2 lines
Regards Yafang
Re: [PATCH bpf v5 1/2] bpf: fix unpopulated name_len field in perf_event link info
In reply to tyrone-wuQuoted 7 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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).
Quoted 6 lines
This patch fills
name_lenwith the actual size of the string name.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>Acked-by Jiri Olsa <[email protected]>
LGTM
Quoted 129 lines
---
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 fromsize_tto u32 since it's only received and used as u32V1 -> V2:
- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)kernel/bpf/syscall.c +24
-15@@ -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
Signature · 2 lines
Regards Yafang
Re: [PATCH bpf v5 1/2] bpf: fix unpopulated name_len field in perf_event link info
In reply to tyrone-wuQuoted 73 lines · tyrone-wu
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by tyrone-wu <[email protected]>Acked-by Jiri Olsa <[email protected]>---
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 fromsize_tto u32 since it's only received and used as u32V1 -> V2:
- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)kernel/bpf/syscall.c +6
-10@@ -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 ;
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3599
Quoted 10 lines
kernel/bpf/syscall.c +7
-0+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
---
kernel/bpf/syscall.c +9 -5
@@ -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 v6 1/2 bpf: fix unpopulated name_len field in perf_event link info
From: tyrone-wu <[email protected]>
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.
---
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 | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
kernel/bpf/syscall.c +23 -9
@@ -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,
Signature · 1 line
2.43.0
PATCH v6 2/2 selftests/bpf: fix perf_event link info name_len assertion
From: tyrone-wu <[email protected]>
Fix name_len field assertions in bpf_link_info.perf_event for kprobe/uprobe/tracepoint to validate correct name size instead of 0.
---
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(-)
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +6 -3
@@ -67,8 +67,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v6 1/2] bpf: fix unpopulated name_len field in perf_event link info
In reply to Tyrone WuQuoted 31 lines · Tyrone Wu
From: tyrone-wu <[email protected]>
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name.Link https://lore.kernel.org/bpf/CABVU1kXwQXhqQGe0RTrr7eegtM6SVW_KayZBy16-yb0Snztmtg@mail.gmail.com/ open hereFixes 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")Signed-off-by Tyrone Wu <[email protected]>Acked-by Jiri Olsa <[email protected]>Acked-by Yafang Shao <[email protected]>---
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 usernameV4 -> V5:
- Check that buf is not NULL before retrieving/using its length
V3 -> V4:
- Split patch into separate kernel and selftest change
Quoted 7 lines
V2 -> V3:
- Use clearer variable name for user set/inputted name len (
name_len->input_len)
- Change (name_len->input_len) type fromsize_tto u32 since it's only received and used as u32V1 -> V2:
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 <[email protected]>
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
Quoted 10 lines
- Use user set *ulen in
bpf_copy_to_userbefore overwriting *ulenkernel/bpf/syscall.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)kernel/bpf/syscall.c +0
-0
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3568
Quoted 5 lines
kernel/bpf/syscall.c +1
-1@@ -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
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3573
Quoted 6 lines
kernel/bpf/syscall.c +1
-1u64 *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.
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3576
Quoted 3 lines
kernel/bpf/syscall.c +0
-0size_t len;int err;
start with just
ulen = *ulenp;
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3577
Quoted 2 lines
kernel/bpf/syscall.c +1
-1-if (!ulen ^ !uname) +if (!(*ulen) ^ !uname)
and remove all the *ulen uses except the final assignment
kernel/bpf/syscall.c static void bpf_perf_link_dealloc(struct bpf_link *link) · line 3589
Quoted 12 lines
kernel/bpf/syscall.c +6
-0return -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.
Quoted 46 lines
kernel/bpf/syscall.c +6
-5if (!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 ;
kernel/bpf/syscall.c static int bpf_perf_link_fill_tracepoint(const struct perf_event *event, · line 3688
Quoted 9 lines
kernel/bpf/syscall.c +3
-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
Quoted 14 lines
kernel/bpf/syscall.c +5
-2+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 v7 1/2 bpf: fix unpopulated name_len field in perf_event link info
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.
---
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(-)
kernel/bpf/syscall.c +22 -7
@@ -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,
Signature · 1 line
2.43.0
PATCH v7 2/2 selftests/bpf: fix perf_event link info name_len assertion
Fix name_len field assertions in bpf_link_info.perf_event for kprobe/uprobe/tracepoint to validate correct name size instead of 0.
---
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(-)
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +6 -3
@@ -67,8 +67,9 @@ static int verify_perf_link_info(int fd, enum bpf_perf_event_type type, long addASSERT_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 addASSERT_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 addcase 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;
Signature · 1 line
2.43.0
Re: [PATCH bpf v7 1/2] bpf: fix unpopulated name_len field in perf_event link info
In reply to Tyrone WuThis series was applied to bpf/bpf.git (master) by Alexei Starovoitov <[email protected]>:
Hello:
This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <[email protected]>:
Quoted 9 lines · On Tue, 8 Oct 2024 16:43:11 +0000 you wrote:
Previously when retrieving
bpf_link_info.perf_eventfor kprobe/uprobe/tracepoint, thename_lenfield 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_lenwith the actual size of the string name.[...]
Here is the summary with links:
- [bpf,v7,1/2] bpf: fix unpopulated name_len field in perf_event link info
https://git.kernel.org/bpf/bpf/c/4deecdd29cf2 - [bpf,v7,2/2] selftests/bpf: fix perf_event link info name_len assertion https://git.kernel.org/bpf/bpf/c/4538a38f654a
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html