[PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 tests/nvme/043 | 1 +
 tests/nvme/044 | 1 +
 tests/nvme/045 | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tests/nvme/043 b/tests/nvme/043
index 381ae755f140..87273e5b414d 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -16,6 +16,7 @@ requires() {
 	_have_kernel_option NVME_TARGET_AUTH
 	_require_nvme_trtype_is_fabrics
 	_require_nvme_cli_auth
+	_have_driver dh_generic
 }
 
 
diff --git a/tests/nvme/044 b/tests/nvme/044
index 046553198ce3..13019659b951 100755
--- a/tests/nvme/044
+++ b/tests/nvme/044
@@ -16,6 +16,7 @@ requires() {
 	_have_kernel_option NVME_TARGET_AUTH
 	_require_nvme_trtype_is_fabrics
 	_require_nvme_cli_auth
+	_have_driver dh_generic
 }
 
 
diff --git a/tests/nvme/045 b/tests/nvme/045
index b60f18fc9f87..264f21053921 100755
--- a/tests/nvme/045
+++ b/tests/nvme/045
@@ -16,6 +16,7 @@ requires() {
 	_have_kernel_option NVME_TARGET_AUTH
 	_require_nvme_trtype_is_fabrics
 	_require_nvme_cli_auth
+	_have_driver dh_generic
 }
 
 
-- 
2.34.1

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Shinichiro Kawasaki]

Hi Sagi,

On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  tests/nvme/043 | 1 +
>  tests/nvme/044 | 1 +
>  tests/nvme/045 | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/tests/nvme/043 b/tests/nvme/043
> index 381ae755f140..87273e5b414d 100755
> --- a/tests/nvme/043
> +++ b/tests/nvme/043
> @@ -16,6 +16,7 @@ requires() {
>  	_have_kernel_option NVME_TARGET_AUTH
>  	_require_nvme_trtype_is_fabrics
>  	_require_nvme_cli_auth
> +	_have_driver dh_generic
>  }

Do you see failure without this check? As far as I understand, this new check is
equivalent to '_have_kernel_option CRYPTO_DH'. This test case already requires
'_have_kernel_option NVME_AUTH' and CONFIG_NVME_AUTH selects CONFIG_CRYPTO_DH.
So, the new check does not look required assuming dh_generic.ko is built and
installed correctly.

>  
>  
> diff --git a/tests/nvme/044 b/tests/nvme/044
> index 046553198ce3..13019659b951 100755
> --- a/tests/nvme/044
> +++ b/tests/nvme/044
> @@ -16,6 +16,7 @@ requires() {
>  	_have_kernel_option NVME_TARGET_AUTH
>  	_require_nvme_trtype_is_fabrics
>  	_require_nvme_cli_auth
> +	_have_driver dh_generic
>  }
>  
>  
> diff --git a/tests/nvme/045 b/tests/nvme/045
> index b60f18fc9f87..264f21053921 100755
> --- a/tests/nvme/045
> +++ b/tests/nvme/045
> @@ -16,6 +16,7 @@ requires() {
>  	_have_kernel_option NVME_TARGET_AUTH
>  	_require_nvme_trtype_is_fabrics
>  	_require_nvme_cli_auth
> +	_have_driver dh_generic
>  }
>  
>  
> -- 
> 2.34.1
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Sagi Grimberg]

> Hi Sagi,
> 
> On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   tests/nvme/043 | 1 +
>>   tests/nvme/044 | 1 +
>>   tests/nvme/045 | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/tests/nvme/043 b/tests/nvme/043
>> index 381ae755f140..87273e5b414d 100755
>> --- a/tests/nvme/043
>> +++ b/tests/nvme/043
>> @@ -16,6 +16,7 @@ requires() {
>>   	_have_kernel_option NVME_TARGET_AUTH
>>   	_require_nvme_trtype_is_fabrics
>>   	_require_nvme_cli_auth
>> +	_have_driver dh_generic
>>   }
> 
> Do you see failure without this check?

Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Jackie Liu]

Hi Sagi.

在 2022/8/30 15:05, Sagi Grimberg 写道:
> 
>> Hi Sagi,
>>
>> On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>>   tests/nvme/043 | 1 +
>>>   tests/nvme/044 | 1 +
>>>   tests/nvme/045 | 1 +
>>>   3 files changed, 3 insertions(+)
>>>
>>> diff --git a/tests/nvme/043 b/tests/nvme/043
>>> index 381ae755f140..87273e5b414d 100755
>>> --- a/tests/nvme/043
>>> +++ b/tests/nvme/043
>>> @@ -16,6 +16,7 @@ requires() {
>>>       _have_kernel_option NVME_TARGET_AUTH
>>>       _require_nvme_trtype_is_fabrics
>>>       _require_nvme_cli_auth
>>> +    _have_driver dh_generic
>>>   }
>>
>> Do you see failure without this check?
> 
> Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).

