On Wed, May 13, 2026 at 10:52:35AM +0200, Daniel Vacek wrote:
> From: Josef Bacik <[email protected]>
>
> 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
