Re: [PATCH v7 01/43] fscrypt: add per-extent encryption support

[Eric Biggers <ebiggers@kernel.org>]

On Wed, May 13, 2026 at 10:52:35AM +0200, Daniel Vacek wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> This adds the code necessary for per-extent encryption.  We will store a
> nonce for every extent we create, and then use the inode's policy and
> the extents nonce to derive a per-extent key.
> 
> This is meant to be flexible, if we choose to expand the on-disk extent
> information in the future we have a version number we can use to change
> what exists on disk.
> 
> The file system indicates it wants to use per-extent encryption by
> setting s_cop->has_per_extent_encryption.  This also requires the use of
> inline block encryption.
> 
> The support is relatively straightforward, the only "extra" bit is we're
> deriving a per-extent key to use for the encryption, the inode still
> controls the policy and access to the master key.
> 
> Since extent based encryption uses a lot of keys, we're requiring the
> use of inline block crypto if you use extent-based encryption.  This
> enables us to take advantage of the built in pooling and reclamation of
> the crypto structures that underpin all of the encryption.

The whole reason for extent-based encryption is that extents can be
shared between inodes.  So the repeated mentions of "the inode" are
really confusing.  This shows up in a lot of different places.

What's actually implemented is that each extent stores its own
(encryption_mode, master_key_identifier, nonce), but for now the
invariant is maintained that all inodes that reference an extent share
the same (encryption_mode, master_key_identifier) as the extent.

It would be helpful to document this stuff accordingly.

> +/*
> + * fscrypt_extent_context - the encryption context of an extent
> + *
> + * This is the on-disk information stored for an extent.  The nonce is used as a
> + * KDF input in conjuction with the inode context to derive a per-extent key for
> + * encryption.  This is used only when the filesystem uses per-extent encryption.
> + *

Basically the same issue here.  The master_key_identifier is actually
stored in the extent.  Just the current implementation enforces that
when the filesystem accesses the extent through some inode, that inode
also has the same master_key_identifier.  How about replacing the second
sentence with something like: "The nonce and master_key_identifier are
used to derive the key which encrypts the extent."

> + * With the current implementation, master_key_identifier and encryption mode
> + * must match the inode context.  These are here for future expansion where we
> + * may want the option of mixing different keys and encryption modes for the
> + * same file.
> + */

Likewise.  Something like: With the current implementation,
master_key_identifier and encryption_mode always match the corresponding
values from the fscrypt_context in each inode that shares the extent.

> +struct fscrypt_extent_context {
> +	u8 version; /* FSCRYPT_EXTENT_CONTEXT_V1 */
> +	u8 encryption_mode;
> +	u8 master_key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE];
> +	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
> +};

Well, it's an extent nonce, not a file nonce.  It seems it's handled
completely separately from the existing file nonce, so it probably
should get its own size constant FSCRYPT_EXTENT_NONCE_SIZE.

