| Message ID | 20220616211016.4037482-1-dylanbhatch@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/proc: Fix proc-pid-vm for vsyscall=xonly. | expand |
On 6/16/22 3:10 PM, Dylan Hatch wrote: > This test would erroneously fail the /proc/$PID/maps case if > vsyscall=xonly since the existing probe of the vsyscall page only > succeeds if the process has read permissions. Fix this by checking for > either no vsyscall mapping OR an execute-only vsyscall mapping in the > case were probing the vsyscall page segfaults. > Does this fix include skipping the test with a clear message that says why test is skipped? > Signed-off-by: Dylan Hatch <dylanbhatch@google.com> > --- > tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c > index 28604c9f805c..5ca85520131f 100644 > --- a/tools/testing/selftests/proc/proc-pid-vm.c > +++ b/tools/testing/selftests/proc/proc-pid-vm.c > @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) > > static bool g_vsyscall = false; > > -static const char str_vsyscall[] = > +static const char str_vsyscall_rx[] = > "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; > > +static const char str_vsyscall_x[] = > +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; > + > #ifdef __x86_64__ > static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) > { > @@ -261,6 +264,7 @@ int main(void) > int exec_fd; > > vsyscall(); > + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; > > atexit(ate); > > @@ -314,7 +318,8 @@ int main(void) > > /* Test /proc/$PID/maps */ > { > - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); > + const size_t len_buf0 = strlen(buf0); > + const size_t len_vsys = strlen(str_vsyscall); > char buf[256]; > ssize_t rv; > int fd; > @@ -325,11 +330,16 @@ int main(void) > return 1; > } > rv = read(fd, buf, sizeof(buf)); > - assert(rv == len); > - assert(memcmp(buf, buf0, strlen(buf0)) == 0); > if (g_vsyscall) { > - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); > + assert(rv == len_buf0 + len_vsys); > + } else { > + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ > + assert(rv == len_buf0 + len_vsys || rv == len_buf0); > } > + assert(memcmp(buf, buf0, len_buf0) == 0); > + /* Check for vsyscall mapping if buf is long enough */ > + if (rv == len_buf0 + len_vsys) > + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); > } > > /* Test /proc/$PID/smaps */ > The change looks good to me. Doesn't look like it skips the test though? thanks, -- Shuah
On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/16/22 3:10 PM, Dylan Hatch wrote: > > This test would erroneously fail the /proc/$PID/maps case if > > vsyscall=xonly since the existing probe of the vsyscall page only > > succeeds if the process has read permissions. Fix this by checking for > > either no vsyscall mapping OR an execute-only vsyscall mapping in the > > case were probing the vsyscall page segfaults. > > > > Does this fix include skipping the test with a clear message that > says why test is skipped? > > > Signed-off-by: Dylan Hatch <dylanbhatch@google.com> > > --- > > tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c > > index 28604c9f805c..5ca85520131f 100644 > > --- a/tools/testing/selftests/proc/proc-pid-vm.c > > +++ b/tools/testing/selftests/proc/proc-pid-vm.c > > @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) > > > > static bool g_vsyscall = false; > > > > -static const char str_vsyscall[] = > > +static const char str_vsyscall_rx[] = > > "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; > > > > +static const char str_vsyscall_x[] = > > +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; > > + > > #ifdef __x86_64__ > > static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) > > { > > @@ -261,6 +264,7 @@ int main(void) > > int exec_fd; > > > > vsyscall(); > > + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; > > > > atexit(ate); > > > > @@ -314,7 +318,8 @@ int main(void) > > > > /* Test /proc/$PID/maps */ > > { > > - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); > > + const size_t len_buf0 = strlen(buf0); > > + const size_t len_vsys = strlen(str_vsyscall); > > char buf[256]; > > ssize_t rv; > > int fd; > > @@ -325,11 +330,16 @@ int main(void) > > return 1; > > } > > rv = read(fd, buf, sizeof(buf)); > > - assert(rv == len); > > - assert(memcmp(buf, buf0, strlen(buf0)) == 0); > > if (g_vsyscall) { > > - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); > > + assert(rv == len_buf0 + len_vsys); > > + } else { > > + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ > > + assert(rv == len_buf0 + len_vsys || rv == len_buf0); > > } > > + assert(memcmp(buf, buf0, len_buf0) == 0); > > + /* Check for vsyscall mapping if buf is long enough */ > > + if (rv == len_buf0 + len_vsys) > > + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); > > } > > > > /* Test /proc/$PID/smaps */ > > > > The change looks good to me. Doesn't look like it skips the test though? Instead of skipping the test, it changes the passing condition to accept both cases of an unmapped vsyscall page and an x-only vsyscall page. Differentiating between these two cases without relying on /proc/$PID/maps would involve both checking the kernel command line for vsyscall=xonly and having a special ifdef block for CONFIG_VSYSCALL_XONLY, so accepting both as passing conditions seems like a simpler solution. Thanks, Dylan
On 6/17/22 12:45 PM, Dylan Hatch wrote: > On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 6/16/22 3:10 PM, Dylan Hatch wrote: >>> This test would erroneously fail the /proc/$PID/maps case if >>> vsyscall=xonly since the existing probe of the vsyscall page only >>> succeeds if the process has read permissions. Fix this by checking for >>> either no vsyscall mapping OR an execute-only vsyscall mapping in the >>> case were probing the vsyscall page segfaults. >>> >> >> Does this fix include skipping the test with a clear message that >> says why test is skipped? >> >>> Signed-off-by: Dylan Hatch <dylanbhatch@google.com> >>> --- >>> tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- >>> 1 file changed, 15 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c >>> index 28604c9f805c..5ca85520131f 100644 >>> --- a/tools/testing/selftests/proc/proc-pid-vm.c >>> +++ b/tools/testing/selftests/proc/proc-pid-vm.c >>> @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) >>> >>> static bool g_vsyscall = false; >>> >>> -static const char str_vsyscall[] = >>> +static const char str_vsyscall_rx[] = >>> "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; >>> >>> +static const char str_vsyscall_x[] = >>> +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; >>> + >>> #ifdef __x86_64__ >>> static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) >>> { >>> @@ -261,6 +264,7 @@ int main(void) >>> int exec_fd; >>> >>> vsyscall(); >>> + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; >>> >>> atexit(ate); >>> >>> @@ -314,7 +318,8 @@ int main(void) >>> >>> /* Test /proc/$PID/maps */ >>> { >>> - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); >>> + const size_t len_buf0 = strlen(buf0); >>> + const size_t len_vsys = strlen(str_vsyscall); >>> char buf[256]; >>> ssize_t rv; >>> int fd; >>> @@ -325,11 +330,16 @@ int main(void) >>> return 1; >>> } >>> rv = read(fd, buf, sizeof(buf)); >>> - assert(rv == len); >>> - assert(memcmp(buf, buf0, strlen(buf0)) == 0); >>> if (g_vsyscall) { >>> - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); >>> + assert(rv == len_buf0 + len_vsys); >>> + } else { >>> + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ >>> + assert(rv == len_buf0 + len_vsys || rv == len_buf0); >>> } >>> + assert(memcmp(buf, buf0, len_buf0) == 0); >>> + /* Check for vsyscall mapping if buf is long enough */ >>> + if (rv == len_buf0 + len_vsys) >>> + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); >>> } >>> >>> /* Test /proc/$PID/smaps */ >>> >> >> The change looks good to me. Doesn't look like it skips the test though? > > Instead of skipping the test, it changes the passing condition to > accept both cases of an unmapped vsyscall page and an x-only vsyscall > page. Differentiating between these two cases without relying on > /proc/$PID/maps would involve both checking the kernel command line > for vsyscall=xonly and having a special ifdef block for > CONFIG_VSYSCALL_XONLY, so accepting both as passing conditions seems > like a simpler solution. > It depends on the goal of the test. Is the test looking to see if the probe fails with insufficient permissions, then you are changing the test to not check for that condition. I would say in this case, the right approach would be to leave the test as is and report expected fail and add other cases. The goal being adding more coverage and not necessarily opt for a simple solution. thanks, -- Shuah
On Fri, Jun 17, 2022 at 12:38 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/17/22 12:45 PM, Dylan Hatch wrote: > > On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > >> > >> On 6/16/22 3:10 PM, Dylan Hatch wrote: > >>> This test would erroneously fail the /proc/$PID/maps case if > >>> vsyscall=xonly since the existing probe of the vsyscall page only > >>> succeeds if the process has read permissions. Fix this by checking for > >>> either no vsyscall mapping OR an execute-only vsyscall mapping in the > >>> case were probing the vsyscall page segfaults. > >>> > >> > >> Does this fix include skipping the test with a clear message that > >> says why test is skipped? > >> > >>> Signed-off-by: Dylan Hatch <dylanbhatch@google.com> > >>> --- > >>> tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- > >>> 1 file changed, 15 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c > >>> index 28604c9f805c..5ca85520131f 100644 > >>> --- a/tools/testing/selftests/proc/proc-pid-vm.c > >>> +++ b/tools/testing/selftests/proc/proc-pid-vm.c > >>> @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) > >>> > >>> static bool g_vsyscall = false; > >>> > >>> -static const char str_vsyscall[] = > >>> +static const char str_vsyscall_rx[] = > >>> "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; > >>> > >>> +static const char str_vsyscall_x[] = > >>> +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; > >>> + > >>> #ifdef __x86_64__ > >>> static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) > >>> { > >>> @@ -261,6 +264,7 @@ int main(void) > >>> int exec_fd; > >>> > >>> vsyscall(); > >>> + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; > >>> > >>> atexit(ate); > >>> > >>> @@ -314,7 +318,8 @@ int main(void) > >>> > >>> /* Test /proc/$PID/maps */ > >>> { > >>> - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); > >>> + const size_t len_buf0 = strlen(buf0); > >>> + const size_t len_vsys = strlen(str_vsyscall); > >>> char buf[256]; > >>> ssize_t rv; > >>> int fd; > >>> @@ -325,11 +330,16 @@ int main(void) > >>> return 1; > >>> } > >>> rv = read(fd, buf, sizeof(buf)); > >>> - assert(rv == len); > >>> - assert(memcmp(buf, buf0, strlen(buf0)) == 0); > >>> if (g_vsyscall) { > >>> - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); > >>> + assert(rv == len_buf0 + len_vsys); > >>> + } else { > >>> + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ > >>> + assert(rv == len_buf0 + len_vsys || rv == len_buf0); > >>> } > >>> + assert(memcmp(buf, buf0, len_buf0) == 0); > >>> + /* Check for vsyscall mapping if buf is long enough */ > >>> + if (rv == len_buf0 + len_vsys) > >>> + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); > >>> } > >>> > >>> /* Test /proc/$PID/smaps */ > >>> > >> > >> The change looks good to me. Doesn't look like it skips the test though? > > > > Instead of skipping the test, it changes the passing condition to > > accept both cases of an unmapped vsyscall page and an x-only vsyscall > > page. Differentiating between these two cases without relying on > > /proc/$PID/maps would involve both checking the kernel command line > > for vsyscall=xonly and having a special ifdef block for > > CONFIG_VSYSCALL_XONLY, so accepting both as passing conditions seems > > like a simpler solution. > > > > It depends on the goal of the test. Is the test looking to see if the > probe fails with insufficient permissions, then you are changing the > test to not check for that condition. The goal of the test is to validate the output of /proc/$PID/maps, and the memory probe is only needed as setup to determine what the expected output should be. This used to be sufficient, but now it can no longer fully disambiguate it with the introduction of vsyscall=xonly. The solution proposed here is to disambiguate it by also checking the length read from /proc/$PID/maps. > > I would say in this case, the right approach would be to leave the test > as is and report expected fail and add other cases. > > The goal being adding more coverage and not necessarily opt for a simple > solution. What does it mean to report a test as expected fail? Is this a mechanism unique to kselftest? I agree adding another test case would work, but I'm unsure how to do it within the framework of kselftest. Ideally, there would be separate test cases for vsyscall=none, vsyscall=emulate, and vsyscall=xonly, but these options can be toggled both in the kernel config and on the kernel command line, meaning (to the best of my knowledge) these test cases would have to be built conditionally against the conflig options and also parse the command line for the 'vsyscall' option. Thanks, Dylan
On 6/17/22 4:05 PM, Dylan Hatch wrote: > On Fri, Jun 17, 2022 at 12:38 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 6/17/22 12:45 PM, Dylan Hatch wrote: >>> On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >>>> > >> >> It depends on the goal of the test. Is the test looking to see if the >> probe fails with insufficient permissions, then you are changing the >> test to not check for that condition. > > The goal of the test is to validate the output of /proc/$PID/maps, and > the memory probe is only needed as setup to determine what the > expected output should be. This used to be sufficient, but now it can > no longer fully disambiguate it with the introduction of > vsyscall=xonly. The solution proposed here is to disambiguate it by > also checking the length read from /proc/$PID/maps. > >> Makes sense. However the question is does this test need to be enhanced with the addition of vsyscall=xonly? >> I would say in this case, the right approach would be to leave the test >> as is and report expected fail and add other cases. >> >> The goal being adding more coverage and not necessarily opt for a simple >> solution. > > What does it mean to report a test as expected fail? Is this a > mechanism unique to kselftest? I agree adding another test case would > work, but I'm unsure how to do it within the framework of kselftest. > Ideally, there would be separate test cases for vsyscall=none, > vsyscall=emulate, and vsyscall=xonly, but these options can be toggled > both in the kernel config and on the kernel command line, meaning (to > the best of my knowledge) these test cases would have to be built > conditionally against the conflig options and also parse the command > line for the 'vsyscall' option. > Expected fail isn't unique kselftest. It is a testing criteria where a test is expected to fail. For example if a file can only be opened with privileged user a test that runs and looks for failure is an expected to fail case - we are looking for a failure. A complete battery of tests for vsyscall=none, vsyscall=emulate, vsyscall=xonly would test for conditions that are expected to pass and fail based on the config. tools/testing/selftests/proc/config doesn't have any config options that are relevant to VSYSCALL Can you please send me the how you are running the test and what the failure output looks like? thanks, -- Shuah
On Fri, Jun 17, 2022 at 3:27 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/17/22 4:05 PM, Dylan Hatch wrote: > > On Fri, Jun 17, 2022 at 12:38 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > >> > >> On 6/17/22 12:45 PM, Dylan Hatch wrote: > >>> On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > >>>> > > > >> > >> It depends on the goal of the test. Is the test looking to see if the > >> probe fails with insufficient permissions, then you are changing the > >> test to not check for that condition. > > > > The goal of the test is to validate the output of /proc/$PID/maps, and > > the memory probe is only needed as setup to determine what the > > expected output should be. This used to be sufficient, but now it can > > no longer fully disambiguate it with the introduction of > > vsyscall=xonly. The solution proposed here is to disambiguate it by > > also checking the length read from /proc/$PID/maps. > > > >> > > Makes sense. However the question is does this test need to be enhanced > with the addition of vsyscall=xonly? > > >> I would say in this case, the right approach would be to leave the test > >> as is and report expected fail and add other cases. > >> > >> The goal being adding more coverage and not necessarily opt for a simple > >> solution. > > > > What does it mean to report a test as expected fail? Is this a > > mechanism unique to kselftest? I agree adding another test case would > > work, but I'm unsure how to do it within the framework of kselftest. > > Ideally, there would be separate test cases for vsyscall=none, > > vsyscall=emulate, and vsyscall=xonly, but these options can be toggled > > both in the kernel config and on the kernel command line, meaning (to > > the best of my knowledge) these test cases would have to be built > > conditionally against the conflig options and also parse the command > > line for the 'vsyscall' option. > > > > Expected fail isn't unique kselftest. It is a testing criteria where > a test is expected to fail. For example if a file can only be opened > with privileged user a test that runs and looks for failure is an > expected to fail case - we are looking for a failure. > > A complete battery of tests for vsyscall=none, vsyscall=emulate, > vsyscall=xonly would test for conditions that are expected to pass > and fail based on the config. > > tools/testing/selftests/proc/config doesn't have any config options > that are relevant to VSYSCALL > > Can you please send me the how you are running the test and what the > failure output looks like? I'm building a kernel with the following relevant configurations: $ cat .config | grep VSYSCALL CONFIG_GENERIC_TIME_VSYSCALL=y CONFIG_X86_VSYSCALL_EMULATION=y CONFIG_LEGACY_VSYSCALL_XONLY=y # CONFIG_LEGACY_VSYSCALL_NONE is not set Running the test without this change both in virtme and on real hardware gives the following error: # ./tools/testing/selftests/proc/proc-pid-vm proc-pid-vm: proc-pid-vm.c:328: int main(void): Assertion `rv == len' failed. Aborted This is because when CONFIG_LEGACY_VSYSCALL_XONLY=y a probe of the vsyscall page results in a segfault. This test was originally written before this option existed so it incorrectly assumes the vsyscall page isn't mapped at all, and the expected buffer length doesn't match the result. An alternate method of fixing this test could involve setting the expected result based on the config with #ifdef blocks, but I wasn't sure if that could be done for kernel config options in kselftest code. There's also the matter of checking the kernel command line for a `vsyscall=` arg, is parsing /proc/cmdline the best way to do this? > > thanks, > -- Shuah Thanks, Dylan
On 6/21/22 6:18 PM, Dylan Hatch wrote: > On Fri, Jun 17, 2022 at 3:27 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 6/17/22 4:05 PM, Dylan Hatch wrote: >>> On Fri, Jun 17, 2022 at 12:38 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >>>> >>>> On 6/17/22 12:45 PM, Dylan Hatch wrote: >>>>> On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >>>>>> >>> >>>> >>>> It depends on the goal of the test. Is the test looking to see if the >>>> probe fails with insufficient permissions, then you are changing the >>>> test to not check for that condition. >>> >>> The goal of the test is to validate the output of /proc/$PID/maps, and >>> the memory probe is only needed as setup to determine what the >>> expected output should be. This used to be sufficient, but now it can >>> no longer fully disambiguate it with the introduction of >>> vsyscall=xonly. The solution proposed here is to disambiguate it by >>> also checking the length read from /proc/$PID/maps. >>> >>>> >> >> Makes sense. However the question is does this test need to be enhanced >> with the addition of vsyscall=xonly? >> >>>> I would say in this case, the right approach would be to leave the test >>>> as is and report expected fail and add other cases. >>>> >>>> The goal being adding more coverage and not necessarily opt for a simple >>>> solution. >>> >>> What does it mean to report a test as expected fail? Is this a >>> mechanism unique to kselftest? I agree adding another test case would >>> work, but I'm unsure how to do it within the framework of kselftest. >>> Ideally, there would be separate test cases for vsyscall=none, >>> vsyscall=emulate, and vsyscall=xonly, but these options can be toggled >>> both in the kernel config and on the kernel command line, meaning (to >>> the best of my knowledge) these test cases would have to be built >>> conditionally against the conflig options and also parse the command >>> line for the 'vsyscall' option. >>> >> >> Expected fail isn't unique kselftest. It is a testing criteria where >> a test is expected to fail. For example if a file can only be opened >> with privileged user a test that runs and looks for failure is an >> expected to fail case - we are looking for a failure. >> >> A complete battery of tests for vsyscall=none, vsyscall=emulate, >> vsyscall=xonly would test for conditions that are expected to pass >> and fail based on the config. >> >> tools/testing/selftests/proc/config doesn't have any config options >> that are relevant to VSYSCALL >> >> Can you please send me the how you are running the test and what the >> failure output looks like? > > I'm building a kernel with the following relevant configurations: > > $ cat .config | grep VSYSCALL > CONFIG_GENERIC_TIME_VSYSCALL=y > CONFIG_X86_VSYSCALL_EMULATION=y > CONFIG_LEGACY_VSYSCALL_XONLY=y > # CONFIG_LEGACY_VSYSCALL_NONE is not set > > Running the test without this change both in virtme and on real > hardware gives the following error: > > # ./tools/testing/selftests/proc/proc-pid-vm > proc-pid-vm: proc-pid-vm.c:328: int main(void): Assertion `rv == len' failed. > Aborted > > This is because when CONFIG_LEGACY_VSYSCALL_XONLY=y a probe of the > vsyscall page results in a segfault. This test was originally written > before this option existed so it incorrectly assumes the vsyscall page > isn't mapped at all, and the expected buffer length doesn't match the > result. > > An alternate method of fixing this test could involve setting the > expected result based on the config with #ifdef blocks, but I wasn't > sure if that could be done for kernel config options in kselftest > code. There's also the matter of checking the kernel command line for > a `vsyscall=` arg, is parsing /proc/cmdline the best way to do this? > We have a few tests do ifdef to be able to test the code as well as deal with config specific tests. Not an issue. Parsing /proc/cmdline line is flexible for sure, if you want to use that route. Thank you for finding the problem and identifying missing coverage. Look forward to any patches fixing the problem. thanks, -- Shuah
Accidentally hit direct reply, adding Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org, Shuah Khan <skhan@linuxfoundation.org> On Mon, Jul 11, 2022 at 4:04 PM Dylan Hatch <dylanbhatch@google.com> wrote: > > On Wed, Jun 22, 2022 at 10:15 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > > > On 6/21/22 6:18 PM, Dylan Hatch wrote: > > > On Fri, Jun 17, 2022 at 3:27 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > >> > > >> On 6/17/22 4:05 PM, Dylan Hatch wrote: > > >>> On Fri, Jun 17, 2022 at 12:38 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > >>>> > > >>>> On 6/17/22 12:45 PM, Dylan Hatch wrote: > > >>>>> On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > >>>>>> > > >>> > > >>>> > > >>>> It depends on the goal of the test. Is the test looking to see if the > > >>>> probe fails with insufficient permissions, then you are changing the > > >>>> test to not check for that condition. > > >>> > > >>> The goal of the test is to validate the output of /proc/$PID/maps, and > > >>> the memory probe is only needed as setup to determine what the > > >>> expected output should be. This used to be sufficient, but now it can > > >>> no longer fully disambiguate it with the introduction of > > >>> vsyscall=xonly. The solution proposed here is to disambiguate it by > > >>> also checking the length read from /proc/$PID/maps. > > >>> > > >>>> > > >> > > >> Makes sense. However the question is does this test need to be enhanced > > >> with the addition of vsyscall=xonly? > > >> > > >>>> I would say in this case, the right approach would be to leave the test > > >>>> as is and report expected fail and add other cases. > > >>>> > > >>>> The goal being adding more coverage and not necessarily opt for a simple > > >>>> solution. > > >>> > > >>> What does it mean to report a test as expected fail? Is this a > > >>> mechanism unique to kselftest? I agree adding another test case would > > >>> work, but I'm unsure how to do it within the framework of kselftest. > > >>> Ideally, there would be separate test cases for vsyscall=none, > > >>> vsyscall=emulate, and vsyscall=xonly, but these options can be toggled > > >>> both in the kernel config and on the kernel command line, meaning (to > > >>> the best of my knowledge) these test cases would have to be built > > >>> conditionally against the conflig options and also parse the command > > >>> line for the 'vsyscall' option. > > >>> > > >> > > >> Expected fail isn't unique kselftest. It is a testing criteria where > > >> a test is expected to fail. For example if a file can only be opened > > >> with privileged user a test that runs and looks for failure is an > > >> expected to fail case - we are looking for a failure. > > >> > > >> A complete battery of tests for vsyscall=none, vsyscall=emulate, > > >> vsyscall=xonly would test for conditions that are expected to pass > > >> and fail based on the config. > > >> > > >> tools/testing/selftests/proc/config doesn't have any config options > > >> that are relevant to VSYSCALL > > >> > > >> Can you please send me the how you are running the test and what the > > >> failure output looks like? > > > > > > I'm building a kernel with the following relevant configurations: > > > > > > $ cat .config | grep VSYSCALL > > > CONFIG_GENERIC_TIME_VSYSCALL=y > > > CONFIG_X86_VSYSCALL_EMULATION=y > > > CONFIG_LEGACY_VSYSCALL_XONLY=y > > > # CONFIG_LEGACY_VSYSCALL_NONE is not set > > > > > > Running the test without this change both in virtme and on real > > > hardware gives the following error: > > > > > > # ./tools/testing/selftests/proc/proc-pid-vm > > > proc-pid-vm: proc-pid-vm.c:328: int main(void): Assertion `rv == len' failed. > > > Aborted > > > > > > This is because when CONFIG_LEGACY_VSYSCALL_XONLY=y a probe of the > > > vsyscall page results in a segfault. This test was originally written > > > before this option existed so it incorrectly assumes the vsyscall page > > > isn't mapped at all, and the expected buffer length doesn't match the > > > result. > > > > > > An alternate method of fixing this test could involve setting the > > > expected result based on the config with #ifdef blocks, but I wasn't > > > sure if that could be done for kernel config options in kselftest > > > code. There's also the matter of checking the kernel command line for > > > a `vsyscall=` arg, is parsing /proc/cmdline the best way to do this? > > > > > > > We have a few tests do ifdef to be able to test the code as well as deal > > with config specific tests. Not an issue. > > > > Parsing /proc/cmdline line is flexible for sure, if you want to use that > > route. > > > > Thank you for finding the problem and identifying missing coverage. Look > > forward to any patches fixing the problem. > > > > thanks, > > -- Shuah > I've done some experimenting with ifdefs on config options, but it seems that these options do not propagate properly into the tests. Is there a specific method I should be using to propagate the config values, or would you be able to point me to an example where this is done properly? Thanks and sorry for the slow reply on this, Dylan
diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c index 28604c9f805c..5ca85520131f 100644 --- a/tools/testing/selftests/proc/proc-pid-vm.c +++ b/tools/testing/selftests/proc/proc-pid-vm.c @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) static bool g_vsyscall = false; -static const char str_vsyscall[] = +static const char str_vsyscall_rx[] = "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; +static const char str_vsyscall_x[] = +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; + #ifdef __x86_64__ static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) { @@ -261,6 +264,7 @@ int main(void) int exec_fd; vsyscall(); + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; atexit(ate); @@ -314,7 +318,8 @@ int main(void) /* Test /proc/$PID/maps */ { - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); + const size_t len_buf0 = strlen(buf0); + const size_t len_vsys = strlen(str_vsyscall); char buf[256]; ssize_t rv; int fd; @@ -325,11 +330,16 @@ int main(void) return 1; } rv = read(fd, buf, sizeof(buf)); - assert(rv == len); - assert(memcmp(buf, buf0, strlen(buf0)) == 0); if (g_vsyscall) { - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); + assert(rv == len_buf0 + len_vsys); + } else { + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ + assert(rv == len_buf0 + len_vsys || rv == len_buf0); } + assert(memcmp(buf, buf0, len_buf0) == 0); + /* Check for vsyscall mapping if buf is long enough */ + if (rv == len_buf0 + len_vsys) + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); } /* Test /proc/$PID/smaps */
This test would erroneously fail the /proc/$PID/maps case if vsyscall=xonly since the existing probe of the vsyscall page only succeeds if the process has read permissions. Fix this by checking for either no vsyscall mapping OR an execute-only vsyscall mapping in the case were probing the vsyscall page segfaults. Signed-off-by: Dylan Hatch <dylanbhatch@google.com> --- tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)