Re: [PATCH net v2 1/3] selftests: net: tls: check if FIPS mode is enabled

[Magali Lemes do Sacramento]

Hi!

On Fri, Jun 9, 2023 at 2:53 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Fri,  9 Jun 2023 13:43:22 -0300 Magali Lemes wrote:
> > diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> > index e699548d4247..0725c60f227c 100644
> > --- a/tools/testing/selftests/net/tls.c
> > +++ b/tools/testing/selftests/net/tls.c
> > @@ -25,6 +25,8 @@
> >  #define TLS_PAYLOAD_MAX_LEN 16384
> >  #define SOL_TLS 282
> >
> > +static int fips_enabled = 0;
>
> No need to zero init static variables, but really instead of doing
> the main() hack you should init this to a return value of a function.
> And have that function read the value.
>

I'm not sure I understand what you mean here. I agree we want to avoid
reading the /proc/sys/crypto/fips_enabled file for every test.
However, correct me where I'm wrong, if we want to have fips_enabled
as a static global variable I don't think we can directly initialize
it from the return value of a function.
Could you clarify that, please?

Best regards,
Magali

> >  struct tls_crypto_info_keys {
> >       union {
> >               struct tls12_crypto_info_aes_gcm_128 aes128;
>
> > @@ -311,6 +317,9 @@ FIXTURE_SETUP(tls)
> >       int one = 1;
> >       int ret;
> >
> > +     if (fips_enabled && variant->fips_non_compliant)
> > +             return;
>
> Eh, let me help you, this should really be part of the SETUP() function
> but SETUP() doesn't currently handle SKIP(). So you'll need to add this
> to your series:
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index d8bff2005dfc..3091c345452e 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -249,7 +249,7 @@
>
>  /**
>   * FIXTURE_SETUP() - Prepares the setup function for the fixture.
> - * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
> + * *_metadata* is included so that EXPECT_*, ASSERT_* etc. work correctly.
>   *
>   * @fixture_name: fixture name
>   *
> @@ -275,7 +275,7 @@
>
>  /**
>   * FIXTURE_TEARDOWN()
> - * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
> + * *_metadata* is included so that EXPECT_*, ASSERT_* etc. work correctly.
>   *
>   * @fixture_name: fixture name
>   *
> @@ -388,7 +388,7 @@
>                 if (setjmp(_metadata->env) == 0) { \
>                         fixture_name##_setup(_metadata, &self, variant->data); \
>                         /* Let setup failure terminate early. */ \
> -                       if (!_metadata->passed) \
> +                       if (!_metadata->passed || _metadata->skip) \
>                                 return; \
>                         _metadata->setup_completed = true; \
>                         fixture_name##_##test_name(_metadata, &self, variant->data); \