[PATCH v2 0/1] Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[PATCH v2 0/1] Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Jan Prusakowski]

Changes in v2: (Thanks to Eric for the review)
- Instead of disabling the test entirely when file compression is used
  we now ensure that compression of the test file is disabled.

Jan Prusakowski (1):
  common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on
    compressed FS

 common/encrypt | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v2 1/1] common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Jan Prusakowski]

verify_ciphertext_for_encryption_policy() checks if encryption works
correctly by reading encrypted file's contents directly from a block device and
comparing it to a known good ciphertext.

This, however, won't work if the test file is also compressed. So this patch
adds a check if a test file is compressed and disables compression in this case.

Signed-off-by: Jan Prusakowski <jprusakowski@google.com>
---
 common/encrypt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/common/encrypt b/common/encrypt
index d4f6e3dc..c25ff5a4 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -790,6 +790,13 @@ _do_verify_ciphertext_for_encryption_policy()
 	_set_encpolicy $dir $keyspec $set_encpolicy_args -f $policy_flags
 	for src in $tmp.testfile_*; do
 		dst=$dir/${src##*.}
+		# To make sure the test file is not compressed we create an empty one
+		# and disable compression first (F2FS won't allow resetting the
+		# compression flag if the file has data already in it).
+		touch $dst
+		if lsattr $dst | grep -qE ".+c.+ $dst" ; then
+			chattr -c $dst
+		fi
 		cp $src $dst
 		inode=$(stat -c %i $dst)
 		blocklist=$(_get_ciphertext_block_list $dst)
-- 
2.51.0.384.g4c02a37b29-goog

Re: [PATCH v2 0/1] Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Eric Biggers]

On Mon, Sep 15, 2025 at 12:04:50PM +0200, Jan Prusakowski wrote:
> Changes in v2: (Thanks to Eric for the review)
> - Instead of disabling the test entirely when file compression is used
>   we now ensure that compression of the test file is disabled.
> 
> Jan Prusakowski (1):
>   common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on
>     compressed FS
> 
>  common/encrypt | 7 +++++++
>  1 file changed, 7 insertions(+)

FYI: for a single patch, there's no need for a cover letter.  If a
changelog is needed, it should be in the patch itself, just below the
"---" line.

- Eric

Re: [PATCH v2 1/1] common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Eric Biggers]

On Mon, Sep 15, 2025 at 12:04:51PM +0200, Jan Prusakowski wrote:
> common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS

