Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220616211016.4037482-1-dylanbhatch@google.com>
 <941e0991-eb3e-f988-8262-3d51ff8badad@linuxfoundation.org>
 <CADBMgpwt2ALzBTtEm7v6DLL_9pjUhVLDpBLHXn1b0bvVf2BSvg@mail.gmail.com>
 <47312e8a-87fe-c7dc-d354-74e81482bc1e@linuxfoundation.org>
 <CADBMgpx9hwHaWe=m2kQhKOJFWnLSejoWa6wz1VECEkLhWq4qog@mail.gmail.com> <a5f46e4e-a472-77ce-f61e-b2f9922bdd50@linuxfoundation.org>
In-Reply-To: <a5f46e4e-a472-77ce-f61e-b2f9922bdd50@linuxfoundation.org>
From:   Dylan Hatch <dylanbhatch@google.com>
Date:   Tue, 21 Jun 2022 17:18:14 -0700
Message-ID: <CADBMgpzyOKVO1ju_WkxYLhXGvwJjHoL6V-+Nw49UdTFoPY7NvQ@mail.gmail.com>
Subject: Re: [PATCH] selftests/proc: Fix proc-pid-vm for vsyscall=xonly.
To:     Shuah Khan <skhan@linuxfoundation.org>
Cc:     Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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