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

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

[Jason A. Donenfeld]

This is not stable material and I didn't mark it as such. Do not backport.

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

[Shuah Khan]

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.

Does this build on stables?

thanks,
-- Shuah

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

[Jason A. Donenfeld]

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.

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

[Shuah Khan]

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??

thanks,
-- Shuah

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

[Jason A. Donenfeld]

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.

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

[Shuah Khan]

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?

thanks,
-- Shuah

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

[Jason A. Donenfeld]

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.

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

[Greg KH]

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.  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.

thanks,

greg k-h

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

[Jason A. Donenfeld]

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.

> 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.

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);
	}

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.
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.

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.

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.

Jason

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

[Shuah Khan]

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

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

[Jason A. Donenfeld]

On Wed, Oct 02, 2024 at 01:45:57PM -0600, Shuah Khan wrote:
> 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.
> 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.

The x86 test from 6.12 works just fine on 6.11.

I really don't follow you at all or what you're getting at. I think if
you actually look at the code, you'll be mostly okay with it. And if
there's something that looks awry to you, send a patch or describe to me
clearly what looks wrong and I'll send a patch.

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

[Shuah Khan]

On 10/2/24 15:01, Jason A. Donenfeld wrote:
> On Wed, Oct 02, 2024 at 01:45:57PM -0600, Shuah Khan wrote:
>> 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.
>> 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.
> 
> The x86 test from 6.12 works just fine on 6.11.

Yes x86 test is a good example to look at that handles 32-bit
and 640-bit issues you brought up in your email.

That is reason why I asked you to refer to x86 Makefile to
solve the architecture differences related to this test you
mentioned in your email.

> 
> I really don't follow you at all or what you're getting at. I think if
> you actually look at the code, you'll be mostly okay with it. And if
> there's something that looks awry to you, send a patch or describe to me
> clearly what looks wrong and I'll send a patch.
To be very clear about the selftest guidelines (covered in
kselftest.rst document)

1. Ideally selftests should compile on all architectures.
    Exception to this are few architecture specific
    features which can be selectively compiled either
    with ifdef statements in the code or Makefile arch
    checks.
    
    The goal is to keep these to a minimum so we can a wide
    range of tests in CIs and other test systems.

2. Selftest from mainline should run on stable releases
    handling missing features and missing config options
    with skip so we don't have to deal false failures.

    The goal is to minimize false negatives and false positives.

3. Reported results are clear to the users and testers.

Thank you for the patches. I will review your patches and give you
feedback.

thanks,
-- Shuah

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

[Jason A. Donenfeld]

On Thu, Oct 3, 2024 at 6:53 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
> > The x86 test from 6.12 works just fine on 6.11.
>
> Yes x86 test is a good example to look at that handles 32-bit
> and 640-bit issues you brought up in your email.

The x86 test is only for 64-bit. It doesn't get compiled on 32-bit.
That changes in the series I attached.

> 1. Ideally selftests should compile on all architectures.
>     Exception to this are few architecture specific
>     features which can be selectively compiled either
>     with ifdef statements in the code or Makefile arch
>     checks.

The thing is, this *is* a test with an architecture-specific feature:
it's not available on all archs, especially not 32-bit ones. There's
the workaround (in the attached series) that I think might help
things. But for the chacha one, it just won't compile on systems that
lack the implementation.

> 2. Selftest from mainline should run on stable releases
>     handling missing features and missing config options
>     with skip so we don't have to deal false failures.

Noted. I would like to point out, though, that this is the "opposite"
stability guarantee that Linux usually offers of old binaries working
on new systems. You want new binaries working on old systems.

> 3. Reported results are clear to the users and testers.

Patch 3/3 fixes this up.

> Thank you for the patches. I will review your patches and give you
> feedback.

Great, okay. I think the series matches your expectations.

I have additional ideas too for the chacha test, if you think it's
necessary to go further, but maybe things are good enough now.

Jason

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

[Jason A. Donenfeld]

On Thu, Oct 03, 2024 at 06:58:26PM +0200, Jason A. Donenfeld wrote:
> Great, okay. I think the series matches your expectations.
> 
> I have additional ideas too for the chacha test, if you think it's
> necessary to go further, but maybe things are good enough now.

Posted patch 4/3 for this purpose:
    https://lore.kernel.org/all/20241003171852.2386463-1-Jason@zx2c4.com/