| Message ID | fd5c7a78de125737abe447370fe37f9fe90155d6.1672547582.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fscrypt: add per-extent encryption keys | expand |
On Sun, Jan 01, 2023 at 12:06:19AM -0500, Sweet Tea Dorminy wrote: > The other half of using per-extent infos is saving and loading them from > disk. > > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> > --- > fs/crypto/keysetup.c | 50 +++++++++++++++++++++++++++++++++++++++++ > fs/crypto/policy.c | 20 +++++++++++++++++ > include/linux/fscrypt.h | 17 ++++++++++++++ > 3 files changed, 87 insertions(+) > > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c > index 136156487e8f..82439fb73dd9 100644 > --- a/fs/crypto/keysetup.c > +++ b/fs/crypto/keysetup.c > @@ -799,6 +799,56 @@ void fscrypt_free_extent_info(struct fscrypt_info **info_ptr) > } > EXPORT_SYMBOL_GPL(fscrypt_free_extent_info); > > +/** > + * fscrypt_load_extent_info() - set up a preexisting extent's fscrypt_info > + * @inode: the inode to which the extent belongs. Must be encrypted. > + * @buf: a buffer containing the extent's stored context > + * @len: the length of the @ctx buffer > + * @info_ptr: a pointer to return the extent's fscrypt_info into. Should be > + * a pointer to a member of the extent struct, as it will be passed > + * back to the filesystem if key removal demands removal of the > + * info from the extent > + * > + * This is not %GFP_NOFS safe, so the caller is expected to call > + * memalloc_nofs_save/restore() if appropriate. > + * > + * Return: 0 if successful, or -errno if it fails. > + */ > +int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len, > + struct fscrypt_info **info_ptr) > +{ When is the filesystem going to call fscrypt_load_extent_info()? My concern (which we've discussed, but probably I didn't explain clearly enough) is that the two "naive" solutions don't really work: Option 1: Set up during the I/O to the extent. I think this is not feasible because the full fscrypt_setup_encryption_info() is not safe to do doing I/O. For example, it allocates memory under GFP_KERNEL, and it uses crypto_alloc_skcipher() which can involve loading kernel modules. Option 2: Set up for *all* extents when the file is opened. I expect that this would not be feasible either, since a file can have a huge number of extents. This patchset seems to be assuming one of those options. (It's not clear whether it's Option 1 or Option 2, since the caller of fscrypt_load_extent_info() is not included in the patchset.) That leaves the option I suggested, which probably I didn't explain clearly enough: split up key setup so that part can be done when opening the file, and part can be done during I/O. Specifically, when opening the file, preallocate some number of crypto_skcipher objects. This would be limited to a fixed number, like 128, even if the file has thousands of extents. Then, when doing I/O to an extent, temporarily take a crypto_skcipher from the cache, derive the extent's key using fscrypt_hkdf_expand(), and set it in the crypto_skcipher using crypto_skcipher_setkey(). That way, during I/O only fscrypt_hkdf_expand() and crypto_skcipher_setkey() would be needed. Those are fairly safe to call during I/O, in contrast to crypto_alloc_skcipher() which is really problematic to call during I/O. Of course, it will still be somewhat expensive to derive and set a key. So it might also make sense to maintain a map that maps (master key, extent nonce, encryption mode) to the corresponding cached crypto_skcipher, if any, so that an already-prepared one can be used when possible. By the way, blk-crypto-fallback (block/blk-crypto-fallback.c) does something similar, as it had to solve a similar problem. The way it was solved is to require that blk_crypto_fallback_start_using_mode() be called to preallocate the crypto_skcipher objects for a given encryption mode. You actually could just use blk-crypto-fallback to do the en/decryption for you, if you wanted to. I.e., you could use the blk-crypto API for en/decryption, instead of going directly through crypto_skcipher. (Note that currently blk-crypto is opt-in via the "inlinecrypt" mount option; it's not used by default. But it doesn't *have* to be that way; it could just be always used. It would be necessary to ensure that CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK gets selected.) That would save having to reimplement the caching of crypto_skcipher objects. However, key derivation would still need to be done at the filesystem level, so it probably would still make sense to cache derived keys. - Eric
(in which I fail to reply-all the first time) > > When is the filesystem going to call fscrypt_load_extent_info()? > > My concern (which we've discussed, but probably I didn't explain clearly enough) > is that the two "naive" solutions don't really work: > > Option 1: Set up during the I/O to the extent. I think this is not feasible > because the full fscrypt_setup_encryption_info() is not safe to do doing I/O. > For example, it allocates memory under GFP_KERNEL, and it uses > crypto_alloc_skcipher() which can involve loading kernel modules. > memalloc_nofs_save()/memalloc_nofs_restore() could do the job of making sure allocations use GFP_NOFS, I think? I guess those calls should be in fscrypt_load_extent_info() instead of just in the doc... > > That leaves the option I suggested, which probably I didn't explain clearly > enough: split up key setup so that part can be done when opening the file, and > part can be done during I/O. Specifically, when opening the file, preallocate > some number of crypto_skcipher objects. This would be limited to a fixed > number, like 128, even if the file has thousands of extents. Then, when doing > I/O to an extent, temporarily take a crypto_skcipher from the cache, derive the > extent's key using fscrypt_hkdf_expand(), and set it in the crypto_skcipher > using crypto_skcipher_setkey(). I didn't elaborate why it wasn't here and should have. With just this patchset, I thought a file only ever has extents with the same encryption mode, since an inode can't change encryption mode/key past creation at present . So loading the parent dir's fscrypt_info should be enough to ensure the module is loaded for the mode of all the extents. I suppose I'd need to ensure that reflinks also only reflink extents sharing the same encryption mode. In the future patchset which allows changing the key being used for new extents (naming that is hard), I had envisioned requiring the filesystem to provide a list of enc modes used by an inode when opening, and then fscrypt_file_open() could make sure all the necessary modules are loaded for those modes. > > That way, during I/O only fscrypt_hkdf_expand() and crypto_skcipher_setkey() > would be needed. Those are fairly safe to call during I/O, in contrast to > crypto_alloc_skcipher() which is really problematic to call during I/O. Could we use a mempool of skciphers for all of fscrypt, or should it only be for extents and be on a per-file basis? > > Of course, it will still be somewhat expensive to derive and set a key. So it > might also make sense to maintain a map that maps (master key, extent nonce, > encryption mode) to the corresponding cached crypto_skcipher, if any, so that an > already-prepared one can be used when possible. Same question: just for extent infos, or can this be generalized to all infos? > > By the way, blk-crypto-fallback (block/blk-crypto-fallback.c) does something > similar, as it had to solve a similar problem. The way it was solved is to > require that blk_crypto_fallback_start_using_mode() be called to preallocate the > crypto_skcipher objects for a given encryption mode. > > You actually could just use blk-crypto-fallback to do the en/decryption for you, > if you wanted to. I.e., you could use the blk-crypto API for en/decryption, > instead of going directly through crypto_skcipher. (Note that currently > blk-crypto is opt-in via the "inlinecrypt" mount option; it's not used by > default. But it doesn't *have* to be that way; it could just be always used. > It would be necessary to ensure that CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK gets > selected.) That would save having to reimplement the caching of crypto_skcipher > objects. However, key derivation would still need to be done at the filesystem > level, so it probably would still make sense to cache derived keys. > > - Eric Interesting. I will have to study that more, thanks for the pointer. Thank you! Sweet Tea
On Mon, Jan 02, 2023 at 05:31:02PM -0500, Sweet Tea Dorminy wrote: > (in which I fail to reply-all the first time) > > > > > When is the filesystem going to call fscrypt_load_extent_info()? > > > > My concern (which we've discussed, but probably I didn't explain clearly enough) > > is that the two "naive" solutions don't really work: > > > > Option 1: Set up during the I/O to the extent. I think this is not feasible > > because the full fscrypt_setup_encryption_info() is not safe to do doing I/O. > > For example, it allocates memory under GFP_KERNEL, and it uses > > crypto_alloc_skcipher() which can involve loading kernel modules. > > > memalloc_nofs_save()/memalloc_nofs_restore() could do the job of making sure > allocations use GFP_NOFS, I think? I guess those calls should be in > fscrypt_load_extent_info() instead of just in the doc... > > > > That leaves the option I suggested, which probably I didn't explain clearly > > enough: split up key setup so that part can be done when opening the file, and > > part can be done during I/O. Specifically, when opening the file, preallocate > > some number of crypto_skcipher objects. This would be limited to a fixed > > number, like 128, even if the file has thousands of extents. Then, when doing > > I/O to an extent, temporarily take a crypto_skcipher from the cache, derive the > > extent's key using fscrypt_hkdf_expand(), and set it in the crypto_skcipher > > using crypto_skcipher_setkey(). > > I didn't elaborate why it wasn't here and should have. With just this > patchset, I thought a file only ever has extents with the same encryption > mode, since an inode can't change encryption mode/key past creation at > present . So loading the parent dir's fscrypt_info should be enough to > ensure the module is loaded for the mode of all the extents. I suppose I'd > need to ensure that reflinks also only reflink extents sharing the same > encryption mode. > > In the future patchset which allows changing the key being used for new > extents (naming that is hard), I had envisioned requiring the filesystem to > provide a list of enc modes used by an inode when opening, and then > fscrypt_file_open() could make sure all the necessary modules are loaded for > those modes. "Loading the parent dir's fscrypt_info" isn't relevant here, since the filenames encryption mode can be different from the contents encryption mode. It's also possible for an encrypted file to be located in an unencrypted directory. Maybe you meant to be talking about the file itself being opened? Anyway, crypto_alloc_skcipher() takes a lock (crypto_alg_sem) under which memory is allocated with GFP_KERNEL. So neither preloading kernel modules nor memalloc_nofs_save() helps for it; it's still not GFP_NOFS-safe. I don't think we should allow files to have extents with different encryption modes. Encryption policies will still be a property of files. > > That way, during I/O only fscrypt_hkdf_expand() and crypto_skcipher_setkey() > > would be needed. Those are fairly safe to call during I/O, in contrast to > > crypto_alloc_skcipher() which is really problematic to call during I/O. > > Could we use a mempool of skciphers for all of fscrypt, or should it only be > for extents and be on a per-file basis? It probably should be global, or at least per-master-key, as it would be a massive overhead to allocate (e.g.) 128 crypto_skcipher objects for every file. blk-crypto-fallback uses a global cache. It does introduce a bottleneck and memory that can't be reclaimed, though. I'd appreciate any thoughts about other solutions. Maybe the number of objects in the cache could be scaled up and down as the number of in-core inodes changes. > > Of course, it will still be somewhat expensive to derive and set a key. So it > > might also make sense to maintain a map that maps (master key, extent nonce, > > encryption mode) to the corresponding cached crypto_skcipher, if any, so that an > > already-prepared one can be used when possible. > Same question: just for extent infos, or can this be generalized to all > infos? I think that we should definitely still cache the per-file key in inode::i_crypt_info and not in some global cache. - Eric
> > Anyway, crypto_alloc_skcipher() takes a lock (crypto_alg_sem) under which memory > is allocated with GFP_KERNEL. So neither preloading kernel modules nor > memalloc_nofs_save() helps for it; it's still not GFP_NOFS-safe. I'm still confused. My understanding is that memalloc_nofs_save() means all allocations on that thread until memalloc_nofs_restore() is called effectively gets GFP_NOFS appended to the allocation flags. So since crypto_alloc_skcipher()'s allocation appears to be on the same thread as we'd be calling memalloc_nofs_save/restore(), it would presumably get allocated as though it had flags GFP_KERNEL | GFP_NOFS, even though the call is kzalloc(..., GFP_KERNEL, ...). I don't understand how the lock would make a difference. Can you elaborate? Sorry for my confusion... Sweet Tea
On Mon, Jan 02, 2023 at 07:33:15PM -0500, Sweet Tea Dorminy wrote: > > > > > Anyway, crypto_alloc_skcipher() takes a lock (crypto_alg_sem) under which memory > > is allocated with GFP_KERNEL. So neither preloading kernel modules nor > > memalloc_nofs_save() helps for it; it's still not GFP_NOFS-safe. > > I'm still confused. My understanding is that memalloc_nofs_save() means all > allocations on that thread until memalloc_nofs_restore() is called > effectively gets GFP_NOFS appended to the allocation flags. So since > crypto_alloc_skcipher()'s allocation appears to be on the same thread as > we'd be calling memalloc_nofs_save/restore(), it would presumably get > allocated as though it had flags GFP_KERNEL | GFP_NOFS, even though the call > is kzalloc(..., GFP_KERNEL, ...). > > I don't understand how the lock would make a difference. Can you elaborate? > > Sorry for my confusion... Other tasks (using the crypto API for another purpose, perhaps totally unrelated to fs/crypto/) can take crypto_alg_sem without taking the same precaution. So, when your task that is running in fs-reclaim context and has used memalloc_nofs_save() tries to take the same lock, it might be that the lock is already held by another thread that is waiting for fs-reclaim to complete in order to satisfy a GFP_KERNEL allocation. That's a deadlock. Locks are only GFP_NOFS-safe when everyone agrees to use them that way. - Eric
On 1/2/23 19:47, Eric Biggers wrote: > On Mon, Jan 02, 2023 at 07:33:15PM -0500, Sweet Tea Dorminy wrote: >> >>> >>> Anyway, crypto_alloc_skcipher() takes a lock (crypto_alg_sem) under which memory >>> is allocated with GFP_KERNEL. So neither preloading kernel modules nor >>> memalloc_nofs_save() helps for it; it's still not GFP_NOFS-safe. >> >> I'm still confused. My understanding is that memalloc_nofs_save() means all >> allocations on that thread until memalloc_nofs_restore() is called >> effectively gets GFP_NOFS appended to the allocation flags. So since >> crypto_alloc_skcipher()'s allocation appears to be on the same thread as >> we'd be calling memalloc_nofs_save/restore(), it would presumably get >> allocated as though it had flags GFP_KERNEL | GFP_NOFS, even though the call >> is kzalloc(..., GFP_KERNEL, ...). >> >> I don't understand how the lock would make a difference. Can you elaborate? >> >> Sorry for my confusion... > > Other tasks (using the crypto API for another purpose, perhaps totally unrelated > to fs/crypto/) can take crypto_alg_sem without taking the same precaution. So, > when your task that is running in fs-reclaim context and has used > memalloc_nofs_save() tries to take the same lock, it might be that the lock is > already held by another thread that is waiting for fs-reclaim to complete in > order to satisfy a GFP_KERNEL allocation. > > That's a deadlock. > > Locks are only GFP_NOFS-safe when everyone agrees to use them that way. > > - Eric Ah that is definitely what I was missing, I've never had to think about that interaction. Thank you for explaining!
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 136156487e8f..82439fb73dd9 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -799,6 +799,56 @@ void fscrypt_free_extent_info(struct fscrypt_info **info_ptr) } EXPORT_SYMBOL_GPL(fscrypt_free_extent_info); +/** + * fscrypt_load_extent_info() - set up a preexisting extent's fscrypt_info + * @inode: the inode to which the extent belongs. Must be encrypted. + * @buf: a buffer containing the extent's stored context + * @len: the length of the @ctx buffer + * @info_ptr: a pointer to return the extent's fscrypt_info into. Should be + * a pointer to a member of the extent struct, as it will be passed + * back to the filesystem if key removal demands removal of the + * info from the extent + * + * This is not %GFP_NOFS safe, so the caller is expected to call + * memalloc_nofs_save/restore() if appropriate. + * + * Return: 0 if successful, or -errno if it fails. + */ +int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len, + struct fscrypt_info **info_ptr) +{ + int res; + union fscrypt_context ctx; + union fscrypt_policy policy; + + if (!fscrypt_has_encryption_key(inode)) + return -EINVAL; + + memcpy(&ctx, buf, len); + + res = fscrypt_policy_from_context(&policy, &ctx, res); + if (res) { + fscrypt_warn(inode, + "Unrecognized or corrupt encryption context"); + return res; + } + + if (!fscrypt_supported_policy(&policy, inode)) { + return -EINVAL; + } + + res = fscrypt_setup_encryption_info(inode, &policy, + fscrypt_context_nonce(&ctx), + IS_CASEFOLDED(inode) && + S_ISDIR(inode->i_mode), + info_ptr); + + if (res == -ENOPKG) /* Algorithm unavailable? */ + res = 0; + return res; +} +EXPORT_SYMBOL_GPL(fscrypt_load_extent_info); + /** * fscrypt_put_encryption_info() - free most of an inode's fscrypt data * @inode: an inode being evicted diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index e7de4872d375..aab5edc1155e 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -751,6 +751,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data) } EXPORT_SYMBOL_GPL(fscrypt_set_context); +/** + * fscrypt_set_extent_context() - Set the fscrypt context for an extent + * @ci: info from which to fetch policy and nonce + * @ctx: where context should be written + * @len: the size of ctx + * + * Given an fscrypt_info belonging to an extent (generated via + * fscrypt_prepare_new_extent()), generate a new context and write it to @ctx. + * len is checked to be at least FSCRYPT_SET_CONTEXT_MAX_SIZE bytes. + * + * Return: size of the resulting context or a negative error code. + */ +int fscrypt_set_extent_context(struct fscrypt_info *ci, void *ctx, size_t len) +{ + if (len < FSCRYPT_SET_CONTEXT_MAX_SIZE) + return -EINVAL; + return fscrypt_new_context(ctx, &ci->ci_policy, ci->ci_nonce); +} +EXPORT_SYMBOL_GPL(fscrypt_set_extent_context); + /** * fscrypt_parse_test_dummy_encryption() - parse the test_dummy_encryption mount option * @param: the mount option diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 6afdcb27f8a2..47c2df1061c7 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -324,6 +324,8 @@ int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg); int fscrypt_has_permitted_context(struct inode *parent, struct inode *child); int fscrypt_context_for_new_inode(void *ctx, struct inode *inode); int fscrypt_set_context(struct inode *inode, void *fs_data); +int fscrypt_set_extent_context(struct fscrypt_info *info, void *ctx, + size_t len); struct fscrypt_dummy_policy { const union fscrypt_policy *policy; @@ -367,6 +369,8 @@ int fscrypt_prepare_new_extent(struct inode *inode, struct fscrypt_info **info_ptr, bool *encrypt_ret); void fscrypt_free_extent_info(struct fscrypt_info **info_ptr); +int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len, + struct fscrypt_info **info_ptr); /* fname.c */ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname, @@ -532,6 +536,12 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data) return -EOPNOTSUPP; } +static inline int fscrypt_set_extent_context(struct fscrypt_info *info, + void *ctx, size_t len) +{ + return -EOPNOTSUPP; +} + struct fscrypt_dummy_policy { }; @@ -638,6 +648,13 @@ static inline void fscrypt_free_extent_info(struct fscrypt_info **info_ptr) { } +static inline int fscrypt_load_extent_info(struct inode *inode, void *buf, + size_t len, + struct fscrypt_info **info_ptr) +{ + return -EOPNOTSUPP; +} + /* fname.c */ static inline int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
The other half of using per-extent infos is saving and loading them from disk. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> --- fs/crypto/keysetup.c | 50 +++++++++++++++++++++++++++++++++++++++++ fs/crypto/policy.c | 20 +++++++++++++++++ include/linux/fscrypt.h | 17 ++++++++++++++ 3 files changed, 87 insertions(+)