| Message ID | 20230808091625.12760-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Rework benchmark command handling | expand |
Hi Ilpo, On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: > Benchmark command is copied into an array in the stack. The array is > BENCHMARK_ARGS items long but the command line could try to provide a > longer command. > > Return error in case the benchmark command does not fit to its array. > > Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index d511daeb6851..eef9e02516ad 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -255,6 +255,9 @@ int main(int argc, char **argv) > return ksft_exit_skip("Not running as root. Skipping...\n"); > > if (has_ben) { > + if (argc - ben_ind >= BENCHMARK_ARGS - 1) > + ksft_exit_fail_msg("Too long benchmark command"); > + I think there are two potential issues here: too many arguments and too long arguments. Current code can handle 64 (63 with last required to be NULL) arguments each expected to be 64 bytes (63 to end with \0). The above fix looks to be handling the first issue, with this the error message could be more accurate if it refers to the number of arguments instead. It looks to me as though the latter issue still needs to be handled. I understand that this becomes unnecessary via the refactor in following patches but I expect that this fix needs to be backported (cc. stable also) and thus it may benefit from a precision added to the sprintf() that follows the snippet below? > /* Extract benchmark command from command line. */ > for (i = ben_ind; i < argc; i++) { > benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; Reinette ps. Unless you have an updated email address that works, could you please remove Sai's email from future submissions?
On Mon, 14 Aug 2023, Reinette Chatre wrote: > On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: > > Benchmark command is copied into an array in the stack. The array is > > BENCHMARK_ARGS items long but the command line could try to provide a > > longer command. > > > > Return error in case the benchmark command does not fit to its array. > > > > Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > > index d511daeb6851..eef9e02516ad 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -255,6 +255,9 @@ int main(int argc, char **argv) > > return ksft_exit_skip("Not running as root. Skipping...\n"); > > > > if (has_ben) { > > + if (argc - ben_ind >= BENCHMARK_ARGS - 1) > > + ksft_exit_fail_msg("Too long benchmark command"); > > + > > I think there are two potential issues here: too many arguments and too > long arguments. Current code can handle 64 (63 with last required to be > NULL) arguments each expected to be 64 bytes (63 to end with \0). > The above fix looks to be handling the first issue, with this the > error message could be more accurate if it refers to the > number of arguments instead. It looks to me as though the latter > issue still needs to be handled. I understand that this becomes > unnecessary via the refactor in following patches but I expect that > this fix needs to be backported (cc. stable also) and thus > it may benefit from a precision added to the sprintf() that follows > the snippet below? Thanks for taking a look, yes, both are problems. Third problem which still remains is if "fill_buf" is the first argument of -b, the arguments are not validated to match the formatting used internally (I guess it might be intentional to allow overriding the internal fill_buf arguments but just the validation is lacking). I'll add strlen() check instead of using sprintf() in order to properly fail rather than silently truncating the arguments. > > /* Extract benchmark command from command line. */ > > for (i = ben_ind; i < argc; i++) { > > benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; > > Reinette > > ps. Unless you have an updated email address that works, could you please > remove Sai's email from future submissions? It's auto-added by git send-email machinery. I guess I can try to make an exception to my usual workflow by sending only to manually specified To addresses (if I remember). Perhaps one day I'll write a tool to filter out the addresses from git send-email generated ones but as is I don't have one.
Hi Ilpo, On 8/15/2023 2:10 AM, Ilpo Järvinen wrote: >> ps. Unless you have an updated email address that works, could you please >> remove Sai's email from future submissions? > > It's auto-added by git send-email machinery. I guess I can try to make > an exception to my usual workflow by sending only to manually specified To > addresses (if I remember). Perhaps one day I'll write a tool to filter out > the addresses from git send-email generated ones but as is I don't have > one. > Which git send-email machinery are you referring to? If I understand correctly it does not automatically pick addresses but you can provide custom commands to it that can do it. Reinette
On Tue, 15 Aug 2023, Reinette Chatre wrote: > On 8/15/2023 2:10 AM, Ilpo Järvinen wrote: > >> ps. Unless you have an updated email address that works, could you please > >> remove Sai's email from future submissions? > > > > It's auto-added by git send-email machinery. I guess I can try to make > > an exception to my usual workflow by sending only to manually specified To > > addresses (if I remember). Perhaps one day I'll write a tool to filter out > > the addresses from git send-email generated ones but as is I don't have > > one. > > > > Which git send-email machinery are you referring to? If I understand correctly > it does not automatically pick addresses but you can provide custom commands to > it that can do it. Ah sorry, it is actually scripts/get_maintainer.pl automation I use with git send-email to figure out where to send the patches besides the --to & --cc entries I provided. For this patch, get_maintainer.pl returns this list: Fenghua Yu <fenghua.yu@intel.com> (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%) Reinette Chatre <reinette.chatre@intel.com> (supporter:RDT - RESOURCE ALLOCATION) Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%) Babu Moger <babu.moger@amd.com> (blamed_fixes:1/1=100%) Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> (blamed_fixes:1/1=100%) linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION) linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) ...which includes Sai's address (not much I can do about that, it's immutably crafted into git history that those lines were once touched by Sai). I've thought of writing yet another wrapper to filter out known failing addresses but until that's done, either I need to (remember to) manually send the series w/o get_maintainer.pl automation or accept a few failures here and there.
Hi Ilpo, On 8/15/2023 11:32 PM, Ilpo Järvinen wrote: > Ah sorry, it is actually scripts/get_maintainer.pl automation I use with > git send-email to figure out where to send the patches besides the --to & > --cc entries I provided. For this patch, get_maintainer.pl returns this > list: > > Fenghua Yu <fenghua.yu@intel.com> (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%) > Reinette Chatre <reinette.chatre@intel.com> (supporter:RDT - RESOURCE ALLOCATION) > Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%) > Babu Moger <babu.moger@amd.com> (blamed_fixes:1/1=100%) > Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> (blamed_fixes:1/1=100%) > linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION) > linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK) > > ...which includes Sai's address (not much I can do about that, it's > immutably crafted into git history that those lines were once touched by > Sai). I've thought of writing yet another wrapper to filter out known > failing addresses but until that's done, either I need to (remember to) > manually send the series w/o get_maintainer.pl automation or accept a few > failures here and there. > You can append Sai's address to .get_maintainer.ignore. Reinette
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..eef9e02516ad 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,6 +255,9 @@ int main(int argc, char **argv) return ksft_exit_skip("Not running as root. Skipping...\n"); if (has_ben) { + if (argc - ben_ind >= BENCHMARK_ARGS - 1) + ksft_exit_fail_msg("Too long benchmark command"); + /* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
Benchmark command is copied into an array in the stack. The array is BENCHMARK_ARGS items long but the command line could try to provide a longer command. Return error in case the benchmark command does not fit to its array. Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++ 1 file changed, 3 insertions(+)