| Message ID | 20220829083614.874878-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] nvme: add dh module requirement for tests that involve dh groups | expand |
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 >
> 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).
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.
> 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 --
My fault, my config file becomes y because NVME_CORE is also y. sorry to bother you.
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" }
>> >>> 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?
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.
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/
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 }
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(+)