Re: [PATCH v5 7/9] fscrypt: add inline encryption support

[Christoph Hellwig <hch@infradead.org>]

On Thu, Oct 31, 2019 at 01:21:26PM -0700, Eric Biggers wrote:
> > > +	/* The file must need contents encryption, not filenames encryption */
> > > +	if (!S_ISREG(inode->i_mode))
> > > +		return false;
> > 
> > But that isn't really what the check checks for..
> 
> This is how fscrypt has always worked.  The type of encryption to do is
> determined as follows:
> 
> S_ISREG() => contents encryption
> S_ISDIR() || S_ISLNK() => filenames encryption
> 
> See e.g. select_encryption_mode(), and similar checks elsewhere in
> fs/{crypto,ext4,f2fs}/.
> 
> Do you have a suggestion to make it clearer?

Maybe have a fscrypt_content_encryption helper that currently
evaluates to S_ISREG(inode->i_mode) as that documents the intent?

> > > +	/* The filesystem must be mounted with -o inlinecrypt */
> > > +	if (!sb->s_cop->inline_crypt_enabled ||
> > > +	    !sb->s_cop->inline_crypt_enabled(sb))
> > > +		return false;
> > 
> > So please add a SB_* flag for that option instead of the weird
> > indirection.
> 
> IMO it's not really "weird" given that the point of the fscrypt_operations is to
> expose filesystem-specific stuff to fs/crypto/.  But yes, using one of the SB_*
> bits would make it simpler, so if people are fine with that we'll do that.

I think that is much simpler.  Additionally it could also replace the
need for the has_stable_inodes and get_ino_and_lblk_bits methods, which
are pretty weird.  Instead just document the requirements for the
SB_INLINE_CRYPT flag and handle the rest in the file system.  That is
for f2f always set it, and for ext4 set it based on s_inodes_count.
Which brings me to:

> > 
> > Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> > That really should be a parameter, not hardcoded.
> 
> To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, but
> rather in what the blk-crypto API provides.  If blk-crypto were to provide
> longer IV support, fs/crypto/ would pretty easily be able to make use of it.

That's what I meant - we hardcode the value in fscrypt.  Instead we need
to expose the size from blk-crypt and check for it.

> 
> (And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and
> Adiantum support to blk-crypto.c, then inline encryption would be able to do
> everything that the existing filesystem-layer contents encryption can do.)
> 
> Do you have anything in mind for how to make the API support longer IVs in a
> clean way?  Are you thinking of something like:
> 
> 	#define BLK_CRYPTO_MAX_DUN_SIZE	24
> 
> 	u8 dun[BLK_CRYPTO_MAX_DUN_SIZE];
> 	int dun_size;
> 
> We do have to perform arithmetic operations on it, so a byte array would make it
> ugly and slower, but it should be possible...

Well, we could make it an array of u64s, which means we can do all the
useful arithmetics on components on one of them.  But I see the point,
this adds significant complexity for no real short term gain, and we
should probably postponed it until needed.  Maybe just document the
assumptions a little better.