[PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Theodore Ts'o]

ext4/048 will fail when running on older kernels that don't support
the filename wipe feature.  The journal checkpoint ioctl is a related
feature, and landed just a little bit after filename wipe feature, so
use support for the journal checkpoint ioctl as a proxy for support
for the filename wipe feature.

Without this change, this test will fail when tesing 5.10, 5.4, and
other LTS kernels.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: Leah Rumancik <leah.rumancik@gmail.com>
---
 tests/ext4/048 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/ext4/048 b/tests/ext4/048
index 51189618..35e6aa7f 100755
--- a/tests/ext4/048
+++ b/tests/ext4/048
@@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
 # create scratch dir for testing
 # create some files with no name a substr of another name so we can grep later
 _scratch_mount >> $seqres.full 2>&1
+
+# Use the presence of the journal checkpoint ioctl as a proxy of filename
+# wipe being supported
+if test -x $here/src/checkpoint_journal && \
+	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
+    _notrun "filename wipe not supported"
+fi
+
 blocksize="$(_get_block_size $SCRATCH_MNT)"
 mkdir $testdir
 file_num=1
-- 
2.31.0

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[xuyang2018.jy]

on 2021/6/22 0:48, Theodore Ts'o wrote:
> ext4/048 will fail when running on older kernels that don't support
> the filename wipe feature.  The journal checkpoint ioctl is a related
> feature, and landed just a little bit after filename wipe feature, so
> use support for the journal checkpoint ioctl as a proxy for support
> for the filename wipe feature.
> 
> Without this change, this test will fail when tesing 5.10, 5.4, and
> other LTS kernels.
Thanks. With this patch, it fix failure on centos7.9 and centos8.4. But
I can't find kernel commit for filename wipe feature and journal
checkpoint ioctl. Can you provide them in commit message?

Best Regards
Yang Xu
> 
> Signed-off-by: Theodore Ts'o<tytso@mit.edu>
> Cc: Leah Rumancik<leah.rumancik@gmail.com>
> ---
>   tests/ext4/048 | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/tests/ext4/048 b/tests/ext4/048
> index 51189618..35e6aa7f 100755
> --- a/tests/ext4/048
> +++ b/tests/ext4/048
> @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024))>>  $seqres.full 2>&1
>   # create scratch dir for testing
>   # create some files with no name a substr of another name so we can grep later
>   _scratch_mount>>  $seqres.full 2>&1
> +
> +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> +# wipe being supported
> +if test -x $here/src/checkpoint_journal&&  \
> +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> +    _notrun "filename wipe not supported"
> +fi
> +
>   blocksize="$(_get_block_size $SCRATCH_MNT)"
>   mkdir $testdir
>   file_num=1

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Leah Rumancik]

On Mon, Jun 21, 2021 at 12:48:51PM -0400, Theodore Ts'o wrote:
> ext4/048 will fail when running on older kernels that don't support
> the filename wipe feature.  The journal checkpoint ioctl is a related
> feature, and landed just a little bit after filename wipe feature, so
> use support for the journal checkpoint ioctl as a proxy for support
> for the filename wipe feature.
> 
> Without this change, this test will fail when tesing 5.10, 5.4, and
> other LTS kernels.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: Leah Rumancik <leah.rumancik@gmail.com>
> ---
>  tests/ext4/048 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/ext4/048 b/tests/ext4/048
> index 51189618..35e6aa7f 100755
> --- a/tests/ext4/048
> +++ b/tests/ext4/048
> @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
>  # create scratch dir for testing
>  # create some files with no name a substr of another name so we can grep later
>  _scratch_mount >> $seqres.full 2>&1
> +
> +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> +# wipe being supported
> +if test -x $here/src/checkpoint_journal && \
> +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> +    _notrun "filename wipe not supported"
> +fi

What if checkpoint_journal is not there? Should the test be
skipped in that case as well? 

-Leah

> +
>  blocksize="$(_get_block_size $SCRATCH_MNT)"
>  mkdir $testdir
>  file_num=1
> -- 
> 2.31.0
> 

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Theodore Ts'o]

On Tue, Jun 22, 2021 at 11:18:45AM -0700, Leah Rumancik wrote:
> > diff --git a/tests/ext4/048 b/tests/ext4/048
> > index 51189618..35e6aa7f 100755
> > --- a/tests/ext4/048
> > +++ b/tests/ext4/048
> > @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
> >  # create scratch dir for testing
> >  # create some files with no name a substr of another name so we can grep later
> >  _scratch_mount >> $seqres.full 2>&1
> > +
> > +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> > +# wipe being supported
> > +if test -x $here/src/checkpoint_journal && \
> > +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> > +    _notrun "filename wipe not supported"
> > +fi
> 
> What if checkpoint_journal is not there? Should the test be
> skipped in that case as well?

I went back and forth on that one.  In actual practice
checkpoint_journal should always be built in a valid xfstests
installation, so the case of it not existing should rarely if ever
arise.  We don't actually _need_ checkpoint_journal to run the test;
we're just using it as a hint as to whether the filename wipe feature
is present.  So I decided to let the test run if we couldn't find it,
on the theory that in the long run, all future kernels will have the
feature.   But the case could be made in other direction....

	       	   	      	 - Ted

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[xuyang2018.jy]

on 2021/6/22 9:58, xuyang2018.jy@fujitsu.com wrote:
> on 2021/6/22 0:48, Theodore Ts'o wrote:
>> ext4/048 will fail when running on older kernels that don't support
>> the filename wipe feature.  The journal checkpoint ioctl is a related
>> feature, and landed just a little bit after filename wipe feature, so
>> use support for the journal checkpoint ioctl as a proxy for support
>> for the filename wipe feature.
>>
>> Without this change, this test will fail when tesing 5.10, 5.4, and
>> other LTS kernels.
> Thanks. With this patch, it fix failure on centos7.9 and centos8.4. But
> I can't find kernel commit for filename wipe feature and journal
> checkpoint ioctl. Can you provide them in commit message?

I guess filename wipe feature commit is

[1]https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=6c0912739699d8e4b6a87086401bf3ad3c59502d

and journal checkpoint iocl commit is
[2]https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=339183dfb87ce94f8e14a0db48cae093516e194c

Since the commit[1] is a feature and this case is desinged to test this
feature, so skipping this test looks ok on non-supported kernel.

Tested-by: Yang Xu <xuyang2018.jy@fujitsu.com>

> 
> Best Regards
> Yang Xu
>>
>> Signed-off-by: Theodore Ts'o<tytso@mit.edu>
>> Cc: Leah Rumancik<leah.rumancik@gmail.com>
>> ---
>>    tests/ext4/048 | 8 ++++++++
>>    1 file changed, 8 insertions(+)
>>
>> diff --git a/tests/ext4/048 b/tests/ext4/048
>> index 51189618..35e6aa7f 100755
>> --- a/tests/ext4/048
>> +++ b/tests/ext4/048
>> @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024))>>   $seqres.full 2>&1
>>    # create scratch dir for testing
>>    # create some files with no name a substr of another name so we can grep later
>>    _scratch_mount>>   $seqres.full 2>&1
>> +
>> +# Use the presence of the journal checkpoint ioctl as a proxy of filename
>> +# wipe being supported
>> +if test -x $here/src/checkpoint_journal&&   \
>> +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
>> +    _notrun "filename wipe not supported"
>> +fi
>> +
>>    blocksize="$(_get_block_size $SCRATCH_MNT)"
>>    mkdir $testdir
>>    file_num=1
> 
> 

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Leah Rumancik]

On Tue, Jun 22, 2021 at 10:02:24PM -0400, Theodore Ts'o wrote:
> On Tue, Jun 22, 2021 at 11:18:45AM -0700, Leah Rumancik wrote:
> > > diff --git a/tests/ext4/048 b/tests/ext4/048
> > > index 51189618..35e6aa7f 100755
> > > --- a/tests/ext4/048
> > > +++ b/tests/ext4/048
> > > @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
> > >  # create scratch dir for testing
> > >  # create some files with no name a substr of another name so we can grep later
> > >  _scratch_mount >> $seqres.full 2>&1
> > > +
> > > +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> > > +# wipe being supported
> > > +if test -x $here/src/checkpoint_journal && \
> > > +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> > > +    _notrun "filename wipe not supported"
> > > +fi
> > 
> > What if checkpoint_journal is not there? Should the test be
> > skipped in that case as well?
> 
> I went back and forth on that one.  In actual practice
> checkpoint_journal should always be built in a valid xfstests
> installation, so the case of it not existing should rarely if ever
> arise.  We don't actually _need_ checkpoint_journal to run the test;
> we're just using it as a hint as to whether the filename wipe feature
> is present.  So I decided to let the test run if we couldn't find it,
> on the theory that in the long run, all future kernels will have the
> feature.   But the case could be made in other direction....
> 
> 	       	   	      	 - Ted

