On Sat, Sep 17, 2022 at 01:50:24AM +0100, Josh Triplett wrote: > On Fri, Sep 16, 2022 at 05:11:18PM -0700, Kees Cook wrote: > > I don't like the idea of penalizing the _succeeding_ case, though, which > > happens if we do the path walk twice. So, I went and refactoring the setup > > order, moving the do_open_execat() up into alloc_bprm() instead of where > > it was in bprm_exec(). The result makes it so it is, as you observed, > > before the mm creation and generally expensive argument copying. The > > difference to your patch seems to only be the allocation of the file > > table entry, but avoids the double lookup, so I'm hoping the result is > > actually even faster. > > Thanks for giving this a try; I'd wondered how feasible it would be to > just do one lookup. > > However, on the same test system with the same test setup, with your > refactor it seems to go slower: > fork/execvpe: 38087ns > fork/execve: 33758ns > > For comparison, the previous numbers (which I re-confirmed): > > Without fast-path: > fork/execvpe: 49876ns > fork/execve: 32773ns > > With my original separate-lookup fast-path: > fork/execvpe: 36890ns > fork/execve: 31551ns Hmm, this shows as slower in the *normal* case, which I find rather surprising -- it's the same work, just reordered. Can you post a URL to your tests? I'd like to reproduce this and maybe throw perf at it as well. > I tried several runs of each, and I seem to get reasonably consistent > results. > > My test program just creates a pipe once, then loops on > clock_gettime/fork/execvpe/read, with the spawned child process doing > clock_gettime/write/exit (in asm to minimize overhead). The test PATH is > PATH=/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin:. with > the test program in the current directory. I'm also curious about less synthetic testing. It'd be nice to see real workloads with these changes, etc. -- Kees Cook
