[PATCH 0/2] encrypt: add ceph support

[PATCH 0/2] encrypt: add ceph support

[xiubli]

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

[PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted

[xiubli]

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

[PATCH 2/2] encrypt: add ceph support

[xiubli]

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

Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted

[Zorro Lang]

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
> 

Re: [PATCH 2/2] encrypt: add ceph support

[Zorro Lang]

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
> 

Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted

[Xiubo Li]

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

Re: [PATCH 2/2] encrypt: add ceph support

[Xiubo Li]

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