Works for me.

-Leah

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Leah Rumancik]

On Tue, Jun 22, 2021 at 10:02:24PM -0400, Theodore Ts'o wrote:
> On Tue, Jun 22, 2021 at 11:18:45AM -0700, Leah Rumancik wrote:
> > > diff --git a/tests/ext4/048 b/tests/ext4/048
> > > index 51189618..35e6aa7f 100755
> > > --- a/tests/ext4/048
> > > +++ b/tests/ext4/048
> > > @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
> > >  # create scratch dir for testing
> > >  # create some files with no name a substr of another name so we can grep later
> > >  _scratch_mount >> $seqres.full 2>&1
> > > +
> > > +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> > > +# wipe being supported
> > > +if test -x $here/src/checkpoint_journal && \
> > > +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> > > +    _notrun "filename wipe not supported"
> > > +fi
> > 
> > What if checkpoint_journal is not there? Should the test be
> > skipped in that case as well?
> 
> I went back and forth on that one.  In actual practice
> checkpoint_journal should always be built in a valid xfstests
> installation, so the case of it not existing should rarely if ever
> arise.  We don't actually _need_ checkpoint_journal to run the test;
> we're just using it as a hint as to whether the filename wipe feature
> is present.  So I decided to let the test run if we couldn't find it,
> on the theory that in the long run, all future kernels will have the
> feature.   But the case could be made in other direction....
> 
> 	       	   	      	 - Ted

Works for me.

-Leah

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Leah Rumancik]

On Tue, Jun 22, 2021 at 10:02:24PM -0400, Theodore Ts'o wrote:
> On Tue, Jun 22, 2021 at 11:18:45AM -0700, Leah Rumancik wrote:
> > > diff --git a/tests/ext4/048 b/tests/ext4/048
> > > index 51189618..35e6aa7f 100755
> > > --- a/tests/ext4/048
> > > +++ b/tests/ext4/048
> > > @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
> > >  # create scratch dir for testing
> > >  # create some files with no name a substr of another name so we can grep later
> > >  _scratch_mount >> $seqres.full 2>&1
> > > +
> > > +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> > > +# wipe being supported
> > > +if test -x $here/src/checkpoint_journal && \
> > > +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> > > +    _notrun "filename wipe not supported"
> > > +fi
> > 
> > What if checkpoint_journal is not there? Should the test be
> > skipped in that case as well?
> 
> I went back and forth on that one.  In actual practice
> checkpoint_journal should always be built in a valid xfstests
> installation, so the case of it not existing should rarely if ever
> arise.  We don't actually _need_ checkpoint_journal to run the test;
> we're just using it as a hint as to whether the filename wipe feature
> is present.  So I decided to let the test run if we couldn't find it,
> on the theory that in the long run, all future kernels will have the
> feature.   But the case could be made in other direction....
> 
> 	       	   	      	 - Ted

Reviewed-by: Leah Rumancik <leah.rumancik@gmail.com>

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Eryu Guan]

On Mon, Jun 21, 2021 at 12:48:51PM -0400, Theodore Ts'o wrote:
> ext4/048 will fail when running on older kernels that don't support
> the filename wipe feature.  The journal checkpoint ioctl is a related

I think that depends on if we treat it as a feature or a bugfix. We
should let the test fail on old kernels if that's a bugfix, and skip the
test if it's a feature. Apparently, we take it as a feature here.

> feature, and landed just a little bit after filename wipe feature, so
> use support for the journal checkpoint ioctl as a proxy for support
> for the filename wipe feature.

This seems fragile, they're related features but not always bounded
together, I think it's possible for distros developers decide to
backport only one of the features (though quite unlikely).

Is it worth to introduce features sysfs entry for journal checkpoint and
filename wipe in ext4? In fstests' point of view, it's great to have
such entries that could be checked. If it's not worth it, I think the
"feature proxy" way is acceptable, given that the two features are
closely related and aimed to the same security issue.

Thanks,
Eryu