Maybe I don't understand correctly, but I think when NVME_AUTH=y
CONFIG_CRYPTO_DH can only be built in.

-- 
Jackie Liu

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Sagi Grimberg]

> Hi Sagi.
> 
> 在 2022/8/30 15:05, Sagi Grimberg 写道:
>>
>>> Hi Sagi,
>>>
>>> On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>> ---
>>>>   tests/nvme/043 | 1 +
>>>>   tests/nvme/044 | 1 +
>>>>   tests/nvme/045 | 1 +
>>>>   3 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/tests/nvme/043 b/tests/nvme/043
>>>> index 381ae755f140..87273e5b414d 100755
>>>> --- a/tests/nvme/043
>>>> +++ b/tests/nvme/043
>>>> @@ -16,6 +16,7 @@ requires() {
>>>>       _have_kernel_option NVME_TARGET_AUTH
>>>>       _require_nvme_trtype_is_fabrics
>>>>       _require_nvme_cli_auth
>>>> +    _have_driver dh_generic
>>>>   }
>>>
>>> Do you see failure without this check?
>>
>> Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).
> 
> Maybe I don't understand correctly, but I think when NVME_AUTH=y
> CONFIG_CRYPTO_DH can only be built in.
> 

No.

In my .config:
--
...
#
# NVME Support
#
CONFIG_NVME_COMMON=m
CONFIG_NVME_CORE=m
CONFIG_BLK_DEV_NVME=m
CONFIG_NVME_MULTIPATH=y
# CONFIG_NVME_VERBOSE_ERRORS is not set
CONFIG_NVME_HWMON=y
CONFIG_NVME_FABRICS=m
CONFIG_NVME_RDMA=m
CONFIG_NVME_FC=m
CONFIG_NVME_TCP=m
CONFIG_NVME_AUTH=y
CONFIG_NVME_TARGET=m
CONFIG_NVME_TARGET_PASSTHRU=y
CONFIG_NVME_TARGET_LOOP=m
CONFIG_NVME_TARGET_RDMA=m
CONFIG_NVME_TARGET_FC=m
CONFIG_NVME_TARGET_FCLOOP=m
CONFIG_NVME_TARGET_TCP=m
CONFIG_NVME_TARGET_AUTH=y
# end of NVME Support
...
#
# Public-key cryptography
#
CONFIG_CRYPTO_RSA=y
CONFIG_CRYPTO_DH=m
CONFIG_CRYPTO_DH_RFC7919_GROUPS=y
CONFIG_CRYPTO_ECC=m
CONFIG_CRYPTO_ECDH=m
# CONFIG_CRYPTO_ECDSA is not set
# CONFIG_CRYPTO_ECRDSA is not set
# CONFIG_CRYPTO_SM2 is not set
# CONFIG_CRYPTO_CURVE25519 is not set
# CONFIG_CRYPTO_CURVE25519_X86 is not set
--

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Jackie Liu]

My fault, my config file becomes y because NVME_CORE is also y.
sorry to bother you.

-- 
Jackie Liu

在 2022/8/30 15:37, Sagi Grimberg 写道:
> 
>> Hi Sagi.
>>
>> 在 2022/8/30 15:05, Sagi Grimberg 写道:
>>>
>>>> Hi Sagi,
>>>>
>>>> On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
>>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>> ---
>>>>>   tests/nvme/043 | 1 +
>>>>>   tests/nvme/044 | 1 +
>>>>>   tests/nvme/045 | 1 +
>>>>>   3 files changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/tests/nvme/043 b/tests/nvme/043
>>>>> index 381ae755f140..87273e5b414d 100755
>>>>> --- a/tests/nvme/043
>>>>> +++ b/tests/nvme/043
>>>>> @@ -16,6 +16,7 @@ requires() {
>>>>>       _have_kernel_option NVME_TARGET_AUTH
>>>>>       _require_nvme_trtype_is_fabrics
>>>>>       _require_nvme_cli_auth
>>>>> +    _have_driver dh_generic
>>>>>   }
>>>>
>>>> Do you see failure without this check?
>>>
>>> Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).
>>
>> Maybe I don't understand correctly, but I think when NVME_AUTH=y
>> CONFIG_CRYPTO_DH can only be built in.
>>
> 
> No.
> 
> In my .config:
> -- 
> ...
> #
> # NVME Support
> #
> CONFIG_NVME_COMMON=m
> CONFIG_NVME_CORE=m
> CONFIG_BLK_DEV_NVME=m
> CONFIG_NVME_MULTIPATH=y
> # CONFIG_NVME_VERBOSE_ERRORS is not set
> CONFIG_NVME_HWMON=y
> CONFIG_NVME_FABRICS=m
> CONFIG_NVME_RDMA=m
> CONFIG_NVME_FC=m
> CONFIG_NVME_TCP=m
> CONFIG_NVME_AUTH=y
> CONFIG_NVME_TARGET=m
> CONFIG_NVME_TARGET_PASSTHRU=y
> CONFIG_NVME_TARGET_LOOP=m
> CONFIG_NVME_TARGET_RDMA=m
> CONFIG_NVME_TARGET_FC=m
> CONFIG_NVME_TARGET_FCLOOP=m
> CONFIG_NVME_TARGET_TCP=m
> CONFIG_NVME_TARGET_AUTH=y
> # end of NVME Support
> ...
> #
> # Public-key cryptography
> #
> CONFIG_CRYPTO_RSA=y
> CONFIG_CRYPTO_DH=m
> CONFIG_CRYPTO_DH_RFC7919_GROUPS=y
> CONFIG_CRYPTO_ECC=m
> CONFIG_CRYPTO_ECDH=m
> # CONFIG_CRYPTO_ECDSA is not set
> # CONFIG_CRYPTO_ECRDSA is not set
> # CONFIG_CRYPTO_SM2 is not set
> # CONFIG_CRYPTO_CURVE25519 is not set
> # CONFIG_CRYPTO_CURVE25519_X86 is not set
> -- 

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Shinichiro Kawasaki]

On Aug 30, 2022 / 10:05, Sagi Grimberg wrote:
> 
> > Hi Sagi,
> > 
> > On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > > ---
> > >   tests/nvme/043 | 1 +
> > >   tests/nvme/044 | 1 +
> > >   tests/nvme/045 | 1 +
> > >   3 files changed, 3 insertions(+)
> > > 
> > > diff --git a/tests/nvme/043 b/tests/nvme/043
> > > index 381ae755f140..87273e5b414d 100755
> > > --- a/tests/nvme/043
> > > +++ b/tests/nvme/043
> > > @@ -16,6 +16,7 @@ requires() {
> > >   	_have_kernel_option NVME_TARGET_AUTH
> > >   	_require_nvme_trtype_is_fabrics
> > >   	_require_nvme_cli_auth
> > > +	_have_driver dh_generic
> > >   }
> > 
> > Do you see failure without this check?
> 
> Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).

Thanks. I was able to recreate the failure setting CONFIG_CRYPTO_DH=m.

Unfortunately, your patch does not avoid the failure after the recent commit
06a0ba866d90 ("common/rc: avoid module load in _have_driver()"). As the commit
title says, _have_driver no longer has the side-effect to leave the dh_generic
module loaded. Instead, I suggest to load dh_generic in the test() function:

diff --git a/tests/nvme/043 b/tests/nvme/043
index 381ae75..dbe9d3f 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -40,6 +40,8 @@ test() {

        _setup_nvmet

+       modprobe -q dh_generic
+
        truncate -s 512M "${file_path}"

        _create_nvmet_subsystem "${subsys_name}" "${file_path}"
@@ -88,5 +90,7 @@ test() {

        rm "${file_path}"

+       modprobe -qr dh_generic
+
        echo "Test complete"
 }

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Sagi Grimberg]

