| Message ID | 20220501051928.540278-2-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update test_dummy_encryption testing in ext4/053 | expand |
On Sat, Apr 30, 2022 at 10:19:27PM -0700, Eric Biggers wrote: > > The kernel patch "ext4: only allow test_dummy_encryption when supported" > will tighten the requirements on when the test_dummy_encryption mount > option will be accepted. Update ext4/053 accordingly. One of the problems with ext4/053 is that it is very implementation dependent. It was useful when we were making the change to the new mount API, but the problem is any future changes to the mount option handling is going to break the patch. So for example, the kernel patch which Eric has proposed, "ext4: only allow test_dummy_encryption when supported", breaks ext4/053, which I noted in the review the patch. But then this patch will break kernels as they currently stand without this patch, and for kernels that haven't moved to the new mount API, fixing it is going to be a mess. Perhaps ext4/053 is still useful in that it will flag changes that might unintentionally make user-visible changes mount options handling in ext4, but for cases like this one, where we are changing a mount option which is really intended for kernel developers, perhaps the right approach here is to just remove the parts of ext4/053 that are testing the behaviour of test_dummy_encryption in such a super-nit-picky way? What do folks think? > Move the test cases to later in the file to group them with the other > test cases that use do_mkfs to add custom mkfs options instead of using > the "default" filesystem that the test creates at the beginning. Note: this patch doesn't apply because ext4/053 currently has this line: not_mnt test_dummy_encryption=v3 and the patch is trying to remove this line in the first patch chunk: mnt test_dummy_encryption=v3 ^test_dummy_encryption=v3 I checked the upstream version of ext4/053 just in case I had some local modification of ext4/053 in my tree via "git diff -r origin/master tests/ext4/053" but that returned no deltas. Eric, do you have a local modification to this test in your tree already, perhaps? - Ted
On Mon, May 02, 2022 at 05:46:11AM -0700, tytso wrote: > On Sat, Apr 30, 2022 at 10:19:27PM -0700, Eric Biggers wrote: > > > > The kernel patch "ext4: only allow test_dummy_encryption when supported" > > will tighten the requirements on when the test_dummy_encryption mount > > option will be accepted. Update ext4/053 accordingly. > > One of the problems with ext4/053 is that it is very implementation > dependent. It was useful when we were making the change to the new > mount API, but the problem is any future changes to the mount option > handling is going to break the patch. > > So for example, the kernel patch which Eric has proposed, "ext4: only > allow test_dummy_encryption when supported", breaks ext4/053, which I > noted in the review the patch. But then this patch will break kernels > as they currently stand without this patch, and for kernels that > haven't moved to the new mount API, fixing it is going to be a mess. > > Perhaps ext4/053 is still useful in that it will flag changes that > might unintentionally make user-visible changes mount options handling > in ext4, but for cases like this one, where we are changing a mount > option which is really intended for kernel developers, perhaps the > right approach here is to just remove the parts of ext4/053 that are > testing the behaviour of test_dummy_encryption in such a > super-nit-picky way? > > What do folks think? I'd like to keep the test_dummy_encryption test cases. Trying to add a couple new test cases (patch 2) actually found a regression. We could gate them on the kernel version, similar to the whole ext4/053 which already only runs on kernel version 5.12. (Kernel versions checks suck, but maybe it's the right choice for this very-nit-picky test.) Alternatively, I could just backport "ext4: only allow test_dummy_encryption when supported" to 5.15, which would be the only relevant LTS kernel version. > > > Move the test cases to later in the file to group them with the other > > test cases that use do_mkfs to add custom mkfs options instead of using > > the "default" filesystem that the test creates at the beginning. > > Note: this patch doesn't apply because ext4/053 currently has this > line: > > not_mnt test_dummy_encryption=v3 > > and the patch is trying to remove this line in the first patch chunk: > > mnt test_dummy_encryption=v3 ^test_dummy_encryption=v3 > > I checked the upstream version of ext4/053 just in case I had some > local modification of ext4/053 in my tree via "git diff -r > origin/master tests/ext4/053" but that returned no deltas. > > Eric, do you have a local modification to this test in your tree > already, perhaps? Sorry about that; as I mentioned in the cover letter, this is based on my other patch "ext4/053: fix the rejected mount option testing". As-is, 'not_mnt' doesn't really work at all, so I wanted to fix that first. - Eric
On Mon, May 02, 2022 at 10:19:10AM -0700, Eric Biggers wrote: > > We could gate them on the kernel version, similar to the whole ext4/053 which > already only runs on kernel version 5.12. (Kernel versions checks suck, but > maybe it's the right choice for this very-nit-picky test.) Alternatively, I > could just backport "ext4: only allow test_dummy_encryption when supported" to > 5.15, which would be the only relevant LTS kernel version. If we don't need the "only allow test_dummy_encryption when supported" in any Android, Distro, or LTS kernel --- which seems to be a reasonable assumption, that seems to be OK. Lukas, do you agree? In the long term I suspect there will be times when we want to backport mount option handling changes to older kernels, and we're going to be hit this issue again, but as the saying goes, "sufficient unto the day is the evil thereof". - Ted
On Tue, May 10, 2022 at 10:53:35AM -0400, Theodore Ts'o wrote: > On Mon, May 02, 2022 at 10:19:10AM -0700, Eric Biggers wrote: > > > > We could gate them on the kernel version, similar to the whole ext4/053 which > > already only runs on kernel version 5.12. (Kernel versions checks suck, but > > maybe it's the right choice for this very-nit-picky test.) Alternatively, I > > could just backport "ext4: only allow test_dummy_encryption when supported" to > > 5.15, which would be the only relevant LTS kernel version. > > If we don't need the "only allow test_dummy_encryption when supported" > in any Android, Distro, or LTS kernel --- which seems to be a > reasonable assumption, that seems to be OK. Lukas, do you agree? Yes I think this a reasonable approach. > > In the long term I suspect there will be times when we want to > backport mount option handling changes to older kernels, and we're > going to be hit this issue again, but as the saying goes, "sufficient > unto the day is the evil thereof". That is true and while it is a bit annoying to deal with I think that we generally have to keep the user facing mount option behaviour stable. The 053 test is helping with that for the price of some nuisance when we actually want to change the behaviour. For now I think it's a worthwhile trade-off. -Lukas > > - Ted > >
diff --git a/tests/ext4/053 b/tests/ext4/053 index bf6e3f6b..84f3eab9 100755 --- a/tests/ext4/053 +++ b/tests/ext4/053 @@ -512,20 +512,6 @@ for fstype in ext2 ext3 ext4; do mnt noinit_itable mnt max_dir_size_kb=4096 - if _has_kernel_config CONFIG_FS_ENCRYPTION; then - mnt test_dummy_encryption - mnt test_dummy_encryption=v1 - mnt test_dummy_encryption=v2 - not_mnt test_dummy_encryption=v3 - not_mnt test_dummy_encryption= - else - mnt test_dummy_encryption ^test_dummy_encryption - mnt test_dummy_encryption=v1 ^test_dummy_encryption=v1 - mnt test_dummy_encryption=v2 ^test_dummy_encryption=v2 - mnt test_dummy_encryption=v3 ^test_dummy_encryption=v3 - not_mnt test_dummy_encryption= - fi - if _has_kernel_config CONFIG_FS_ENCRYPTION_INLINE_CRYPT; then mnt inlinecrypt else @@ -687,6 +673,27 @@ for fstype in ext2 ext3 ext4; do mnt_then_not_remount defaults jqfmt=vfsv1 remount defaults grpjquota=,usrjquota= ignored + echo "== Testing the test_dummy_encryption option" >> $seqres.full + # Since kernel commit "ext4: only allow test_dummy_encryption when + # supported", the test_dummy_encryption mount option is only allowed + # when the filesystem has the encrypt feature and the kernel has + # CONFIG_FS_ENCRYPTION. Note, the encrypt feature requirement implies + # that this option is never allowed on ext2 or ext3 mounts. + if [[ $fstype == ext4 ]] && _has_kernel_config CONFIG_FS_ENCRYPTION; then + do_mkfs -O encrypt $SCRATCH_DEV ${SIZE}k + mnt test_dummy_encryption + mnt test_dummy_encryption=v1 + mnt test_dummy_encryption=v2 + not_mnt test_dummy_encryption=bad + not_mnt test_dummy_encryption= + do_mkfs -O ^encrypt $SCRATCH_DEV ${SIZE}k + fi + not_mnt test_dummy_encryption + not_mnt test_dummy_encryption=v1 + not_mnt test_dummy_encryption=v2 + not_mnt test_dummy_encryption=bad + not_mnt test_dummy_encryption= + done #for fstype in ext2 ext3 ext4; do $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1