Re: [PATCH v4 02/46] fscrypt: add per-extent encryption support

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Dec 05, 2023 at 05:48:09PM -0500, Josef Bacik wrote:
> > Also, I think that for now we should keep things simple by doing the following
> > instead of fscrypt_generate_dun():
> > 
> > 	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { first_lblk };
> > 
> > Note that these two changes would eliminate the need for the inode parameter to
> > the function.
> > 
> > Please also make sure to update the function comment.  Note that the first line
> > gives the function name incorrectly.
> > 
> 
> First off thanks for the review, I'm currently going through it one by one, so
> I've come to this bit and I've got a question.
> 
> The DUN has to be le64, so this strictly isn't ok.  So would you be ok with
> 
> __le64 first_lblk_le = le64_to_cpu(first_lblk;
> u64 dun [BLK_CRYPTO_DUN_ARRAY_SIZE] = { first_lblk_le };
> 
> or do you want something different?  Additionally fscrypt_generate_dun() also
> takes into account the data units per block bits, which as I'm typing this out
> I'm realizing doesn't really matter for us even if we have different sectorsize
> per pagesize.  I guess I'll put a big comment about why we're not using that in
> there to make sure future Josef isn't confused.

The blk-crypto DUN is CPU endian, as indicated by the type (array of u64).  Note
that fscrypt_generate_dun() converts u64 => __le64 => u64.  (And then
blk-crypto-fallback converts u64 => __le64.)  Getting rid of that round trip is
nice, which is why I'm recommending it.  The only reason that
fscrypt_generate_dun() does the round trip is because it allows it to reuse
fscrypt_generate_iv(), and I wanted to be 100% sure that all of the IV methods
were implemented the same as in FS layer encryption.  With the extent-based
encryption we're just supporting one method, so things are simpler.

Yeah, it should be 'first_lblk << ci_data_units_per_block_bits'.
That would just be for future-proofing, since btrfs isn't setting
supports_subblock_data_units anyway.

> > > +/**
> > > + * fscrypt_extent_context_size() - Return the size of the on-disk extent context
> > > + * @inode: the inode this extent belongs to.
> > > + *
> > > + * Return the size of the extent context for this inode.  Since there is only
> > > + * one extent context version currently this is just the size of the extent
> > > + * context if the inode is encrypted.
> > > + */
> > > +size_t fscrypt_extent_context_size(struct inode *inode)
> > > +{
> > > +	if (!IS_ENCRYPTED(inode))
> > > +		return 0;
> > > +
> > > +	return sizeof(struct fscrypt_extent_context);
> > > +}
> > > +EXPORT_SYMBOL_GPL(fscrypt_extent_context_size);
> > 
> > Huh, shouldn't the filesystem use the size from fscrypt_set_extent_context() (or
> > fscrypt_context_for_new_extent() as I recommend calling it)?  Why is the above
> > function necessary?
> 
> This is because we need to know how big the context is ahead of time before
> searching down the b-tree to insert the file extent item, and we have to add
> this size to the file extent item ahead of time.  The disconnect between when we
> need to know how big it'll be and when we actually generate the context to save
> on disk is the reason for there being two helpers.
> 
> The main reason for this is code separation.  I would have to have the context
> on stack for the !fscrypt case if I were to only have one helper, because I
> would need it there to generate the extent context to save on disk to pass it
> around to all the things that add file extents, and I could use the length from
> that.  Not a huge deal, but this way is a little cleaner.  You can see how I've
> used the two helpers in later patches, but the general flow is
> 
> total_size = sizeof(our file extent);
> total_size += fscrypt_extent_context_size(inode);
> 
> btrfs_insert_empty_item(path, total_size);
> 
> btrfs_fscrypt_save_extent_info(path, inode);
> 
> and then in btrfs_fscrypt_save_extent_info() we do
> 
> u8 ctx[BTRFS_MAX_EXTENT_CTX_SIZE];
> 
> size = fscrypt_set_extent_context(inode, ctx);
> 
> Now the above case does seem like we could just uconditionally do something like
> 
> u8 ctx[BTRFS_MAX_EXTENT_CTX_SIZE];
> 
> total_size = sizeof(our file extent);
> ctx_size = fscrypt_set_extent_context(inode, ctx);
> if (ctx_size < 0)
> 	goto error;
> total_size += ctx_size;
> btrfs_insert_empty_item(path, total_size);
> copy_ctx_into_file_extent(path, ctx);
> 
> But there's some other file extent manipulation cases that aren't this clear
> cut, so I'd have to pass this ctx around to be copied in.  And in the !encrypted
> part we're paying a higher stack memory cost.
> 
> If you still don't like this I can rework it, but I wanted to explain my
> reasoning before I went changing a bunch of stuff to make the new paradigm fit.
> 
> The rest of the comments make sense, I'll fix everything up.  Thanks,

Maybe fscrypt_set_extent_context() (or fscrypt_context_for_new_extent()?) should
return the size if it's passed a NULL pointer?  It just seems error-prone have
the size of the thing being generated be returned by a different function.

- Eric