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

On Wed, 12 Aug 2026 at 12:34, Daniel Vacek <[email protected]> wrote:
> On Tue, 2 Jun 2026 at 00:44, Eric Biggers <[email protected]> wrote:
> > 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.
>
> Yeah, that makes sense. I believe this is what we want - any file is
> unlocked by one key. Not that you would need multiple keys for
> unlocking different parts of a file.
>
> > > +/*
> > > + * 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."
>
> Right, this sounds better.
>
> > > + * 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.
>
> Do we actually want this freedom or should we drop the encryption_mode
> and master_key_identifier and consider them implicit from
> inode/fscrypt_context?
> That would mean storing only the nonce in the extent context. The
> version field still remains, preserving the flexibility to eventually
> add them back later if we decide to.
>
> > > +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.
>
> This actually sounds more confusing to me. They are both the same
> thing. My understanding was that extent is "kind of file" on it's own.
> If anything I'd argue for FSCRYPT_NONCE_SIZE used in both places. But
> a FSCRYPT_FILE_NONCE_SIZE used in inode context as well as in extent
> context sounds fine to me.
>
> > > +/**
> > > + * 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
>
> True, the @ei is optional. I'll fix that.
>
> > > + * 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.
>
> OK.
>
> > > +/**
> > > + * 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
>
> True.
>
> > > + * @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)
>
> Ack.
>
> > > +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()
>
> That would add a useless VFS_WARN_ON_ONCE() which will never trigger;
> otherwise, the next line would crash.
> But I guess that's OK.
>
> > > +/**
> > > + * 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.
>
> OK.
>
> > > * 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
>
> That should be fscrypt_context_for_new_extent(). I see it was renamed
> in v5 but this one (and the one below) was forgotten.
> Will fix.
>
> > > +     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.
>
> setup_extent_info() dereferences ci right away. Without this check it
> would crash if the inode was not encrypted.
>
> > ... 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.
>
> This may be better. The comment says this is only meant to be used
> with inline crypto but that actually looks fishy. We ensure this in
> btrfs but fscrypt itself does not need to.
>
> > 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.
>
> I think it makes sense to check before consuming entropy.
> setup_extent_info() can have a second check if needed, though perhaps
> it was not needed so far?
>
> > > +/**
> > > + * 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.)
>
> Honestly, I have to admit I was puzzled by Sashiko's explanation and I
> did not trust it. It sounded rather off.
>
> Trying to dig up the origin of that memalloc_nofs_save() - it is there
> since the very beginning of Josef's [v1] posting. This was around the
> time of Linux v6.6 but the code is still similar.
> So far I don't see if it is really needed or why it was implemented
> this way in the first place. It is called from
> btrfs_do_readpage()->btrfs_get_extent() to set up the key context.
> IIUC, this should not be called from a reclaim path. Hence, I think
> that that memalloc_nofs_save() is redundant and it should be OK to
> simply remove it.

And removing it and testing with stress-ng induced memory pressure
seems to confirm the memalloc_nofs_save() is not needed in this code
path.

@Josef: By any chance, do you recall why this was originally put in?

--nX

> [v1] https://lore.kernel.org/linux-btrfs/de7dec18d4dd440ebf3c538af5c765d747e1d3ef.1695750478.git.josef@toxicpanda.com/
>
> > > +     const struct fscrypt_inode_info *ci = *fscrypt_inode_info_addr(inode);
> >
> > fscrypt_get_inode_info_raw(inode)
>
> OK.
>
> > > +/**
> > > + * 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.
>
> As mentioned above, I see V4 -> v5 rename. No semantics changes.
>
> > > + * @inode: the inode this extent belongs to
> >
> > The inode that the extent will initially belong to, I guess?
>
> Yes, at this point it's a new extent for this inode.
>
> > > +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)
>
> Again, adds an useless WARN_ON_ONCE, but won't hurt.
>
> Thanks!
>
> --nX
>
> > - Eric