> +/**
> + * fscrypt_set_bio_crypt_ctx_from_extent() - prepare a file contents bio for
> + *					     inline crypto with extent
> + *					     encryption
> + * @bio: a bio which will eventually be submitted to the file
> + * @ei: the extent's crypto info

@ei: the extent's crypto info, or NULL if the extent is unencrypted

> + * If the contents of the file should be encrypted (or decrypted) with inline
> + * encryption, then assign the appropriate encryption context to the bio.

"If the contents of the file should be encrypted (or decrypted) with
inline encryption" => "If the extent should be encrypted (or decrypted)"

There's no "file" here.  And inline encryption is the only option for
extents.

> +/**
> + * fscrypt_mergeable_extent_bio() - test whether data can be added to a bio
> + * @bio: the bio being built up
> + * @ei: the fscrypt_extent_info for this extent

@ei: the extent's crypto info, or NULL if the extent is unencrypted

> + * @pos: the next extent logical offset (in bytes) in the I/O
> + *
> + * When building a bio which may contain data which should undergo inline
> + * encryption (or decryption) via fscrypt,

When building a bio which may contain data which should undergo extent
encryption (or decryption)

> +static struct fscrypt_extent_info *
> +setup_extent_info(struct inode *inode, const u8 nonce[FSCRYPT_FILE_NONCE_SIZE])
> +{
> +	struct fscrypt_extent_info *ei;
> +	struct fscrypt_inode_info *ci;
> +	struct fscrypt_master_key *mk;
> +	u8 derived_key[FSCRYPT_MAX_RAW_KEY_SIZE];
> +	int keysize;
> +	int err;
> +
> +	ci = *fscrypt_inode_info_addr(inode);

fscrypt_get_inode_info_raw()

> +/**
> + * fscrypt_prepare_new_extent() - prepare to create a new extent for a file
> + * @inode: the encrypted inode
> + *
> + * If the inode is encrypted, setup the fscrypt_extent_info for a new extent.
> +
> + * This will include the nonce and the derived key necessary for the extent to
> + * be encrypted.  This is only meant to be used with inline crypto and on inodes
> + * that need their contents encrypted.

This is ambiguous and contradictory about what type of @inode is
required.  It should be something like:

* @inode: an encrypted regular file with its key already set up, on a
*        filesystem that uses per-extent encryption
*
* Prepare to encrypt a new extent by generating a new extent nonce,
* deriving an extent key, and allocating an fscrypt_extent_info.

> * This doesn't persist the new extents encryption context, this is done later   
> * by calling fscrypt_set_extent_context().

There's no function with that name

> +	if (WARN_ON_ONCE(!*fscrypt_inode_info_addr(inode)))
> +		return ERR_PTR(-EOPNOTSUPP);
> +	if (WARN_ON_ONCE(!fscrypt_inode_uses_inline_crypto(inode)))
> +		return ERR_PTR(-EOPNOTSUPP);

I'm confused what these checks are trying to do.  The first part checks
for the inode's encryption key, but setup_extent_info() does that
anyway.  The second part is maybe intended to check that the file uses
extent encryption, but it doesn't do it correctly.  That would require:
fscrypt_needs_contents_encryption(inode) &&
inode->i_sb->s_cop->has_per_extent_encryption.

It probably would make sense to check that directly in
setup_extent_info(), so that it's closer to the call to
fscrypt_hkdf_expand() which would has a *very* bad failure mode when
!has_per_extent_encryption.

> +/**
> + * fscrypt_load_extent_info() - create an fscrypt_extent_info from the context
> + * @inode: the inode
> + * @ctx: the context buffer
> + * @ctx_size: the size of the context buffer
> + *
> + * Create the fscrypt_extent_info and derive the key based on the
> + * fscrypt_extent_context buffer that is provided.
> + *
> + * Return: The newly allocated fscrypt_extent_info on success, -EOPNOTSUPP if
> + *	   we're not encrypted, or another -errno code
> + */

What context is this expected to be called in?  I see the caller uses
memalloc_nofs_save().  This would require making ->mk_sem nofs-safe; is
there a plan to do that?  (Sashiko noticed this too, by the way.)

> +	const struct fscrypt_inode_info *ci = *fscrypt_inode_info_addr(inode);

fscrypt_get_inode_info_raw(inode)

> +/**
> + * fscrypt_set_extent_context() - Set the fscrypt extent context of a new extent

It seems the function name and semantics changed at some point, but the
kerneldoc wasn't updated.

> + * @inode: the inode this extent belongs to

The inode that the extent will initially belong to, I guess?

> +ssize_t fscrypt_context_for_new_extent(struct inode *inode,
> +				       struct fscrypt_extent_info *ei, u8 *buf)
> +{
> +	struct fscrypt_extent_context *ctx = (struct fscrypt_extent_context *)buf;
> +	const struct fscrypt_inode_info *ci = *fscrypt_inode_info_addr(inode);

fscrypt_get_inode_info_raw(inode)

- Eric