Re: Patch "selftests: vDSO: skip getrandom test if architecture is unsupported" has been added to the 6.11-stable tree

[Shuah Khan <skhan@linuxfoundation.org>]

On 10/2/24 11:00, Jason A. Donenfeld wrote:
> On Wed, Oct 02, 2024 at 08:21:36AM +0200, Greg KH wrote:
>> On Wed, Oct 02, 2024 at 06:13:45AM +0200, Jason A. Donenfeld wrote:
>>> On Tue, Oct 01, 2024 at 09:29:45AM -0600, Shuah Khan wrote:
>>>> On 10/1/24 09:03, Jason A. Donenfeld wrote:
>>>>> On Tue, Oct 01, 2024 at 08:56:43AM -0600, Shuah Khan wrote:
>>>>>> On 10/1/24 08:45, Jason A. Donenfeld wrote:
>>>>>>> On Tue, Oct 01, 2024 at 08:43:05AM -0600, Shuah Khan wrote:
>>>>>>>> On 9/30/24 21:56, Jason A. Donenfeld wrote:
>>>>>>>>> This is not stable material and I didn't mark it as such. Do not backport.
>>>>>>>>
>>>>>>>> The way selftest work is they just skip if a feature isn't supported.
>>>>>>>> As such this test should run gracefully on stable releases.
>>>>>>>>
>>>>>>>> I would say backport unless and skip if the feature isn't supported.
>>>>>>>
>>>>>>> Nonsense. 6.11 never returns ENOSYS from vDSO. This doesn't make sense.
>>>>>>
>>>>>> Not sure what you mean by Nonsense. ENOSYS can be used to skip??
>>>>>
>>>>> The branch that this patch adds will never be reached in 6.11 because
>>>>> the kernel does not have the corresponding code.
>>>>
>>>> What should/would happen if this test is run on a kernel that doesn't
>>>> support the feature?
>>>
>>> The build system doesn't compile it for kernels without the feature.
>>>
>>
>> That's not how the kselftests should be working.
> 
> If you'd like to get involved in the development of these, by all means
> send patches. As you can see, for 6.12, these were intensely improved in
> all manner of ways:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/tools/testing/selftests/vDSO
> 
> Just look at that flurry of activity. Things are getting better! And
> things were in pretty bad shape before. If you think there's an
> interesting subset of that for backporting, by all means go for it, but
> do it thoughtfully and don't pick patches willy-nilly.
> 

This is not different from other kernel APIs and enhancements and
correspo0nding updates to the existing selftests.

The
vdso_test_getrandom test a user-space program exists in 6.11.

Use should be able to run vdso_test_getrandom compiled on 6.12
repo on a 6.11 kernel.

>> They can run on any
>> kernel image (build is separate from running on many test systems), and
>> so they should just fail with whatever the "feature not present" error
>> is if the feature isn't present in the system-that-is-being-tested.
> 

vdso_test_getrandom test a user-space program exists in 6.11.
Users should be able to run vdso_test_getrandom compiled on 6.12
repo on a 6.11 kernel. This is what several CIs do.

> So, it's actually not that clear what the best thing is. Firstly, for
> vdso_test_chacha.c, it can't even compile without the symlink and a
> resolving tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S symlink, which
> is on a per-arch basis. You might say that in this case, it's best to
> condition the Makefile on `ifneq ($(wildcard tools/arch/$(SRCARCH)/vdso/
> vgetrandom-chacha.S),)` instead of on $(ARCH), but there's this ugly
> wrinkle where some of the code that's being compiled is 64-bit only, and
> x86_64 and x86 share a $(SRCARCH) path. (That Makefile makes use of
> $(CONFIG_X86_32), which is pretty gross and might not work; I'm not yet
> sure how to fix that.) Christophe experimented with having the compiler
> decide, and then there being a runtime result, but it added a lot of
> complexity that didn't seem necessary. There's more experimentation to
> be done here.
> 
> Meanwhile, part of vdso_test_getrandom.c's purpose is to test whether
> __kernel_getrandom() or __vdso_getrandom() is actually being properly
> exported from the vDSO. This is also interesting on powerpc, where it's
> implemented on both 32-bit and 64-bit, so there's the compat case to
> worry about. That in turn means that this test has in it:
> 
> 	vgrnd.fn = (__typeof__(vgrnd.fn))vdso_sym(version, name);
> 	if (!vgrnd.fn) {
> 		printf("%s is missing!\n", name);
> 		exit(KSFT_FAIL);
> 	}
> 

x86 selftest handles 32 vs 64-bit related scenarios now. You can take
a look at the Makefile. You can also split the test specific to 32 vs
64 and compile it for native and cross-compile cases.

> And not exit(KSFT_SKIP), since that would hide the failure to export the
> symbol. Now, you could say that since development on the fundamental
> part is mostly concluded, we could move to a KSFT_SKIP, in order to
> simplify the build choice and such. I'm not sure where I stand on that.

If I am understanding this correctly, KSFT_FAIL is used during development
and as of today, KSFT_SKIP is the correct exit code??

> At the very least, there's still RISC-V coming down the pipeline for
> this feature, so it probably would change after that comes out.
> 

This can be handled as part RISC-V.

> Anyway, that is all to say that this stuff has been thoroughly
> considered, not haphazardly glued together or something. Maybe that
> consideration has reached wrong conclusions -- that's an entirely
> possible of an outcome -- but it wouldn't be for lack of caring. If
> you'd like to contribute to it, I'd certainly welcome a hand. But please
> don't do the arm-chair coding thing.
> 

Probably. We don't know what we don't know. selftest use-cases are the
ones we are discussing here.

You can check the kselftest use-cases and contribution guidelines
in kselftest.rst

> Meanwhile, this ENOSYS thing has nothing to do with what either of you
> assumed it does. This is to handle obscure/exotic arm64 hardware, which
> might not exist in the Linux world, that doesn't have NEON support. But
> since arm64 support for this function didn't even come to Linux 6.11,
> there's no point in discussing it as a backport.

#define ENOSYS          78      /* Function not implemented */

user-space understands ENOSYS as feature/function not implemented.

If ENOSYS is the right choice for this obscure/exotic arm64 hardware?

The user-space vdso_test_getrandom test has to run on all architectures
if can be compiled (unless Makefile restricts the compile) and older
releases and handle not finding the feature and fail gracefully.

thanks,
-- Shuah