I lost all my original comments on this patch due to a computer crash, so apologies if this sounds a bit rushed. (I should know better than to run the latest mainline kernel. Would be nice if kernel developers focused on quality over new features...)
> +/* > + * 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. > + * > + * 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. > + */ Above comment should document that this is used only when the filesystem uses per-extent encryption > +/** > + * 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 > + * @first_lblk: the first file logical block number in the I/O first_lblk probably should be 'pos' to match Christoph's pending patches (https://lore.kernel.org/linux-fscrypt/[email protected]). Either way, it also needs to be correctly documented to be an offset into the extent, not the file. > + * If the contents of the file should be encrypted (or decrypted) with inline > + * encryption, then assign the appropriate encryption context to the bio. Above comment was copy-pasted and is misleading in its new context. This function assigns the encryption context unconditionally. > + * Normally the bio should be newly allocated (i.e. no pages added yet), as > + * otherwise fscrypt_mergeable_bio() won't work as intended. Likewise, copy-pasted comment that is misleading in the new context. It should refer to fscrypt_mergeable_extent_bio(). > +void fscrypt_set_bio_crypt_ctx_from_extent(struct bio *bio, > + const struct fscrypt_extent_info *ei, > + u64 first_lblk, gfp_t gfp_mask) > +{ > + u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { first_lblk }; Above needs to calculate the DUN correctly when the data unit size is less than the file logical block size, or else the combination of sub-block data units and per-extent encryption needs to be explicitly not supported. Probably just the latter for now (it can be enforced by fscrypt_supported_v2_policy()). > +/** > + * 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 > + * @next_lblk: the next file logical block number in the I/O > + * > + * When building a bio which may contain data which should undergo inline > + * encryption (or decryption) via fscrypt, filesystems should call this > function > + * to ensure that the resulting bio contains only contiguous data unit > numbers. > + * This will return false if the next part of the I/O cannot be merged with > the > + * bio because either the encryption key would be different or the encryption > + * data unit numbers would be discontiguous. > + * > + * fscrypt_set_bio_crypt_ctx_from_extent() must have already been called on > the > + * bio. > + * > + * This function isn't required in cases where crypto-mergeability is > ensured in > + * another way, such as I/O targeting only a single file (and thus a single > key) > + * combined with fscrypt_limit_io_blocks() to ensure DUN contiguity. > + * > + * Return: true iff the I/O is mergeable > + */ > +bool fscrypt_mergeable_extent_bio(struct bio *bio, > + const struct fscrypt_extent_info *ei, > + u64 next_lblk) > +{ > + const struct bio_crypt_ctx *bc = bio->bi_crypt_context; > + u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { next_lblk }; > + > + if (!ei) > + return true; > + if (!bc) > + return true; > + > + /* > + * Comparing the key pointers is good enough, as all I/O for each key > + * uses the same pointer. I.e., there's currently no need to support > + * merging requests where the keys are the same but the pointers differ. > + */ > + if (bc->bc_key != ei->prep_key.blk_key) > + return false; > + > + return bio_crypt_dun_is_contiguous(bc, bio->bi_iter.bi_size, next_dun); > +} > +EXPORT_SYMBOL_GPL(fscrypt_mergeable_extent_bio); Similar to fscrypt_set_bio_crypt_ctx_from_extent(). The copy-pasted comment needs to be updated to remove no-longer-relevant information specific to per-file encryption and correctly reflect per-extent encryption. The DUN needs to be calculated correctly for sub-block data units or else the combination of the two needs to be unsupported. > +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 err; > + > + ci = *fscrypt_inode_info_addr(inode); > + mk = ci->ci_master_key; > + if (WARN_ON_ONCE(!mk)) > + return ERR_PTR(-ENOKEY); > + > + ei = kmem_cache_zalloc(fscrypt_extent_info_cachep, GFP_KERNEL); > + if (!ei) > + return ERR_PTR(-ENOMEM); > + > + refcount_set(&ei->refs, 1); > + memcpy(ei->nonce, nonce, FSCRYPT_FILE_NONCE_SIZE); > + ei->sb = inode->i_sb; > + > + down_read(&mk->mk_sem); > + /* > + * We specifically don't check ->mk_present here because if the inode is > + * open and has a reference on the master key then it should be > + * available for us to use. > + */ Above comment should be reworded to clarify that it is expected for ->mk_present to be either true or false here. As-is, it can be interpreted as meaning that checking ->mk_present is unnecessary because it is guaranteed to be true. The comment above struct fscrypt_master_key (which documents the different states the master key can be in) also needs to be updated to document that with filesystems that use per-extent encryption, ->mk_secret isn't wiped when the key is in the incompletely-removed state (and why that needs to be the case). > +/** > + * fscrypt_prepare_new_extent() - prepare to create a new extent for a file > + * @inode: the possibly-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 doesn't persist the new extents encryption context, this is done > later > + * by calling fscrypt_set_extent_context(). > + * > + * Return: The newly allocated fscrypt_extent_info on success, -EOPNOTSUPP if > + * we're not encrypted, or another -errno code > + */ > +struct fscrypt_extent_info *fscrypt_prepare_new_extent(struct inode *inode) > +{ > + u8 nonce[FSCRYPT_FILE_NONCE_SIZE]; > + > + 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); > + > + get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE); > + return setup_extent_info(inode, nonce); > +} > +EXPORT_SYMBOL_GPL(fscrypt_prepare_new_extent); Similarly, there seems to have been a lot of incorrect copy+pasting in the function comment. This new function requires that the caller *must* provide an encrypted inode, otherwise it WARNs. It can't be "possibly-encrypted". > +/** > + * 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 > + */ > +struct fscrypt_extent_info *fscrypt_load_extent_info(struct inode *inode, > + u8 *ctx, size_t ctx_size) ctx should have type 'const u8 *' > +/** > + * fscrypt_set_extent_context() - Set the fscrypt extent context of a new > extent > + * @inode: the inode this extent belongs to > + * @ei: the fscrypt_extent_info for the given extent > + * @buf: the buffer to copy the fscrypt extent context into > + * > + * This should be called after fscrypt_prepare_new_extent(), using the > + * fscrypt_extent_info that was created at that point. > + * > + * buf must be at most FSCRYPT_SET_CONTEXT_MAX_SIZE. > + * > + * Return: the size of the fscrypt_extent_context, errno if the inode has the > + * wrong policy version. > + */ > +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); > + > + BUILD_BUG_ON(sizeof(struct fscrypt_extent_context) > > + FSCRYPT_SET_CONTEXT_MAX_SIZE); > + > + if (WARN_ON_ONCE(ci->ci_policy.version != 2)) > + return -EINVAL; > + > + ctx->version = FSCRYPT_EXTENT_CONTEXT_V1; > + ctx->encryption_mode = ci->ci_policy.v2.contents_encryption_mode; > + memcpy(ctx->master_key_identifier, > + ci->ci_policy.v2.master_key_identifier, > + sizeof(ctx->master_key_identifier)); > + memcpy(ctx->nonce, ei->nonce, FSCRYPT_FILE_NONCE_SIZE); > + return sizeof(struct fscrypt_extent_context); > +} > +EXPORT_SYMBOL_GPL(fscrypt_context_for_new_extent); The documentation "buf must be at most FSCRYPT_SET_CONTEXT_MAX_SIZE" is incorrect. It must actually be *at least* the size of 'struct fscrypt_extent_context'. Given that it's a fixed size, it probably would make sense to make the ouptut parameter reflect that: 'u8 out[FSCRYPT_EXTENT_CONTEXT_SIZE]'. Or even just use the struct itself. > + /* > + * If set then extent based encryption will be used for this file > + * system, and fs/crypto/ will enforce limits on the policies that are > + * allowed to be chosen. Currently this means only plain v2 policies > + * are supported. > + */ > + unsigned int has_per_extent_encryption : 1; Needs clarification about what is meant by "plain". Some flags are supported (specifically the filename padding ones), some flags are not. All encryption modes still seem to be supported. > + if (count > 0 && inode->i_sb->s_cop->has_per_extent_encryption) { > + fscrypt_warn(inode, > + "Encryption flags aren't supported on file systems > that use extent encryption"); > + return false; > + } Similarly, this error message needs clarification. Some encryption flags are supported, some aren't. - Eric
