* [PATCH 0/2] encrypt: add ceph support @ 2022-10-26 7:04 xiubli 2022-10-26 7:04 ` [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted xiubli 2022-10-26 7:04 ` [PATCH 2/2] encrypt: add ceph support xiubli 0 siblings, 2 replies; 7+ messages in thread From: xiubli @ 2022-10-26 7:04 UTC (permalink / raw) To: fstests; +Cc: david, djwong, lhenriques, ceph-devel, jlayton, mchangir, Xiubo Li From: Xiubo Li <xiubli@redhat.com> Xiubo Li (2): encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted encrypt: add ceph support common/encrypt | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted 2022-10-26 7:04 [PATCH 0/2] encrypt: add ceph support xiubli @ 2022-10-26 7:04 ` xiubli 2022-10-26 14:04 ` Zorro Lang 2022-10-26 7:04 ` [PATCH 2/2] encrypt: add ceph support xiubli 1 sibling, 1 reply; 7+ messages in thread From: xiubli @ 2022-10-26 7:04 UTC (permalink / raw) To: fstests; +Cc: david, djwong, lhenriques, ceph-devel, jlayton, mchangir, Xiubo Li From: Xiubo Li <xiubli@redhat.com> For ceph we couldn't check the encryption feature by mkfs, we need to mount it first and then check the 'get_encpolicy' ioctl cmd. Signed-off-by: Xiubo Li <xiubli@redhat.com> --- common/encrypt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/common/encrypt b/common/encrypt index 45ce0954..fd620c41 100644 --- a/common/encrypt +++ b/common/encrypt @@ -29,7 +29,7 @@ _require_scratch_encryption() # Make a filesystem on the scratch device with the encryption feature # enabled. If this fails then probably the userspace tools (e.g. # e2fsprogs or f2fs-tools) are too old to understand encryption. - if ! _scratch_mkfs_encrypted &>>$seqres.full; then + if ! _scratch_check_encrypted &>>$seqres.full; then _notrun "$FSTYP userspace tools do not support encryption" fi @@ -143,7 +143,7 @@ _require_encryption_policy_support() rm -r $dir } -_scratch_mkfs_encrypted() +_scratch_check_encrypted() { case $FSTYP in ext4|f2fs) @@ -171,7 +171,7 @@ _scratch_mkfs_sized_encrypted() esac } -# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the +# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the # filesystem type. This is necessary for using encryption policies that include # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set. _scratch_mkfs_stable_inodes_encrypted() @@ -183,7 +183,7 @@ _scratch_mkfs_stable_inodes_encrypted() fi ;; *) - _scratch_mkfs_encrypted + _scratch_check_encrypted ;; esac } @@ -923,7 +923,7 @@ _verify_ciphertext_for_encryption_policy() FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then _scratch_mkfs_stable_inodes_encrypted &>> $seqres.full else - _scratch_mkfs_encrypted &>> $seqres.full + _scratch_check_encrypted &>> $seqres.full fi _scratch_mount -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted 2022-10-26 7:04 ` [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted xiubli @ 2022-10-26 14:04 ` Zorro Lang 2022-10-27 2:19 ` Xiubo Li 0 siblings, 1 reply; 7+ messages in thread From: Zorro Lang @ 2022-10-26 14:04 UTC (permalink / raw) To: xiubli; +Cc: fstests, david, djwong, lhenriques, ceph-devel, jlayton, mchangir On Wed, Oct 26, 2022 at 03:04:17PM +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > For ceph we couldn't check the encryption feature by mkfs, we need > to mount it first and then check the 'get_encpolicy' ioctl cmd. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- This patch only does an *incomplete* function rename, without any change on that function body, that doesn't make sense, and even will bring in regression, due to lots of cases depend on this common function. If you really need to change a common function, please "grep" [1] this function in xfstests-dev/ to find out and check all places use it at least. The _scratch_mkfs_encrypted is a "mkfs" function, likes _scratch_mkfs. I think we shouldn't change its name. If you want to check if encryption feature is supported by ceph, I think you might hope to do that in _require_* functions, or you even can have a ceph specific function in common/ceph. Just not the way you did in this patchset. Thanks, Zorro [1] $ grep -rsn _scratch_mkfs_encrypted . ./common/encrypt:32: if ! _scratch_mkfs_encrypted &>>$seqres.full; then ./common/encrypt:146:_scratch_mkfs_encrypted() ./common/encrypt:174:# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the ./common/encrypt:186: _scratch_mkfs_encrypted ./common/encrypt:926: _scratch_mkfs_encrypted &>> $seqres.full ./common/verity:178:_scratch_mkfs_encrypted_verity() ./common/verity:190: _notrun "$FSTYP not supported in _scratch_mkfs_encrypted_verity" ./tests/ext4/024:36:_scratch_mkfs_encrypted &>>$seqres.full ./tests/generic/395:22:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/396:21:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/580:23:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/581:36:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/595:35:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/613:29:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/621:57:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/429:36:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/397:28:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/398:28:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/421:24:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/440:29:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/419:29:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/435:33:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/593:24:_scratch_mkfs_encrypted &>> $seqres.full ./tests/generic/576:34:_scratch_mkfs_encrypted_verity &>> $seqres.full > common/encrypt | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/common/encrypt b/common/encrypt > index 45ce0954..fd620c41 100644 > --- a/common/encrypt > +++ b/common/encrypt > @@ -29,7 +29,7 @@ _require_scratch_encryption() > # Make a filesystem on the scratch device with the encryption feature > # enabled. If this fails then probably the userspace tools (e.g. > # e2fsprogs or f2fs-tools) are too old to understand encryption. > - if ! _scratch_mkfs_encrypted &>>$seqres.full; then > + if ! _scratch_check_encrypted &>>$seqres.full; then > _notrun "$FSTYP userspace tools do not support encryption" > fi > > @@ -143,7 +143,7 @@ _require_encryption_policy_support() > rm -r $dir > } > > -_scratch_mkfs_encrypted() > +_scratch_check_encrypted() > { > case $FSTYP in > ext4|f2fs) > @@ -171,7 +171,7 @@ _scratch_mkfs_sized_encrypted() > esac > } > > -# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the > +# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the > # filesystem type. This is necessary for using encryption policies that include > # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set. > _scratch_mkfs_stable_inodes_encrypted() > @@ -183,7 +183,7 @@ _scratch_mkfs_stable_inodes_encrypted() > fi > ;; > *) > - _scratch_mkfs_encrypted > + _scratch_check_encrypted > ;; > esac > } > @@ -923,7 +923,7 @@ _verify_ciphertext_for_encryption_policy() > FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then > _scratch_mkfs_stable_inodes_encrypted &>> $seqres.full > else > - _scratch_mkfs_encrypted &>> $seqres.full > + _scratch_check_encrypted &>> $seqres.full > fi > _scratch_mount > > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted 2022-10-26 14:04 ` Zorro Lang @ 2022-10-27 2:19 ` Xiubo Li 0 siblings, 0 replies; 7+ messages in thread From: Xiubo Li @ 2022-10-27 2:19 UTC (permalink / raw) To: Zorro Lang Cc: fstests, david, djwong, lhenriques, ceph-devel, jlayton, mchangir On 26/10/2022 22:04, Zorro Lang wrote: > On Wed, Oct 26, 2022 at 03:04:17PM +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> For ceph we couldn't check the encryption feature by mkfs, we need >> to mount it first and then check the 'get_encpolicy' ioctl cmd. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- > This patch only does an *incomplete* function rename, without any change > on that function body, that doesn't make sense, and even will bring in > regression, due to lots of cases depend on this common function. If you > really need to change a common function, please "grep" [1] this function > in xfstests-dev/ to find out and check all places use it at least. > > The _scratch_mkfs_encrypted is a "mkfs" function, likes _scratch_mkfs. I > think we shouldn't change its name. If you want to check if encryption > feature is supported by ceph, I think you might hope to do that in > _require_* functions, or you even can have a ceph specific function in > common/ceph. Just not the way you did in this patchset. Okay, make sense and will do that. Thanks Zorro. - Xiubo > > Thanks, > Zorro > > [1] > $ grep -rsn _scratch_mkfs_encrypted . > ./common/encrypt:32: if ! _scratch_mkfs_encrypted &>>$seqres.full; then > ./common/encrypt:146:_scratch_mkfs_encrypted() > ./common/encrypt:174:# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the > ./common/encrypt:186: _scratch_mkfs_encrypted > ./common/encrypt:926: _scratch_mkfs_encrypted &>> $seqres.full > ./common/verity:178:_scratch_mkfs_encrypted_verity() > ./common/verity:190: _notrun "$FSTYP not supported in _scratch_mkfs_encrypted_verity" > ./tests/ext4/024:36:_scratch_mkfs_encrypted &>>$seqres.full > ./tests/generic/395:22:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/396:21:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/580:23:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/581:36:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/595:35:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/613:29:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/621:57:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/429:36:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/397:28:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/398:28:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/421:24:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/440:29:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/419:29:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/435:33:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/593:24:_scratch_mkfs_encrypted &>> $seqres.full > ./tests/generic/576:34:_scratch_mkfs_encrypted_verity &>> $seqres.full > > > >> common/encrypt | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/common/encrypt b/common/encrypt >> index 45ce0954..fd620c41 100644 >> --- a/common/encrypt >> +++ b/common/encrypt >> @@ -29,7 +29,7 @@ _require_scratch_encryption() >> # Make a filesystem on the scratch device with the encryption feature >> # enabled. If this fails then probably the userspace tools (e.g. >> # e2fsprogs or f2fs-tools) are too old to understand encryption. >> - if ! _scratch_mkfs_encrypted &>>$seqres.full; then >> + if ! _scratch_check_encrypted &>>$seqres.full; then >> _notrun "$FSTYP userspace tools do not support encryption" >> fi >> >> @@ -143,7 +143,7 @@ _require_encryption_policy_support() >> rm -r $dir >> } >> >> -_scratch_mkfs_encrypted() >> +_scratch_check_encrypted() >> { >> case $FSTYP in >> ext4|f2fs) >> @@ -171,7 +171,7 @@ _scratch_mkfs_sized_encrypted() >> esac >> } >> >> -# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the >> +# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the >> # filesystem type. This is necessary for using encryption policies that include >> # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set. >> _scratch_mkfs_stable_inodes_encrypted() >> @@ -183,7 +183,7 @@ _scratch_mkfs_stable_inodes_encrypted() >> fi >> ;; >> *) >> - _scratch_mkfs_encrypted >> + _scratch_check_encrypted >> ;; >> esac >> } >> @@ -923,7 +923,7 @@ _verify_ciphertext_for_encryption_policy() >> FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then >> _scratch_mkfs_stable_inodes_encrypted &>> $seqres.full >> else >> - _scratch_mkfs_encrypted &>> $seqres.full >> + _scratch_check_encrypted &>> $seqres.full >> fi >> _scratch_mount >> >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] encrypt: add ceph support 2022-10-26 7:04 [PATCH 0/2] encrypt: add ceph support xiubli 2022-10-26 7:04 ` [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted xiubli @ 2022-10-26 7:04 ` xiubli 2022-10-26 14:12 ` Zorro Lang 1 sibling, 1 reply; 7+ messages in thread From: xiubli @ 2022-10-26 7:04 UTC (permalink / raw) To: fstests; +Cc: david, djwong, lhenriques, ceph-devel, jlayton, mchangir, Xiubo Li From: Xiubo Li <xiubli@redhat.com> For ceph we couldn't use the mkfs to check whether the encryption is support or not, we need to mount it first and then check the 'set_encpolicy', etc. Signed-off-by: Xiubo Li <xiubli@redhat.com> --- common/encrypt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/common/encrypt b/common/encrypt index fd620c41..e837c9de 100644 --- a/common/encrypt +++ b/common/encrypt @@ -153,6 +153,23 @@ _scratch_check_encrypted() # erase the UBI volume; reformated automatically on next mount $UBIUPDATEVOL_PROG ${SCRATCH_DEV} -t ;; + ceph) + # Try to mount the filesystem. We need to check whether the encryption + # is support or not via the ioctl cmd, such as 'set_encpolicy'. + if ! _try_scratch_mount &>>$seqres.full; then + _notrun "kernel is unaware of $FSTYP encryption feature," \ + "or mkfs options are not compatible with encryption" + fi + + mkdir $SCRATCH_MNT/tmpdir + if _set_encpolicy $SCRATCH_MNT/tmpdir 2>&1 >>$seqres.full | \ + grep -Eq 'Inappropriate ioctl for device|Operation not supported' + then + _notrun "kernel does not support $FSTYP encryption" + fi + rmdir $SCRATCH_MNT/tmpdir + _scratch_unmount + ;; *) _notrun "No encryption support for $FSTYP" ;; -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] encrypt: add ceph support 2022-10-26 7:04 ` [PATCH 2/2] encrypt: add ceph support xiubli @ 2022-10-26 14:12 ` Zorro Lang 2022-10-27 2:22 ` Xiubo Li 0 siblings, 1 reply; 7+ messages in thread From: Zorro Lang @ 2022-10-26 14:12 UTC (permalink / raw) To: xiubli; +Cc: fstests, david, djwong, lhenriques, ceph-devel, jlayton, mchangir On Wed, Oct 26, 2022 at 03:04:18PM +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > For ceph we couldn't use the mkfs to check whether the encryption > is support or not, we need to mount it first and then check the > 'set_encpolicy', etc. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > common/encrypt | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/common/encrypt b/common/encrypt > index fd620c41..e837c9de 100644 > --- a/common/encrypt > +++ b/common/encrypt > @@ -153,6 +153,23 @@ _scratch_check_encrypted() > # erase the UBI volume; reformated automatically on next mount > $UBIUPDATEVOL_PROG ${SCRATCH_DEV} -t > ;; > + ceph) > + # Try to mount the filesystem. We need to check whether the encryption > + # is support or not via the ioctl cmd, such as 'set_encpolicy'. > + if ! _try_scratch_mount &>>$seqres.full; then > + _notrun "kernel is unaware of $FSTYP encryption feature," \ > + "or mkfs options are not compatible with encryption" > + fi > + > + mkdir $SCRATCH_MNT/tmpdir > + if _set_encpolicy $SCRATCH_MNT/tmpdir 2>&1 >>$seqres.full | \ > + grep -Eq 'Inappropriate ioctl for device|Operation not supported' > + then > + _notrun "kernel does not support $FSTYP encryption" > + fi > + rmdir $SCRATCH_MNT/tmpdir > + _scratch_unmount As I replied in patch 1/2, this function is a mkfs function, if ceph need a specific mkfs way, you can do it in this function, or you even can keep it empty ceph) ;; Or does a simple cleanup ceph) _scratch_cleanup_files ... ;; I'm not familar with ceph, that depends on you. But the change in this patch is not "mkfs", it's a checking, checking if the current $SCRATCH_MNT supports encryption, you should do it in other function which does that checking job, not change a mkfs function to be a check function. Thanks, Zorro > + ;; > *) > _notrun "No encryption support for $FSTYP" > ;; > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] encrypt: add ceph support 2022-10-26 14:12 ` Zorro Lang @ 2022-10-27 2:22 ` Xiubo Li 0 siblings, 0 replies; 7+ messages in thread From: Xiubo Li @ 2022-10-27 2:22 UTC (permalink / raw) To: Zorro Lang Cc: fstests, david, djwong, lhenriques, ceph-devel, jlayton, mchangir On 26/10/2022 22:12, Zorro Lang wrote: > On Wed, Oct 26, 2022 at 03:04:18PM +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> For ceph we couldn't use the mkfs to check whether the encryption >> is support or not, we need to mount it first and then check the >> 'set_encpolicy', etc. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> common/encrypt | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/common/encrypt b/common/encrypt >> index fd620c41..e837c9de 100644 >> --- a/common/encrypt >> +++ b/common/encrypt >> @@ -153,6 +153,23 @@ _scratch_check_encrypted() >> # erase the UBI volume; reformated automatically on next mount >> $UBIUPDATEVOL_PROG ${SCRATCH_DEV} -t >> ;; >> + ceph) >> + # Try to mount the filesystem. We need to check whether the encryption >> + # is support or not via the ioctl cmd, such as 'set_encpolicy'. >> + if ! _try_scratch_mount &>>$seqres.full; then >> + _notrun "kernel is unaware of $FSTYP encryption feature," \ >> + "or mkfs options are not compatible with encryption" >> + fi >> + >> + mkdir $SCRATCH_MNT/tmpdir >> + if _set_encpolicy $SCRATCH_MNT/tmpdir 2>&1 >>$seqres.full | \ >> + grep -Eq 'Inappropriate ioctl for device|Operation not supported' >> + then >> + _notrun "kernel does not support $FSTYP encryption" >> + fi >> + rmdir $SCRATCH_MNT/tmpdir >> + _scratch_unmount > As I replied in patch 1/2, this function is a mkfs function, if ceph need a > specific mkfs way, you can do it in this function, or you even can keep it > empty > > ceph) > ;; > > Or does a simple cleanup > > ceph) > _scratch_cleanup_files > ... > ;; > > I'm not familar with ceph, that depends on you. But the change in this patch is > not "mkfs", it's a checking, checking if the current $SCRATCH_MNT supports > encryption, you should do it in other function which does that checking job, not > change a mkfs function to be a check function. Sounds good. Let me check how to that. Thanks Zorro again. - Xiubo > Thanks, > Zorro > >> + ;; >> *) >> _notrun "No encryption support for $FSTYP" >> ;; >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-27 2:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-26 7:04 [PATCH 0/2] encrypt: add ceph support xiubli 2022-10-26 7:04 ` [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted xiubli 2022-10-26 14:04 ` Zorro Lang 2022-10-27 2:19 ` Xiubo Li 2022-10-26 7:04 ` [PATCH 2/2] encrypt: add ceph support xiubli 2022-10-26 14:12 ` Zorro Lang 2022-10-27 2:22 ` Xiubo Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox