Re: [PATCH 1/4] generic: add utilities for testing filesystem encryption

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 21, 2016 at 10:40:50AM -0800, Eric Biggers wrote:
> Hi Dave, thanks for reviewing.
> 
> On Mon, Nov 21, 2016 at 08:33:13AM +1100, Dave Chinner wrote:
> > > +
> > > +. ./common/rc
> > 
> > Tests will already have included common/rc before this file, so we
> > do not source it here.
> ...
> > These go in the tests, not here.
> ...
> > _requires_real_encryption()
> > 
> > In each test.
> ...
> > And this should all be in a _requires_encryption() function.
> > 
> 
> I'll do all of these.  Of course the intent was to avoid duplicating code in
> each test, but I will use the more verbose style if that's preferred.  I assume
> you'd also prefer explicitly formatting and mounting the scratch device in each
> test even though _require_encryption would already have to do that?

Yes, because in future _require_encryption may change to no require
touching the scratch device. Also, checking for encryption may
create a filesystem with different configuration than the one we
want to test. So it's better to be consistent across all tests and
require the scratch_mkfs call in each test so we know the exact
state of the filesystem before the test starts....

> > Ok, can we get this added to xfs_io rather than a stand-alone
> > fstests helper? There are three clear commands here:
> > 
> > 	{"gen_key", gen_key},
> > 	{"rm_key", rm_key},
> > 	{"set_policy", set_policy},
> > 
> > So it should plug straight into the xfs_io command parsing
> > infrastructure without much change at all.
> 
> I see that xfs_io is part of xfsprogs, not xfstests.  Does it make sense to add
> filesystem encryption commands to xfs_io even though XFS doesn't support them
> yet?  Currently only ext4 and f2fs support filesystem encryption via this common
> API.  (ubifs support has been proposed too.)

Yes, because it is in the plan to support the generic encryption in
XFS, too. It'll just take us a little while to get to it, but it
won't hurt to support these operations ahead of that time...

> If we do go that route then it should be considered only adding
> "set_policy" and "get_policy" commands, and for "gen_key" and
> "rm_key" instead using shell wrappers around 'keyctl' instead.
> gen_key and rm_key don't touch the filesystem at all; they only
> work with the keyring.  It's possible to use 'keyctl padd' to add
> a fscrypt key, though it's not completely trivial because you'd
> have to use 'echo -e' to generate the C structure 'struct
> fscrypt_key' with mode = 0, raw = actual key in binary, size = 64.

Sounds fine to me.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com