> 
> Without this change, this test will fail when tesing 5.10, 5.4, and
> other LTS kernels.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: Leah Rumancik <leah.rumancik@gmail.com>
> ---
>  tests/ext4/048 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/ext4/048 b/tests/ext4/048
> index 51189618..35e6aa7f 100755
> --- a/tests/ext4/048
> +++ b/tests/ext4/048
> @@ -93,6 +93,14 @@ _scratch_mkfs_sized $((128 * 1024 * 1024)) >> $seqres.full 2>&1
>  # create scratch dir for testing
>  # create some files with no name a substr of another name so we can grep later
>  _scratch_mount >> $seqres.full 2>&1
> +
> +# Use the presence of the journal checkpoint ioctl as a proxy of filename
> +# wipe being supported
> +if test -x $here/src/checkpoint_journal && \
> +	! $here/src/checkpoint_journal $SCRATCH_MNT --dry-run ; then
> +    _notrun "filename wipe not supported"
> +fi
> +
>  blocksize="$(_get_block_size $SCRATCH_MNT)"
>  mkdir $testdir
>  file_num=1
> -- 
> 2.31.0

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Theodore Ts'o]

On Sun, Jul 04, 2021 at 06:47:22PM +0800, Eryu Guan wrote:
> 
> I think that depends on if we treat it as a feature or a bugfix. We
> should let the test fail on old kernels if that's a bugfix, and skip the
> test if it's a feature. Apparently, we take it as a feature here.

Yes, it's clearly a feature.

> > feature, and landed just a little bit after filename wipe feature, so
> > use support for the journal checkpoint ioctl as a proxy for support
> > for the filename wipe feature.
> 
> This seems fragile, they're related features but not always bounded
> together, I think it's possible for distros developers decide to
> backport only one of the features (though quite unlikely).

Given the discussion for the patches, I suspect that it will be only
the distros that are meant for Cloud environments (e.g., Google's COS,
maybe Amazon Linux, etc.) that will be interested at all in the first
place --- and if they do, they'll want to take both of them.

That's because it's only in a Cluod environment we can make specific
guarantees about how discard works, and it's mostly in a Cloud
environment where you have customers which appear to be the most
concerned about being able to certify to their auditors that PII can
be reliably wiped after they "lift and shift" from their private data
centers to third-party operated hyperscale Cloud providers.

> Is it worth to introduce features sysfs entry for journal checkpoint and
> filename wipe in ext4? In fstests' point of view, it's great to have
> such entries that could be checked. If it's not worth it, I think the
> "feature proxy" way is acceptable, given that the two features are
> closely related and aimed to the same security issue.

Well, we didn't when the patch first landed, so even if we did add the
features sysfs file, there's no guarantee that distros doing the
backport will pick up the patch to add the features file.  The sysfs
files do take up a few hundred bytes each, and so there is some
resistance in adding a lot of sysfs features, saving them for major
features.

For better or for worse, one of the principles of sysfs is to _not_
require any parsing, such that something like /proc/stat would have to
be replaced by something like individual 1,000+ sysfs files (try
running wc -w /proc/stat), depending on how cpu cores and interrupt
channels the system has.

This is why we are using separate sysfs files for each feature,
instead of a /proc file containing a list of features which fstests
could test by checking the exit status of "grep ^feature_name$
/proc/fs/ext4/feature-list".  But in retrospect, maybe we should have
gone down that path, since adding features to a feature-list
pseudo-file is essentially free, and would make life much more
convenient for fstests --- right now we do a lot of "create a scratch
file system; try to mount it and see if it succeed or fails, which is
a lot of needless church and extra write cycles on the test device,
and takes more time than just running "grep" on a festure-list file.

Alas, sysfs was considered the new hotness, and the superior approach,
while /proc was considered the legacy, hacky approach.  Ah, well...

Cheers,

						- Ted

Re: [PATCH] ext4/048: skip test of filename wipe if journal checkpoint is not supported

[Christoph Hellwig]

On Sun, Jul 04, 2021 at 06:05:30PM -0400, Theodore Ts'o wrote:
> Given the discussion for the patches, I suspect that it will be only
> the distros that are meant for Cloud environments (e.g., Google's COS,
> maybe Amazon Linux, etc.) that will be interested at all in the first
> place --- and if they do, they'll want to take both of them.

FYI, I still think supporting discard at all is a active and dangerous
mis-service to your users.  Discard must not and often will not
clear data.  Write smae OTOH does, and with the REQ_UNMAP flag will
give you the same results as discards for devices that zero on
discard, while haveing a safe (but slow) fallback if not.