| Message ID | 20210621164851.1808923-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4/048: skip test of filename wipe if journal checkpoint is not supported | expand |
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
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 >
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
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 > >
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
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
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>
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
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
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.
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
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(+)