| Message ID | 20241015151117.5562bd41@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs: Have tracefs_dynevent_get_all() find kprobes and uprobes properly | expand |
On 15/10/2024 8:11 pm, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The function tracefs_dynevent_get_all() will only look at the > dynamic_events file if it exists to find matching probes. But because > uprobes and kprobes both use the same prefix "p" as well as uretprobes and > kretprobes (with prefix "r"), it cannot use the dynamic_events file to > differentiate between the two. > > Have kprobes and uprobes always use their own files (kprobe_events and > uprobe_events) even if dynamic_events file exists. > > Also cleaned up the code to be a bit more efficient. > > Link: https://lore.kernel.org/all/20241015123831.347ff0f4@gandalf.local.home/ > > Fixes: b04f18b005c6b ("libtracefs: New APIs for dynamic events") > Reported-by: Metin Kaya <metin.kaya@arm.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > src/tracefs-dynevents.c | 113 +++++++++++++++++++++------------------- > 1 file changed, 60 insertions(+), 53 deletions(-) > > diff --git a/src/tracefs-dynevents.c b/src/tracefs-dynevents.c > index 85c1fcd9d5d5..6b9ba539fde3 100644 > --- a/src/tracefs-dynevents.c > +++ b/src/tracefs-dynevents.c > @@ -34,23 +34,21 @@ static int dyn_generic_del(struct dyn_events_desc *, struct tracefs_dynevent *); > static int dyn_synth_del(struct dyn_events_desc *, struct tracefs_dynevent *); > > struct dyn_events_desc { > - enum tracefs_dynevent_type type; > - const char *file; > - const char *prefix; > + enum tracefs_dynevent_type type; > + const char *file; > + const char *prefix; > int (*del)(struct dyn_events_desc *desc, struct tracefs_dynevent *dyn); > int (*parse)(struct dyn_events_desc *desc, const char *group, > char *line, struct tracefs_dynevent **ret_dyn); > } dynevents[] = { > - {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, > + {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, > {TRACEFS_DYNEVENT_KRETPROBE, KPROBE_EVENTS, "r", dyn_generic_del, dyn_generic_parse}, > - {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, > + {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, > {TRACEFS_DYNEVENT_URETPROBE, UPROBE_EVENTS, "r", dyn_generic_del, dyn_generic_parse}, > - {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, dyn_generic_parse}, > - {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, dyn_synth_parse}, > + {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, dyn_generic_parse}, > + {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, dyn_synth_parse}, > }; > > - > - > static int dyn_generic_del(struct dyn_events_desc *desc, struct tracefs_dynevent *dyn) > { > char *str; > @@ -280,8 +278,13 @@ static void init_devent_desc(void) > return; > > /* Use ftrace dynamic_events, if available */ > - for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) > + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { > + /* kprobes and uprobes do not use default file */ > + if (dynevents[i].prefix[0] == 'p' || > + dynevents[i].prefix[0] == 'r') > + continue; > dynevents[i].file = DYNEVENTS_EVENTS; > + } > > dynevents[EVENT_INDEX(TRACEFS_DYNEVENT_SYNTH)].prefix = "s"; > } > @@ -480,16 +483,15 @@ int tracefs_dynevent_destroy(struct tracefs_dynevent *devent, bool force) > return desc->del(desc, devent); > } > > -static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system, > - struct tracefs_dynevent ***ret_all) > +static int get_dynevent(enum tracefs_dynevent_type type, const char *system, > + struct tracefs_dynevent ***ret_all, int count) > { > struct dyn_events_desc *desc; > - struct tracefs_dynevent *devent, **tmp, **all = NULL; > + struct tracefs_dynevent *devent, **tmp, **all; > char *content; > - int count = 0; > char *line; > char *next; > - int ret; > + int ret = -1; > > desc = get_devent_desc(type); > if (!desc) > @@ -499,6 +501,9 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system > if (!content) > return -1; > > + if (ret_all) > + all = *ret_all; > + > line = content; > do { > next = strchr(line, '\n'); > @@ -507,11 +512,12 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system > ret = desc->parse(desc, system, line, ret_all ? &devent : NULL); > if (!ret) { > if (ret_all) { > - tmp = realloc(all, (count + 1) * sizeof(*tmp)); > + tmp = realloc(all, (count + 2) * sizeof(*tmp)); > if (!tmp) > - goto error; > + break; > all = tmp; > all[count] = devent; > + all[count + 1] = NULL; > } > count++; > } > @@ -521,12 +527,38 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system > free(content); > if (ret_all) > *ret_all = all; > + > return count; > +} > > -error: > - free(content); > - free(all); > - return -1; > +static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system, > + struct tracefs_dynevent ***ret_all) > +{ > + int count = 0; > + int i; > + > + if (ret_all) > + *ret_all = NULL; > + > + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { > + if (!((1 << i) & type)) > + continue; > + > + count = get_dynevent((1 << i), system, ret_all, count); > + if (count < 0) { > + count = 0; > + break; > + } > + } > + > + if (!count) { > + if (ret_all) { > + free(*ret_all); > + *ret_all = NULL; > + } > + count = -1; > + } > + return count; > } > > /** > @@ -561,41 +593,16 @@ void tracefs_dynevent_list_free(struct tracefs_dynevent **events) > struct tracefs_dynevent ** > tracefs_dynevent_get_all(unsigned int types, const char *system) > { > - struct tracefs_dynevent **events, **tmp, **all_events = NULL; > - int count, all = 0; > - int i; > + struct tracefs_dynevent **events; > + int count; > > - for (i = 1; i < TRACEFS_DYNEVENT_MAX; i <<= 1) { > - if (types) { > - if (i > types) > - break; > - if (!(types & i)) > - continue; > - } > - count = get_all_dynevents(i, system, &events); > - if (count > 0) { > - tmp = realloc(all_events, (all + count + 1) * sizeof(*tmp)); > - if (!tmp) > - goto error; > - all_events = tmp; > - memcpy(all_events + all, events, count * sizeof(*events)); > - all += count; > - /* Add a NULL pointer at the end */ > - all_events[all] = NULL; > - free(events); > - } > + count = get_all_dynevents(types, system, &events); > + if (count <= 0) { > + free(events); > + return NULL; > } > > - return all_events; > - > -error: > - free(events); > - if (all_events) { > - for (i = 0; i < all; i++) > - free(all_events[i]); > - free(all_events); > - } > - return NULL; > + return events; > } > > /** I could successfully apply the patch on 4cbebed79b1f ("libtracefs: Documentation: Add missing documentation to meson.build"). The unit tests at [1] passes with this patch. Just for the records, there are some unit test failures in libtracefs (they were there even before the patch): $ sudo ./utest/trace-utest CUnit - A unit testing framework for C - Version 2.1-3 http://cunit.sourceforge.net/ Memory mapped buffers not supported Suite: tracefs library Test: Test tracefs/debugfs mounting ...FAILED 1. tracefs-utest.c:1806 - ret == 0 Test: trace cpu read ...passed Test: trace cpu read_buf_percent ...passed Test: trace cpu pipe ...passed Test: trace pid events filter ...passed Test: trace pid function filter ...passed Test: trace sql ...passed Test: trace sql trace onmax ...passed Test: trace sql trace onchange ...passed Test: trace sql snapshot onmax ...passed Test: trace sql snapshot onchange ...passed Test: trace sql save onmax ...passed Test: trace sql save onchange ...passed Test: trace sql trace and snapshot onmax ...passed Test: trace sql trace and snapshot onchange ...passed Test: tracing file / directory APIs ...passed Test: instance file / directory APIs ...passed Test: instance file descriptor ...passed Test: instance reset ...passed Test: systems and events APIs ...passed Test: tracefs_iterate_snapshot_events API ...passed Test: tracefs_iterate_raw_events API ...FAILED 1. tracefs-utest.c:235 - ret == sizeof(struct test_sample) 2. tracefs-utest.c:235 - ret == sizeof(struct test_sample) Test: Follow events ...passed Test: Follow events clear ...passed Test: tracefs_tracers API ...passed Test: tracefs_local events API ...passed Test: tracefs_instances_walk API ...passed Test: tracefs_get_clock API ...passed Test: tracing on / off ...passed Test: tracing options ...passed Test: custom system directory ...passed Test: ftrace marker ...passed Test: kprobes ...passed Test: synthetic events ...passed Test: eprobes ...passed Test: uprobes ...FAILED 1. tracefs-utest.c:2222 - ret == 0 2. tracefs-utest.c:2222 - ret == 0 Run Summary: Type Total Ran Passed Failed Inactive suites 1 1 n/a 0 0 tests 36 36 33 3 0 asserts 30341119 30341119 30341114 5 n/a Elapsed time = 124.937 seconds With that, Reviewed-by: Metin Kaya <metin.kaya@arm.com> Thanks a lot for fixing it promptly! [1] https://lore.kernel.org/all/20241015140840.4183007-1-metin.kaya@arm.com/
On 16/10/2024 7:40 am, Metin Kaya wrote: > On 15/10/2024 8:11 pm, Steven Rostedt wrote: >> From: "Steven Rostedt (Google)" <rostedt@goodmis.org> >> >> The function tracefs_dynevent_get_all() will only look at the >> dynamic_events file if it exists to find matching probes. But because >> uprobes and kprobes both use the same prefix "p" as well as uretprobes >> and >> kretprobes (with prefix "r"), it cannot use the dynamic_events file to >> differentiate between the two. >> >> Have kprobes and uprobes always use their own files (kprobe_events and >> uprobe_events) even if dynamic_events file exists. >> >> Also cleaned up the code to be a bit more efficient. >> >> Link: >> https://lore.kernel.org/all/20241015123831.347ff0f4@gandalf.local.home/ >> >> Fixes: b04f18b005c6b ("libtracefs: New APIs for dynamic events") >> Reported-by: Metin Kaya <metin.kaya@arm.com> >> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> >> --- >> src/tracefs-dynevents.c | 113 +++++++++++++++++++++------------------- >> 1 file changed, 60 insertions(+), 53 deletions(-) >> >> diff --git a/src/tracefs-dynevents.c b/src/tracefs-dynevents.c >> index 85c1fcd9d5d5..6b9ba539fde3 100644 >> --- a/src/tracefs-dynevents.c >> +++ b/src/tracefs-dynevents.c >> @@ -34,23 +34,21 @@ static int dyn_generic_del(struct dyn_events_desc >> *, struct tracefs_dynevent *); >> static int dyn_synth_del(struct dyn_events_desc *, struct >> tracefs_dynevent *); >> struct dyn_events_desc { >> - enum tracefs_dynevent_type type; >> - const char *file; >> - const char *prefix; >> + enum tracefs_dynevent_type type; >> + const char *file; >> + const char *prefix; >> int (*del)(struct dyn_events_desc *desc, struct tracefs_dynevent >> *dyn); >> int (*parse)(struct dyn_events_desc *desc, const char *group, >> char *line, struct tracefs_dynevent **ret_dyn); >> } dynevents[] = { >> - {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, >> dyn_generic_parse}, >> + {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, >> dyn_generic_parse}, >> {TRACEFS_DYNEVENT_KRETPROBE, KPROBE_EVENTS, "r", >> dyn_generic_del, dyn_generic_parse}, >> - {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, >> dyn_generic_parse}, >> + {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, >> dyn_generic_parse}, >> {TRACEFS_DYNEVENT_URETPROBE, UPROBE_EVENTS, "r", >> dyn_generic_del, dyn_generic_parse}, >> - {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, >> dyn_generic_parse}, >> - {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, >> dyn_synth_parse}, >> + {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, >> dyn_generic_parse}, >> + {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, >> dyn_synth_parse}, >> }; >> - >> - >> static int dyn_generic_del(struct dyn_events_desc *desc, struct >> tracefs_dynevent *dyn) >> { >> char *str; >> @@ -280,8 +278,13 @@ static void init_devent_desc(void) >> return; >> /* Use ftrace dynamic_events, if available */ >> - for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) >> + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { >> + /* kprobes and uprobes do not use default file */ >> + if (dynevents[i].prefix[0] == 'p' || >> + dynevents[i].prefix[0] == 'r') >> + continue; >> dynevents[i].file = DYNEVENTS_EVENTS; >> + } >> dynevents[EVENT_INDEX(TRACEFS_DYNEVENT_SYNTH)].prefix = "s"; >> } >> @@ -480,16 +483,15 @@ int tracefs_dynevent_destroy(struct >> tracefs_dynevent *devent, bool force) >> return desc->del(desc, devent); >> } >> -static int get_all_dynevents(enum tracefs_dynevent_type type, const >> char *system, >> - struct tracefs_dynevent ***ret_all) >> +static int get_dynevent(enum tracefs_dynevent_type type, const char >> *system, >> + struct tracefs_dynevent ***ret_all, int count) >> { >> struct dyn_events_desc *desc; >> - struct tracefs_dynevent *devent, **tmp, **all = NULL; >> + struct tracefs_dynevent *devent, **tmp, **all; >> char *content; >> - int count = 0; >> char *line; >> char *next; >> - int ret; >> + int ret = -1; >> desc = get_devent_desc(type); >> if (!desc) >> @@ -499,6 +501,9 @@ static int get_all_dynevents(enum >> tracefs_dynevent_type type, const char *system >> if (!content) >> return -1; >> + if (ret_all) >> + all = *ret_all; >> + >> line = content; >> do { >> next = strchr(line, '\n'); >> @@ -507,11 +512,12 @@ static int get_all_dynevents(enum >> tracefs_dynevent_type type, const char *system >> ret = desc->parse(desc, system, line, ret_all ? &devent : >> NULL); >> if (!ret) { >> if (ret_all) { >> - tmp = realloc(all, (count + 1) * sizeof(*tmp)); >> + tmp = realloc(all, (count + 2) * sizeof(*tmp)); >> if (!tmp) >> - goto error; >> + break; >> all = tmp; >> all[count] = devent; >> + all[count + 1] = NULL; >> } >> count++; >> } >> @@ -521,12 +527,38 @@ static int get_all_dynevents(enum >> tracefs_dynevent_type type, const char *system >> free(content); >> if (ret_all) >> *ret_all = all; >> + >> return count; >> +} >> -error: >> - free(content); >> - free(all); >> - return -1; >> +static int get_all_dynevents(enum tracefs_dynevent_type type, const >> char *system, >> + struct tracefs_dynevent ***ret_all) >> +{ >> + int count = 0; >> + int i; >> + >> + if (ret_all) >> + *ret_all = NULL; >> + >> + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { >> + if (!((1 << i) & type)) >> + continue; >> + >> + count = get_dynevent((1 << i), system, ret_all, count); >> + if (count < 0) { >> + count = 0; >> + break; >> + } >> + } >> + >> + if (!count) { >> + if (ret_all) { >> + free(*ret_all); >> + *ret_all = NULL; >> + } >> + count = -1; >> + } >> + return count; >> } >> /** >> @@ -561,41 +593,16 @@ void tracefs_dynevent_list_free(struct >> tracefs_dynevent **events) >> struct tracefs_dynevent ** >> tracefs_dynevent_get_all(unsigned int types, const char *system) >> { >> - struct tracefs_dynevent **events, **tmp, **all_events = NULL; >> - int count, all = 0; >> - int i; >> + struct tracefs_dynevent **events; >> + int count; >> - for (i = 1; i < TRACEFS_DYNEVENT_MAX; i <<= 1) { >> - if (types) { >> - if (i > types) >> - break; >> - if (!(types & i)) >> - continue; >> - } >> - count = get_all_dynevents(i, system, &events); >> - if (count > 0) { >> - tmp = realloc(all_events, (all + count + 1) * sizeof(*tmp)); >> - if (!tmp) >> - goto error; >> - all_events = tmp; >> - memcpy(all_events + all, events, count * sizeof(*events)); >> - all += count; >> - /* Add a NULL pointer at the end */ >> - all_events[all] = NULL; >> - free(events); >> - } >> + count = get_all_dynevents(types, system, &events); >> + if (count <= 0) { >> + free(events); >> + return NULL; >> } >> - return all_events; >> - >> -error: >> - free(events); >> - if (all_events) { >> - for (i = 0; i < all; i++) >> - free(all_events[i]); >> - free(all_events); >> - } >> - return NULL; >> + return events; >> } >> /** > > > I could successfully apply the patch on 4cbebed79b1f ("libtracefs: > Documentation: Add missing documentation to meson.build"). > The unit tests at [1] passes with this patch. > > Just for the records, there are some unit test failures in libtracefs > (they were there even before the patch): > > $ sudo ./utest/trace-utest > CUnit - A unit testing framework for C - Version 2.1-3 > http://cunit.sourceforge.net/ > > Memory mapped buffers not supported > > Suite: tracefs library > Test: Test tracefs/debugfs mounting ...FAILED > 1. tracefs-utest.c:1806 - ret == 0 > Test: trace cpu read ...passed > Test: trace cpu read_buf_percent ...passed > Test: trace cpu pipe ...passed > Test: trace pid events filter ...passed > Test: trace pid function filter ...passed > Test: trace sql ...passed > Test: trace sql trace onmax ...passed > Test: trace sql trace onchange ...passed > Test: trace sql snapshot onmax ...passed > Test: trace sql snapshot onchange ...passed > Test: trace sql save onmax ...passed > Test: trace sql save onchange ...passed > Test: trace sql trace and snapshot onmax ...passed > Test: trace sql trace and snapshot onchange ...passed > Test: tracing file / directory APIs ...passed > Test: instance file / directory APIs ...passed > Test: instance file descriptor ...passed > Test: instance reset ...passed > Test: systems and events APIs ...passed > Test: tracefs_iterate_snapshot_events API ...passed > Test: tracefs_iterate_raw_events API ...FAILED > 1. tracefs-utest.c:235 - ret == sizeof(struct test_sample) > 2. tracefs-utest.c:235 - ret == sizeof(struct test_sample) > Test: Follow events ...passed > Test: Follow events clear ...passed > Test: tracefs_tracers API ...passed > Test: tracefs_local events API ...passed > Test: tracefs_instances_walk API ...passed > Test: tracefs_get_clock API ...passed > Test: tracing on / off ...passed > Test: tracing options ...passed > Test: custom system directory ...passed > Test: ftrace marker ...passed > Test: kprobes ...passed > Test: synthetic events ...passed > Test: eprobes ...passed > Test: uprobes ...FAILED > 1. tracefs-utest.c:2222 - ret == 0 > 2. tracefs-utest.c:2222 - ret == 0 > > Run Summary: Type Total Ran Passed Failed Inactive > suites 1 1 n/a 0 0 > tests 36 36 33 3 0 > asserts 30341119 30341119 30341114 5 n/a > > Elapsed time = 124.937 seconds > > With that, > > Reviewed-by: Metin Kaya <metin.kaya@arm.com> > > > Thanks a lot for fixing it promptly! > > [1] > https://lore.kernel.org/all/20241015140840.4183007-1-metin.kaya@arm.com/ > > Sorry, I need to revoke my Reviewed-by. Because, "trace-cmd reset" cannot destroy uretprobes after this patch. # cd /sys/kernel/tracing/ /sys/kernel/tracing # cat uprobe_events /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events /sys/kernel/tracing # cat uprobe_events r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 /sys/kernel/tracing # trace-cmd reset /sys/kernel/tracing # cat uprobe_events r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 OTOH, "trace-cmd reset" is able to destroy uretprobes if there is also a kprobe in addition to a uretprobe: /sys/kernel/tracing # cat dynamic_events /sys/kernel/tracing # cat uprobe_events /sys/kernel/tracing # echo 'p do_sys_open' > kprobe_events /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events /sys/kernel/tracing # cat dynamic_events p:kprobes/p_do_sys_open_0 do_sys_open r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 /sys/kernel/tracing # cat uprobe_events r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 /sys/kernel/tracing # trace-cmd reset /sys/kernel/tracing # cat dynamic_events /sys/kernel/tracing # cat uprobe_events /sys/kernel/tracing #
On Wed, 16 Oct 2024 09:33:00 +0100 Metin Kaya <metin.kaya@arm.com> wrote: > > > > I could successfully apply the patch on 4cbebed79b1f ("libtracefs: > > Documentation: Add missing documentation to meson.build"). > > The unit tests at [1] passes with this patch. > > > > Just for the records, there are some unit test failures in libtracefs > > (they were there even before the patch): > > > > $ sudo ./utest/trace-utest > > CUnit - A unit testing framework for C - Version 2.1-3 > > http://cunit.sourceforge.net/ > > > > Memory mapped buffers not supported > > > > Suite: tracefs library > > Test: Test tracefs/debugfs mounting ...FAILED > > 1. tracefs-utest.c:1806 - ret == 0 Note, if you have anything in the /sys/kernel/tracing directory (including gdb instances of trace-cmd), it will fail to unmount. I tripped over this too. > > Test: trace cpu read ...passed > > Test: trace cpu read_buf_percent ...passed > > Test: trace cpu pipe ...passed > > Test: trace pid events filter ...passed > > Test: trace pid function filter ...passed > > Test: trace sql ...passed > > Test: trace sql trace onmax ...passed > > Test: trace sql trace onchange ...passed > > Test: trace sql snapshot onmax ...passed > > Test: trace sql snapshot onchange ...passed > > Test: trace sql save onmax ...passed > > Test: trace sql save onchange ...passed > > Test: trace sql trace and snapshot onmax ...passed > > Test: trace sql trace and snapshot onchange ...passed > > Test: tracing file / directory APIs ...passed > > Test: instance file / directory APIs ...passed > > Test: instance file descriptor ...passed > > Test: instance reset ...passed > > Test: systems and events APIs ...passed > > Test: tracefs_iterate_snapshot_events API ...passed > > Test: tracefs_iterate_raw_events API ...FAILED > > 1. tracefs-utest.c:235 - ret == sizeof(struct test_sample) > > 2. tracefs-utest.c:235 - ret == sizeof(struct test_sample) Did you do a trace-cmd reset before running the tests? > > Test: Follow events ...passed > > Test: Follow events clear ...passed > > Test: tracefs_tracers API ...passed > > Test: tracefs_local events API ...passed > > Test: tracefs_instances_walk API ...passed > > Test: tracefs_get_clock API ...passed > > Test: tracing on / off ...passed > > Test: tracing options ...passed > > Test: custom system directory ...passed > > Test: ftrace marker ...passed > > Test: kprobes ...passed > > Test: synthetic events ...passed > > Test: eprobes ...passed > > Test: uprobes ...FAILED > > 1. tracefs-utest.c:2222 - ret == 0 > > 2. tracefs-utest.c:2222 - ret == 0 Did you have left over uprobes? > > > > Run Summary: Type Total Ran Passed Failed Inactive > > suites 1 1 n/a 0 0 > > tests 36 36 33 3 0 > > asserts 30341119 30341119 30341114 5 n/a > > > > Elapsed time = 124.937 seconds > > > > With that, > > > > Reviewed-by: Metin Kaya <metin.kaya@arm.com> > > > > > > Thanks a lot for fixing it promptly! > > > > [1] > > https://lore.kernel.org/all/20241015140840.4183007-1-metin.kaya@arm.com/ > > > > > > Sorry, I need to revoke my Reviewed-by. Because, "trace-cmd reset" > cannot destroy uretprobes after this patch. > > # cd /sys/kernel/tracing/ > /sys/kernel/tracing # cat uprobe_events > /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events > /sys/kernel/tracing # cat uprobe_events > r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 > /sys/kernel/tracing # trace-cmd reset > /sys/kernel/tracing # cat uprobe_events > r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 > > OTOH, "trace-cmd reset" is able to destroy uretprobes if there is also a > kprobe in addition to a uretprobe: Oops, I know why. I had a bug in the code that collects all the probes and looks at different files. It assumed that if the return of tracefs_instance_file_read() returns NULL from kprobe_events, it is an error. But that function also returns NULL if the file is empty. I changed the code from: content = tracefs_instance_file_read(NULL, desc->file, NULL); if (!content) return -1; to: if (!tracefs_file_exists(NULL, desc->file)) return -1; content = tracefs_instance_file_read(NULL, desc->file, NULL); /* File exists, but may be empty */ if (!content) return 0; I'll send out a v2. Thanks for testing. I'll also add more tests to the libtracefs utest to check for this too. -- Steve > > /sys/kernel/tracing # cat dynamic_events > /sys/kernel/tracing # cat uprobe_events > /sys/kernel/tracing # echo 'p do_sys_open' > kprobe_events > /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events > /sys/kernel/tracing # cat dynamic_events > p:kprobes/p_do_sys_open_0 do_sys_open > r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 > /sys/kernel/tracing # cat uprobe_events > r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 > /sys/kernel/tracing # trace-cmd reset > /sys/kernel/tracing # cat dynamic_events > /sys/kernel/tracing # cat uprobe_events > /sys/kernel/tracing #
On 16/10/2024 2:46 pm, Steven Rostedt wrote: > On Wed, 16 Oct 2024 09:33:00 +0100 > Metin Kaya <metin.kaya@arm.com> wrote: > > >>> >>> I could successfully apply the patch on 4cbebed79b1f ("libtracefs: >>> Documentation: Add missing documentation to meson.build"). >>> The unit tests at [1] passes with this patch. >>> >>> Just for the records, there are some unit test failures in libtracefs >>> (they were there even before the patch): >>> >>> $ sudo ./utest/trace-utest >>> CUnit - A unit testing framework for C - Version 2.1-3 >>> http://cunit.sourceforge.net/ >>> >>> Memory mapped buffers not supported >>> >>> Suite: tracefs library >>> Test: Test tracefs/debugfs mounting ...FAILED >>> 1. tracefs-utest.c:1806 - ret == 0 > > Note, if you have anything in the /sys/kernel/tracing directory (including > gdb instances of trace-cmd), it will fail to unmount. I tripped over this > too. I ran "trace-cmd reset && umount /sys/kernel/tracing" before the unit test. And this error is gone. > >>> Test: trace cpu read ...passed >>> Test: trace cpu read_buf_percent ...passed >>> Test: trace cpu pipe ...passed >>> Test: trace pid events filter ...passed >>> Test: trace pid function filter ...passed >>> Test: trace sql ...passed >>> Test: trace sql trace onmax ...passed >>> Test: trace sql trace onchange ...passed >>> Test: trace sql snapshot onmax ...passed >>> Test: trace sql snapshot onchange ...passed >>> Test: trace sql save onmax ...passed >>> Test: trace sql save onchange ...passed >>> Test: trace sql trace and snapshot onmax ...passed >>> Test: trace sql trace and snapshot onchange ...passed >>> Test: tracing file / directory APIs ...passed >>> Test: instance file / directory APIs ...passed >>> Test: instance file descriptor ...passed >>> Test: instance reset ...passed >>> Test: systems and events APIs ...passed >>> Test: tracefs_iterate_snapshot_events API ...passed >>> Test: tracefs_iterate_raw_events API ...FAILED >>> 1. tracefs-utest.c:235 - ret == sizeof(struct test_sample) >>> 2. tracefs-utest.c:235 - ret == sizeof(struct test_sample) > > Did you do a trace-cmd reset before running the tests? Yes. And one of these failures still persists: Test: tracefs_iterate_snapshot_events API ...FAILED 1. tracefs-utest.c:235 - ret == sizeof(struct test_sample) > >>> Test: Follow events ...passed >>> Test: Follow events clear ...passed >>> Test: tracefs_tracers API ...passed >>> Test: tracefs_local events API ...passed >>> Test: tracefs_instances_walk API ...passed >>> Test: tracefs_get_clock API ...passed >>> Test: tracing on / off ...passed >>> Test: tracing options ...passed >>> Test: custom system directory ...passed >>> Test: ftrace marker ...passed >>> Test: kprobes ...passed >>> Test: synthetic events ...passed >>> Test: eprobes ...passed >>> Test: uprobes ...FAILED >>> 1. tracefs-utest.c:2222 - ret == 0 >>> 2. tracefs-utest.c:2222 - ret == 0 > > Did you have left over uprobes? Nope. I ran "trace-cmd reset" before the test and confirmed no uprobes are left. But still getting these failures. > >>> >>> Run Summary: Type Total Ran Passed Failed Inactive >>> suites 1 1 n/a 0 0 >>> tests 36 36 33 3 0 >>> asserts 30341119 30341119 30341114 5 n/a >>> >>> Elapsed time = 124.937 seconds >>> >>> With that, >>> >>> Reviewed-by: Metin Kaya <metin.kaya@arm.com> >>> >>> >>> Thanks a lot for fixing it promptly! >>> >>> [1] >>> https://lore.kernel.org/all/20241015140840.4183007-1-metin.kaya@arm.com/ >>> >>> >> >> Sorry, I need to revoke my Reviewed-by. Because, "trace-cmd reset" >> cannot destroy uretprobes after this patch. >> >> # cd /sys/kernel/tracing/ >> /sys/kernel/tracing # cat uprobe_events >> /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events >> /sys/kernel/tracing # cat uprobe_events >> r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 >> /sys/kernel/tracing # trace-cmd reset >> /sys/kernel/tracing # cat uprobe_events >> r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 >> >> OTOH, "trace-cmd reset" is able to destroy uretprobes if there is also a >> kprobe in addition to a uretprobe: > > Oops, I know why. I had a bug in the code that collects all the probes and > looks at different files. It assumed that if the return of > tracefs_instance_file_read() returns NULL from kprobe_events, it is an > error. But that function also returns NULL if the file is empty. I changed > the code from: > > content = tracefs_instance_file_read(NULL, desc->file, NULL); > if (!content) > return -1; > > to: > > if (!tracefs_file_exists(NULL, desc->file)) > return -1; > > content = tracefs_instance_file_read(NULL, desc->file, NULL); > /* File exists, but may be empty */ > if (!content) > return 0; > > I'll send out a v2. Thanks for testing. > > I'll also add more tests to the libtracefs utest to check for this too. > > -- Steve > > >> >> /sys/kernel/tracing # cat dynamic_events >> /sys/kernel/tracing # cat uprobe_events >> /sys/kernel/tracing # echo 'p do_sys_open' > kprobe_events >> /sys/kernel/tracing # echo 'r /bin/bash:0x4245c0' > uprobe_events >> /sys/kernel/tracing # cat dynamic_events >> p:kprobes/p_do_sys_open_0 do_sys_open >> r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 >> /sys/kernel/tracing # cat uprobe_events >> r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0 >> /sys/kernel/tracing # trace-cmd reset >> /sys/kernel/tracing # cat dynamic_events >> /sys/kernel/tracing # cat uprobe_events >> /sys/kernel/tracing # >
diff --git a/src/tracefs-dynevents.c b/src/tracefs-dynevents.c index 85c1fcd9d5d5..6b9ba539fde3 100644 --- a/src/tracefs-dynevents.c +++ b/src/tracefs-dynevents.c @@ -34,23 +34,21 @@ static int dyn_generic_del(struct dyn_events_desc *, struct tracefs_dynevent *); static int dyn_synth_del(struct dyn_events_desc *, struct tracefs_dynevent *); struct dyn_events_desc { - enum tracefs_dynevent_type type; - const char *file; - const char *prefix; + enum tracefs_dynevent_type type; + const char *file; + const char *prefix; int (*del)(struct dyn_events_desc *desc, struct tracefs_dynevent *dyn); int (*parse)(struct dyn_events_desc *desc, const char *group, char *line, struct tracefs_dynevent **ret_dyn); } dynevents[] = { - {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, + {TRACEFS_DYNEVENT_KPROBE, KPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, {TRACEFS_DYNEVENT_KRETPROBE, KPROBE_EVENTS, "r", dyn_generic_del, dyn_generic_parse}, - {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, + {TRACEFS_DYNEVENT_UPROBE, UPROBE_EVENTS, "p", dyn_generic_del, dyn_generic_parse}, {TRACEFS_DYNEVENT_URETPROBE, UPROBE_EVENTS, "r", dyn_generic_del, dyn_generic_parse}, - {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, dyn_generic_parse}, - {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, dyn_synth_parse}, + {TRACEFS_DYNEVENT_EPROBE, "", "e", dyn_generic_del, dyn_generic_parse}, + {TRACEFS_DYNEVENT_SYNTH, SYNTH_EVENTS, "", dyn_synth_del, dyn_synth_parse}, }; - - static int dyn_generic_del(struct dyn_events_desc *desc, struct tracefs_dynevent *dyn) { char *str; @@ -280,8 +278,13 @@ static void init_devent_desc(void) return; /* Use ftrace dynamic_events, if available */ - for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { + /* kprobes and uprobes do not use default file */ + if (dynevents[i].prefix[0] == 'p' || + dynevents[i].prefix[0] == 'r') + continue; dynevents[i].file = DYNEVENTS_EVENTS; + } dynevents[EVENT_INDEX(TRACEFS_DYNEVENT_SYNTH)].prefix = "s"; } @@ -480,16 +483,15 @@ int tracefs_dynevent_destroy(struct tracefs_dynevent *devent, bool force) return desc->del(desc, devent); } -static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system, - struct tracefs_dynevent ***ret_all) +static int get_dynevent(enum tracefs_dynevent_type type, const char *system, + struct tracefs_dynevent ***ret_all, int count) { struct dyn_events_desc *desc; - struct tracefs_dynevent *devent, **tmp, **all = NULL; + struct tracefs_dynevent *devent, **tmp, **all; char *content; - int count = 0; char *line; char *next; - int ret; + int ret = -1; desc = get_devent_desc(type); if (!desc) @@ -499,6 +501,9 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system if (!content) return -1; + if (ret_all) + all = *ret_all; + line = content; do { next = strchr(line, '\n'); @@ -507,11 +512,12 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system ret = desc->parse(desc, system, line, ret_all ? &devent : NULL); if (!ret) { if (ret_all) { - tmp = realloc(all, (count + 1) * sizeof(*tmp)); + tmp = realloc(all, (count + 2) * sizeof(*tmp)); if (!tmp) - goto error; + break; all = tmp; all[count] = devent; + all[count + 1] = NULL; } count++; } @@ -521,12 +527,38 @@ static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system free(content); if (ret_all) *ret_all = all; + return count; +} -error: - free(content); - free(all); - return -1; +static int get_all_dynevents(enum tracefs_dynevent_type type, const char *system, + struct tracefs_dynevent ***ret_all) +{ + int count = 0; + int i; + + if (ret_all) + *ret_all = NULL; + + for (i = 0; i < EVENT_INDEX(TRACEFS_DYNEVENT_MAX); i++) { + if (!((1 << i) & type)) + continue; + + count = get_dynevent((1 << i), system, ret_all, count); + if (count < 0) { + count = 0; + break; + } + } + + if (!count) { + if (ret_all) { + free(*ret_all); + *ret_all = NULL; + } + count = -1; + } + return count; } /** @@ -561,41 +593,16 @@ void tracefs_dynevent_list_free(struct tracefs_dynevent **events) struct tracefs_dynevent ** tracefs_dynevent_get_all(unsigned int types, const char *system) { - struct tracefs_dynevent **events, **tmp, **all_events = NULL; - int count, all = 0; - int i; + struct tracefs_dynevent **events; + int count; - for (i = 1; i < TRACEFS_DYNEVENT_MAX; i <<= 1) { - if (types) { - if (i > types) - break; - if (!(types & i)) - continue; - } - count = get_all_dynevents(i, system, &events); - if (count > 0) { - tmp = realloc(all_events, (all + count + 1) * sizeof(*tmp)); - if (!tmp) - goto error; - all_events = tmp; - memcpy(all_events + all, events, count * sizeof(*events)); - all += count; - /* Add a NULL pointer at the end */ - all_events[all] = NULL; - free(events); - } + count = get_all_dynevents(types, system, &events); + if (count <= 0) { + free(events); + return NULL; } - return all_events; - -error: - free(events); - if (all_events) { - for (i = 0; i < all; i++) - free(all_events[i]); - free(all_events); - } - return NULL; + return events; } /**