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.jo...@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