A better title would be something like
"common/encrypt: Explicitly set the test file to uncompressed".
(We do still run _verify_ciphertext_for_encryption_policy.  And as I
mentioned before, there's not really any such thing as a compressed FS.)

> verify_ciphertext_for_encryption_policy() checks if encryption works
> correctly by reading encrypted file's contents directly from a block device and
> comparing it to a known good ciphertext.
> 
> This, however, won't work if the test file is also compressed. So this patch
> adds a check if a test file is compressed and disables compression in this case.
> 
> Signed-off-by: Jan Prusakowski <jprusakowski@google.com>
> ---
>  common/encrypt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/common/encrypt b/common/encrypt
> index d4f6e3dc..c25ff5a4 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -790,6 +790,13 @@ _do_verify_ciphertext_for_encryption_policy()
>  	_set_encpolicy $dir $keyspec $set_encpolicy_args -f $policy_flags
>  	for src in $tmp.testfile_*; do
>  		dst=$dir/${src##*.}
> +		# To make sure the test file is not compressed we create an empty one
> +		# and disable compression first (F2FS won't allow resetting the
> +		# compression flag if the file has data already in it).
> +		touch $dst
> +		if lsattr $dst | grep -qE ".+c.+ $dst" ; then
> +			chattr -c $dst
> +		fi
>  		cp $src $dst
>  		inode=$(stat -c %i $dst)
>  		blocklist=$(_get_ciphertext_block_list $dst)

Is adding 'm' (FS_NOCOMP_FL) needed too?  If not, why does it exist?

Also, have you verified that the tests that use this function still pass
on both ext4 and f2fs?

- Eric

Re: [PATCH v2 1/1] common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Jan Prusakowski]

On Mon, Sep 15, 2025 at 4:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Sep 15, 2025 at 12:04:51PM +0200, Jan Prusakowski wrote:
> > common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS
>
> A better title would be something like
> "common/encrypt: Explicitly set the test file to uncompressed".

Agreed, will fix that in v3.

> > @@ -790,6 +790,13 @@ _do_verify_ciphertext_for_encryption_policy()
> >       _set_encpolicy $dir $keyspec $set_encpolicy_args -f $policy_flags
> >       for src in $tmp.testfile_*; do
> >               dst=$dir/${src##*.}
> > +             # To make sure the test file is not compressed we create an empty one
> > +             # and disable compression first (F2FS won't allow resetting the
> > +             # compression flag if the file has data already in it).
> > +             touch $dst
> > +             if lsattr $dst | grep -qE ".+c.+ $dst" ; then
> > +                     chattr -c $dst
> > +             fi
> >               cp $src $dst
> >               inode=$(stat -c %i $dst)
> >               blocklist=$(_get_ciphertext_block_list $dst)
>
> Is adding 'm' (FS_NOCOMP_FL) needed too?  If not, why does it exist?

In my setup files created have FS_COMPR_FL set from the start. Just clearing
FS_COMPR_FL appears to help as now all the tests using
_verify_ciphertext_for_encryption_policy
pass when I run them on f2fs with "-o compress_extension=*".

Do you think we should add 'm' (FS_NOCOMP_FL) as well just in case some other FS
behaves differently? Do you have any ideas on what other FS I should check?

> Also, have you verified that the tests that use this function still pass
> on both ext4 and f2fs?

Yes, there are no regressions on ext4 and f2fs. I checked generic/369,
generic/548, generic/549, generic/550, generic/582, generic/583, generic/584,
generic/592, generic/602, generic/693 and generic/739.

All now pass except generic/369 which is skipped due to no support for
hardware-wrapped inline encryption keys).

Kind regards,
Jan

Re: [PATCH v2 1/1] common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS

[Eric Biggers]

On Tue, Sep 16, 2025 at 02:13:39PM +0200, Jan Prusakowski wrote:
> On Mon, Sep 15, 2025 at 4:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Mon, Sep 15, 2025 at 12:04:51PM +0200, Jan Prusakowski wrote:
> > > common/encrypt: Do not run _verify_ciphertext_for_encryption_policy on compressed FS
> >
> > A better title would be something like
> > "common/encrypt: Explicitly set the test file to uncompressed".
> 
> Agreed, will fix that in v3.
> 
> > > @@ -790,6 +790,13 @@ _do_verify_ciphertext_for_encryption_policy()
> > >       _set_encpolicy $dir $keyspec $set_encpolicy_args -f $policy_flags
> > >       for src in $tmp.testfile_*; do
> > >               dst=$dir/${src##*.}
> > > +             # To make sure the test file is not compressed we create an empty one
> > > +             # and disable compression first (F2FS won't allow resetting the
> > > +             # compression flag if the file has data already in it).
> > > +             touch $dst
> > > +             if lsattr $dst | grep -qE ".+c.+ $dst" ; then
> > > +                     chattr -c $dst
> > > +             fi
> > >               cp $src $dst
> > >               inode=$(stat -c %i $dst)
> > >               blocklist=$(_get_ciphertext_block_list $dst)
> >
> > Is adding 'm' (FS_NOCOMP_FL) needed too?  If not, why does it exist?
> 
> In my setup files created have FS_COMPR_FL set from the start. Just clearing
> FS_COMPR_FL appears to help as now all the tests using
> _verify_ciphertext_for_encryption_policy
> pass when I run them on f2fs with "-o compress_extension=*".
> 
> Do you think we should add 'm' (FS_NOCOMP_FL) as well just in case some other FS
> behaves differently? Do you have any ideas on what other FS I should check?

I think so.  It doesn't look like f2fs does anything with FS_NOCOMP_FL,
other than enforce that it's mutually exclusive with FS_COMPR_FL.  But
there could be filesystems where 0 gives the default behavior (which
could be compress) and FS_NOCOMP_FL is needed to disable compression.

btrfs might do that, actually.  It doesn't support encryption yet,
though, so these tests can't be run on btrfs.

I would just add FS_NOCOMP_FL and make sure it still works on f2fs.

- Eric