>>
>>> Hi Sagi,
>>>
>>> On Aug 29, 2022 / 11:36, Sagi Grimberg wrote:
>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>> ---
>>>>    tests/nvme/043 | 1 +
>>>>    tests/nvme/044 | 1 +
>>>>    tests/nvme/045 | 1 +
>>>>    3 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/tests/nvme/043 b/tests/nvme/043
>>>> index 381ae755f140..87273e5b414d 100755
>>>> --- a/tests/nvme/043
>>>> +++ b/tests/nvme/043
>>>> @@ -16,6 +16,7 @@ requires() {
>>>>    	_have_kernel_option NVME_TARGET_AUTH
>>>>    	_require_nvme_trtype_is_fabrics
>>>>    	_require_nvme_cli_auth
>>>> +	_have_driver dh_generic
>>>>    }
>>>
>>> Do you see failure without this check?
>>
>> Yes, if dh_generic is built as a module (CONFIG_CRYPTO_DH=m).
> 
> Thanks. I was able to recreate the failure setting CONFIG_CRYPTO_DH=m.
> 
> Unfortunately, your patch does not avoid the failure after the recent commit
> 06a0ba866d90 ("common/rc: avoid module load in _have_driver()"). As the commit
> title says, _have_driver no longer has the side-effect to leave the dh_generic
> module loaded. Instead, I suggest to load dh_generic in the test() function:
> 
> diff --git a/tests/nvme/043 b/tests/nvme/043
> index 381ae75..dbe9d3f 100755
> --- a/tests/nvme/043
> +++ b/tests/nvme/043
> @@ -40,6 +40,8 @@ test() {
> 
>          _setup_nvmet
> 
> +       modprobe -q dh_generic
> +
>          truncate -s 512M "${file_path}"
> 
>          _create_nvmet_subsystem "${subsys_name}" "${file_path}"
> @@ -88,5 +90,7 @@ test() {
> 
>          rm "${file_path}"
> 
> +       modprobe -qr dh_generic
> +
>          echo "Test complete"
>   }
> 

That's fine with me, can you prepare an alternative patch for it?

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Shinichiro Kawasaki]

On Aug 30, 2022 / 16:26, Sagi Grimberg wrote:

[...]

> > Unfortunately, your patch does not avoid the failure after the recent commit
> > 06a0ba866d90 ("common/rc: avoid module load in _have_driver()"). As the commit
> > title says, _have_driver no longer has the side-effect to leave the dh_generic
> > module loaded. Instead, I suggest to load dh_generic in the test() function:
> > 
> > diff --git a/tests/nvme/043 b/tests/nvme/043
> > index 381ae75..dbe9d3f 100755
> > --- a/tests/nvme/043
> > +++ b/tests/nvme/043
> > @@ -40,6 +40,8 @@ test() {
> > 
> >          _setup_nvmet
> > 
> > +       modprobe -q dh_generic
> > +
> >          truncate -s 512M "${file_path}"
> > 
> >          _create_nvmet_subsystem "${subsys_name}" "${file_path}"
> > @@ -88,5 +90,7 @@ test() {
> > 
> >          rm "${file_path}"
> > 
> > +       modprobe -qr dh_generic
> > +
> >          echo "Test complete"
> >   }
> > 
> 
> That's fine with me, can you prepare an alternative patch for it?

Sure, will post it soon.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme: add dh module requirement for tests that involve dh groups

[Shinichiro Kawasaki]

On Aug 31, 2022 / 04:59, Shinichiro Kawasaki wrote:
> On Aug 30, 2022 / 16:26, Sagi Grimberg wrote:
> 
> [...]
> 
> > > Unfortunately, your patch does not avoid the failure after the recent commit
> > > 06a0ba866d90 ("common/rc: avoid module load in _have_driver()"). As the commit
> > > title says, _have_driver no longer has the side-effect to leave the dh_generic
> > > module loaded. Instead, I suggest to load dh_generic in the test() function:
> > > 
> > > diff --git a/tests/nvme/043 b/tests/nvme/043
> > > index 381ae75..dbe9d3f 100755
> > > --- a/tests/nvme/043
> > > +++ b/tests/nvme/043
> > > @@ -40,6 +40,8 @@ test() {
> > > 
> > >          _setup_nvmet
> > > 
> > > +       modprobe -q dh_generic
> > > +
> > >          truncate -s 512M "${file_path}"
> > > 
> > >          _create_nvmet_subsystem "${subsys_name}" "${file_path}"
> > > @@ -88,5 +90,7 @@ test() {
> > > 
> > >          rm "${file_path}"
> > > 
> > > +       modprobe -qr dh_generic
> > > +
> > >          echo "Test complete"
> > >   }
> > > 
> > 
> > That's fine with me, can you prepare an alternative patch for it?
> 
> Sure, will post it soon.

As discussed, the alternative patch I suggested was not good [1]. Then I applied
your original fix together with improvement of _have_driver() [2].

Thanks!

[1] https://lore.kernel.org/linux-block/20220901063551.djziyblzw6g2cxwd@shindev/
[2] https://lore.kernel.org/linux-block/20220902034516.223173-1-shinichiro.kawasaki@wdc.com/

-- 
Shin'ichiro Kawasaki