| Message ID | Y8HiIR9IrfWFG+MY@mail.google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls | expand |
On Sat, Jan 14, 2023 at 11:58:41AM +1300, Paulo Miguel Almeida wrote: > In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable> > is launched via the libc's execvp() routine. The way that execvp() routine > works is by invoking execve syscall for every entry on the $PATH if > command specified is neither absolute nor relative which can come across > as a bit cryptic to untrained eyes. > > - absolute path example: > > # trace-cmd record -p function_graph \ > -g __x64_sys_execve -O nofuncgraph-irqs \ > -n __cond_resched --max-graph-depth 1 \ > -F /usr/bin/echo "ftrace" > /dev/null > > # trace-cmd report > echo-172994 [000] 185539.798539: funcgraph_entry: ! 803.376 us | __x64_sys_execve(); > > - PATH-dependent path example: > > # trace-cmd record -p function_graph \ > -g __x64_sys_execve -O nofuncgraph-irqs \ > -n __cond_resched --max-graph-depth 1 \ > -F echo "ftrace" > /dev/null > > # trace-cmd report > echo-172656 [002] 185009.671586: funcgraph_entry: ! 288.732 us | __x64_sys_execve(); > echo-172656 [002] 185009.671879: funcgraph_entry: ! 158.337 us | __x64_sys_execve(); > echo-172656 [002] 185009.672042: funcgraph_entry: ! 161.843 us | __x64_sys_execve(); > echo-172656 [002] 185009.672207: funcgraph_entry: ! 157.656 us | __x64_sys_execve(); > echo-172656 [002] 185009.672369: funcgraph_entry: ! 156.343 us | __x64_sys_execve(); > echo-172656 [002] 185009.672529: funcgraph_entry: ! 863.629 us | __x64_sys_execve(); > > Open code the libc's execvp routine into trace-cmd so ftrace will only > start recording once the command is found when it needs to be found in > PATH. > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > Changelog: > > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt) > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/ > --- Ignore this patch. I just realised that I didn't tweak the CUnit tests. I will submit another patch shortly. - Paulo A.
On Sat, Jan 14, 2023 at 12:05:44PM +1300, Paulo Miguel Almeida wrote: > On Sat, Jan 14, 2023 at 11:58:41AM +1300, Paulo Miguel Almeida wrote: > > In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable> > > is launched via the libc's execvp() routine. The way that execvp() routine > > works is by invoking execve syscall for every entry on the $PATH if > > command specified is neither absolute nor relative which can come across > > as a bit cryptic to untrained eyes. > > > > - absolute path example: > > > > # trace-cmd record -p function_graph \ > > -g __x64_sys_execve -O nofuncgraph-irqs \ > > -n __cond_resched --max-graph-depth 1 \ > > -F /usr/bin/echo "ftrace" > /dev/null > > > > # trace-cmd report > > echo-172994 [000] 185539.798539: funcgraph_entry: ! 803.376 us | __x64_sys_execve(); > > > > - PATH-dependent path example: > > > > # trace-cmd record -p function_graph \ > > -g __x64_sys_execve -O nofuncgraph-irqs \ > > -n __cond_resched --max-graph-depth 1 \ > > -F echo "ftrace" > /dev/null > > > > # trace-cmd report > > echo-172656 [002] 185009.671586: funcgraph_entry: ! 288.732 us | __x64_sys_execve(); > > echo-172656 [002] 185009.671879: funcgraph_entry: ! 158.337 us | __x64_sys_execve(); > > echo-172656 [002] 185009.672042: funcgraph_entry: ! 161.843 us | __x64_sys_execve(); > > echo-172656 [002] 185009.672207: funcgraph_entry: ! 157.656 us | __x64_sys_execve(); > > echo-172656 [002] 185009.672369: funcgraph_entry: ! 156.343 us | __x64_sys_execve(); > > echo-172656 [002] 185009.672529: funcgraph_entry: ! 863.629 us | __x64_sys_execve(); > > > > Open code the libc's execvp routine into trace-cmd so ftrace will only > > start recording once the command is found when it needs to be found in > > PATH. > > > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > > --- > > Changelog: > > > > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt) > > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/ > > --- > > Ignore this patch. I just realised that I didn't tweak the CUnit tests. > I will submit another patch shortly. > > - Paulo A. False alarm Steve. You can review this patch as it is :-) The error I was getting before: --- Suite: trace-cmd Test: Simple record and report ...passed Test: Test convert from v7 to v6 ...passed Test: Use libraries to read file ...FAILED 1. tracecmd-utest.c:441 - data.counter > 0 Test: Test max length ...passed Run Summary: Type Total Ran Passed Failed Inactive suites 1 1 n/a 0 0 tests 4 4 3 1 0 asserts 24 24 23 1 n/a Elapsed time = 0.193 seconds --- After git bisect'ing the error I realised that my commit has nothing to do with the CUnit test failure given that this particular test case has been failing for me since its introduction to the code base on commit <d83b6628927326d158>. Regardless of what fix it requires, I'm assuming that would be done in a different patch anyway. So as soon as this one is reviewed and merged, I'm happy to further investigate the other :-) thanks! - Paulo A.
On Sat, 14 Jan 2023 11:58:41 +1300 Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: Hi Paulo, A couple of nits about submitting a follow up patch. 1) A second patch should always start a new thread. It's easier to find in inboxes. If you want, you could add a link to the first thread in the "changes" section (see below). 2) Please start the subject with a capital letter: [PATCH v2] trace-cmd: Open code execvp routine to avoid multiple execve syscalls > In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable> > is launched via the libc's execvp() routine. The way that execvp() routine > works is by invoking execve syscall for every entry on the $PATH if > command specified is neither absolute nor relative which can come across > as a bit cryptic to untrained eyes. > > - absolute path example: > > # trace-cmd record -p function_graph \ > -g __x64_sys_execve -O nofuncgraph-irqs \ > -n __cond_resched --max-graph-depth 1 \ > -F /usr/bin/echo "ftrace" > /dev/null > > # trace-cmd report > echo-172994 [000] 185539.798539: funcgraph_entry: ! 803.376 us | __x64_sys_execve(); > > - PATH-dependent path example: > > # trace-cmd record -p function_graph \ > -g __x64_sys_execve -O nofuncgraph-irqs \ > -n __cond_resched --max-graph-depth 1 \ > -F echo "ftrace" > /dev/null > > # trace-cmd report > echo-172656 [002] 185009.671586: funcgraph_entry: ! 288.732 us | __x64_sys_execve(); > echo-172656 [002] 185009.671879: funcgraph_entry: ! 158.337 us | __x64_sys_execve(); > echo-172656 [002] 185009.672042: funcgraph_entry: ! 161.843 us | __x64_sys_execve(); > echo-172656 [002] 185009.672207: funcgraph_entry: ! 157.656 us | __x64_sys_execve(); > echo-172656 [002] 185009.672369: funcgraph_entry: ! 156.343 us | __x64_sys_execve(); > echo-172656 [002] 185009.672529: funcgraph_entry: ! 863.629 us | __x64_sys_execve(); > > Open code the libc's execvp routine into trace-cmd so ftrace will only > start recording once the command is found when it needs to be found in > PATH. > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > Changelog: > > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt) > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/ > --- > tracecmd/trace-record.c | 59 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 53 insertions(+), 6 deletions(-) > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index 7f0cebe..4a54637 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -1683,6 +1683,58 @@ static int change_user(const char *user) > return 0; > } > > +static void execute_program(int argc, char **argv) > +{ > + char buf[PATH_MAX + NAME_MAX + 1]; > + char *path_env; > + size_t path_len; > + size_t entry_len; > + char *ptr_start; > + char *ptr_end; > + > + /* > + * if command specified by user is neither absolute nor > + * relative than we search for it in $PATH. > + */ > + if (!strchr(argv[0], '/') && !strchr(argv[0], '.')) { Why the search of '.'? If you have an executable called: my.exec Wouldn't that be found? Can you have a relative path without '/'? Usually, you would do: ./exec > + path_env = getenv("PATH"); Need to check for NULL, in the rare case that no "PATH" is defined. > + path_len = strlen(path_env); > + ptr_start = path_env; > + > + while ((ptr_start - path_env) < path_len) { > + ptr_end = strchr(ptr_start, ':'); Why not just use strtok_r() here? Something like (untested): char *saveptr; for (path = strtok_r(path_env, ":", &saveptr); path; path = strtok_r(NULL, ":", &saveptr) { snprintf(buf, PATH_MAX, "%s/%s", path, argv[0]); if (access(buf, X_OK) == 0) break; } > + > + /* single entry in PATH? */ > + if (!ptr_end) > + entry_len = path_len; > + else > + entry_len = ptr_end - ptr_start; > + > + strncpy(buf, ptr_start, entry_len); > + > + if (buf[entry_len - 1] != '/') > + buf[entry_len++] = '/'; > + > + strncpy(buf + entry_len, argv[0], sizeof(buf) - entry_len); > + > + /* does it exist and can we execute it? */ > + if (access(buf, X_OK) == 0) > + break; > + > + ptr_start = ptr_end + 1; > + } > + } else { > + strncpy(buf, argv[0], sizeof(buf)); > + } Don't we want to enable tracing here? -- Steve > + > + if (execve(buf, argv, environ)) { > + fprintf(stderr, "\n********************\n"); > + fprintf(stderr, " Unable to exec %s\n", argv[0]); > + fprintf(stderr, "********************\n"); > + die("Failed to exec %s", argv[0]); > + } > +} > + > static void run_cmd(enum trace_type type, const char *user, int argc, char **argv) > { > int status; > @@ -1709,12 +1761,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg > if (change_user(user) < 0) > die("Failed to change user to %s", user); > > - if (execvp(argv[0], argv)) { > - fprintf(stderr, "\n********************\n"); > - fprintf(stderr, " Unable to exec %s\n", argv[0]); > - fprintf(stderr, "********************\n"); > - die("Failed to exec %s", argv[0]); > - } > + execute_program(argc, argv); > } > if (fork_process) > exit(0);
On Sat, 14 Jan 2023 00:51:05 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > Hi Paulo, > > A couple of nits about submitting a follow up patch. > > 1) A second patch should always start a new thread. It's easier to find > in inboxes. > > If you want, you could add a link to the first thread in the "changes" > section (see below). And I forgot to update that "see below" comment. [..] > > Open code the libc's execvp routine into trace-cmd so ftrace will only > > start recording once the command is found when it needs to be found in > > PATH. > > > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > > --- > > Changelog: > > > > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt) > > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/ Because you did what I was going to say and I did not update the previous statement ;-) -- Steve
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index 7f0cebe..4a54637 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -1683,6 +1683,58 @@ static int change_user(const char *user) return 0; } +static void execute_program(int argc, char **argv) +{ + char buf[PATH_MAX + NAME_MAX + 1]; + char *path_env; + size_t path_len; + size_t entry_len; + char *ptr_start; + char *ptr_end; + + /* + * if command specified by user is neither absolute nor + * relative than we search for it in $PATH. + */ + if (!strchr(argv[0], '/') && !strchr(argv[0], '.')) { + path_env = getenv("PATH"); + path_len = strlen(path_env); + ptr_start = path_env; + + while ((ptr_start - path_env) < path_len) { + ptr_end = strchr(ptr_start, ':'); + + /* single entry in PATH? */ + if (!ptr_end) + entry_len = path_len; + else + entry_len = ptr_end - ptr_start; + + strncpy(buf, ptr_start, entry_len); + + if (buf[entry_len - 1] != '/') + buf[entry_len++] = '/'; + + strncpy(buf + entry_len, argv[0], sizeof(buf) - entry_len); + + /* does it exist and can we execute it? */ + if (access(buf, X_OK) == 0) + break; + + ptr_start = ptr_end + 1; + } + } else { + strncpy(buf, argv[0], sizeof(buf)); + } + + if (execve(buf, argv, environ)) { + fprintf(stderr, "\n********************\n"); + fprintf(stderr, " Unable to exec %s\n", argv[0]); + fprintf(stderr, "********************\n"); + die("Failed to exec %s", argv[0]); + } +} + static void run_cmd(enum trace_type type, const char *user, int argc, char **argv) { int status; @@ -1709,12 +1761,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg if (change_user(user) < 0) die("Failed to change user to %s", user); - if (execvp(argv[0], argv)) { - fprintf(stderr, "\n********************\n"); - fprintf(stderr, " Unable to exec %s\n", argv[0]); - fprintf(stderr, "********************\n"); - die("Failed to exec %s", argv[0]); - } + execute_program(argc, argv); } if (fork_process) exit(0);
In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable> is launched via the libc's execvp() routine. The way that execvp() routine works is by invoking execve syscall for every entry on the $PATH if command specified is neither absolute nor relative which can come across as a bit cryptic to untrained eyes. - absolute path example: # trace-cmd record -p function_graph \ -g __x64_sys_execve -O nofuncgraph-irqs \ -n __cond_resched --max-graph-depth 1 \ -F /usr/bin/echo "ftrace" > /dev/null # trace-cmd report echo-172994 [000] 185539.798539: funcgraph_entry: ! 803.376 us | __x64_sys_execve(); - PATH-dependent path example: # trace-cmd record -p function_graph \ -g __x64_sys_execve -O nofuncgraph-irqs \ -n __cond_resched --max-graph-depth 1 \ -F echo "ftrace" > /dev/null # trace-cmd report echo-172656 [002] 185009.671586: funcgraph_entry: ! 288.732 us | __x64_sys_execve(); echo-172656 [002] 185009.671879: funcgraph_entry: ! 158.337 us | __x64_sys_execve(); echo-172656 [002] 185009.672042: funcgraph_entry: ! 161.843 us | __x64_sys_execve(); echo-172656 [002] 185009.672207: funcgraph_entry: ! 157.656 us | __x64_sys_execve(); echo-172656 [002] 185009.672369: funcgraph_entry: ! 156.343 us | __x64_sys_execve(); echo-172656 [002] 185009.672529: funcgraph_entry: ! 863.629 us | __x64_sys_execve(); Open code the libc's execvp routine into trace-cmd so ftrace will only start recording once the command is found when it needs to be found in PATH. Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> --- Changelog: - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt) - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/ --- tracecmd/trace-record.c | 59 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 6 deletions(-)