* [PATCH] f2fs crypto: add rwsem to avoid data races @ 2015-05-19 5:36 Jaegeuk Kim 2015-05-19 14:29 ` Theodore Ts'o 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2015-05-19 5:36 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim Previoulsy, fi->i_crypt_info was not covered by any lock, resulting in memory leak. This patch adds a rwsem to avoid leaking objects on i_crypt_info. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/crypto_key.c | 29 ++++++++++++++++++++++------- fs/f2fs/f2fs.h | 1 + fs/f2fs/super.c | 1 + 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/fs/f2fs/crypto_key.c b/fs/f2fs/crypto_key.c index 8a10569..a25b164 100644 --- a/fs/f2fs/crypto_key.c +++ b/fs/f2fs/crypto_key.c @@ -87,7 +87,7 @@ out: return res; } -void f2fs_free_encryption_info(struct inode *inode) +static void _f2fs_free_encryption_info(struct inode *inode) { struct f2fs_inode_info *fi = F2FS_I(inode); struct f2fs_crypt_info *ci = fi->i_crypt_info; @@ -103,6 +103,13 @@ void f2fs_free_encryption_info(struct inode *inode) fi->i_crypt_info = NULL; } +void f2fs_free_encryption_info(struct inode *inode) +{ + down_write(&F2FS_I(inode)->crypto_rwsem); + _f2fs_free_encryption_info(inode); + up_write(&F2FS_I(inode)->crypto_rwsem); +} + int _f2fs_get_encryption_info(struct inode *inode) { struct f2fs_inode_info *fi = F2FS_I(inode); @@ -119,12 +126,13 @@ int _f2fs_get_encryption_info(struct inode *inode) if (res) return res; - if (fi->i_crypt_info) { - if (!fi->i_crypt_info->ci_keyring_key || - key_validate(fi->i_crypt_info->ci_keyring_key) == 0) - return 0; - f2fs_free_encryption_info(inode); + down_read(&fi->crypto_rwsem); + if (fi->i_crypt_info && (!fi->i_crypt_info->ci_keyring_key || + key_validate(fi->i_crypt_info->ci_keyring_key) == 0)) { + up_read(&fi->crypto_rwsem); + return 0; } + up_read(&fi->crypto_rwsem); res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, @@ -187,8 +195,11 @@ out: res = 0; kmem_cache_free(f2fs_crypt_info_cachep, crypt_info); } else { + down_write(&fi->crypto_rwsem); + _f2fs_free_encryption_info(inode); fi->i_crypt_info = crypt_info; crypt_info->ci_keyring_key = keyring_key; + up_write(&fi->crypto_rwsem); keyring_key = NULL; } if (keyring_key) @@ -199,6 +210,10 @@ out: int f2fs_has_encryption_key(struct inode *inode) { struct f2fs_inode_info *fi = F2FS_I(inode); + int ret; - return (fi->i_crypt_info != NULL); + down_read(&fi->crypto_rwsem); + ret = (fi->i_crypt_info != NULL); + up_read(&fi->crypto_rwsem); + return ret; } diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 5119167..c44d7bf 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -431,6 +431,7 @@ struct f2fs_inode_info { #ifdef CONFIG_F2FS_FS_ENCRYPTION /* Encryption params */ struct f2fs_crypt_info *i_crypt_info; + struct rw_semaphore crypto_rwsem; /* lock for crypt_info */ #endif }; diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index bbeb6d7..137d1b7 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -418,6 +418,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) #ifdef CONFIG_F2FS_FS_ENCRYPTION fi->i_crypt_info = NULL; + init_rwsem(&fi->crypto_rwsem); #endif return &fi->vfs_inode; } -- 2.1.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs crypto: add rwsem to avoid data races 2015-05-19 5:36 [PATCH] f2fs crypto: add rwsem to avoid data races Jaegeuk Kim @ 2015-05-19 14:29 ` Theodore Ts'o 2015-05-19 17:42 ` Jaegeuk Kim [not found] ` <555B4A32.3000302@gmail.com> 0 siblings, 2 replies; 7+ messages in thread From: Theodore Ts'o @ 2015-05-19 14:29 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel On Mon, May 18, 2015 at 10:36:41PM -0700, Jaegeuk Kim wrote: > Previoulsy, fi->i_crypt_info was not covered by any lock, resulting in > memory leak. > > This patch adds a rwsem to avoid leaking objects on i_crypt_info. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> I'm not sure we need an rwsem to fix this issue. In terms of serializing the creation and deletion of the structure, it should be possible to use an cmpxchg() on the pointer itself. (e.g., if we lose the race on the creation side, we just release our structure and use the one that the winner allocated). If we do end up needing to serialize access to the tfm in the i_crypt_info object for datapath reads/writes, then we might need a mutex, but I think that should be it, no? - Ted ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs crypto: add rwsem to avoid data races 2015-05-19 14:29 ` Theodore Ts'o @ 2015-05-19 17:42 ` Jaegeuk Kim [not found] ` <555B4A32.3000302@gmail.com> 1 sibling, 0 replies; 7+ messages in thread From: Jaegeuk Kim @ 2015-05-19 17:42 UTC (permalink / raw) To: Theodore Ts'o, linux-kernel, linux-fsdevel, linux-f2fs-devel On Tue, May 19, 2015 at 10:29:43AM -0400, Theodore Ts'o wrote: > On Mon, May 18, 2015 at 10:36:41PM -0700, Jaegeuk Kim wrote: > > Previoulsy, fi->i_crypt_info was not covered by any lock, resulting in > > memory leak. > > > > This patch adds a rwsem to avoid leaking objects on i_crypt_info. > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > I'm not sure we need an rwsem to fix this issue. In terms of > serializing the creation and deletion of the structure, it should be > possible to use an cmpxchg() on the pointer itself. (e.g., if we lose > the race on the creation side, we just release our structure and use > the one that the winner allocated). What I'm looking is when multiple threads enter into get_encryption_info. If ei->i_crypt_info doesn't exist, all of them can go into the allocation phase. Since, new ei->i_crypt_info will be assigned after finishing all the stuffs, it can do allocation redundantly without freeing the existing one. I've seen some crypt_info object leaks reported by kmemleak after finishing some tests in xfstests below. And I confirmed that this patch fixes that. ============================================================================= BUG f2fs_crypt_info (Tainted: G O ): Objects remaining in f2fs_crypt_info on kmem_cache_close() ----------------------------------------------------------------------------- Disabling lock debugging due to kernel taint CPU: 3 PID: 26284 Comm: rmmod Tainted: G B O 4.1.0-rc2+ #20 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 ffff8800376d98c0 ffff88001dd13d38 ffffffff817ffd6a 0000000000000001 ffffea00007d6a80 ffff88001dd13e08 ffffffff81218ac0 ffff880000000020 ffff88001dd13e18 ffff88001dd13dc8 656a624f001a0000 616d657220737463 Call Trace: [<ffffffff817ffd6a>] dump_stack+0x4f/0x7b [<ffffffff81218ac0>] slab_err+0xa0/0xb0 [<ffffffff8121c89c>] ? __kmalloc+0x37c/0x3d0 [<ffffffff8121dcd6>] ? __kmem_cache_shutdown+0x126/0x340 [<ffffffff8121dcd6>] ? __kmem_cache_shutdown+0x126/0x340 [<ffffffff8121dcf6>] __kmem_cache_shutdown+0x146/0x340 [<ffffffff811e3079>] ? kmem_cache_destroy+0x39/0x130 [<ffffffff811e30e8>] kmem_cache_destroy+0xa8/0x130 [<ffffffffa03a0801>] f2fs_exit_crypto+0x41/0x50 [f2fs] [<ffffffffa03a1e2a>] exit_f2fs_fs+0x28/0x1fe [f2fs] [<ffffffff8113125c>] SyS_delete_module+0x18c/0x210 [<ffffffff8180b532>] system_call_fastpath+0x16/0x7a INFO: Object 0xffff88001f5ab3e0 @offset=5088 INFO: Allocated in _f2fs_get_encryption_info+0x97/0x260 [f2fs] __slab_alloc+0x4bd/0x562 kmem_cache_alloc+0x2a4/0x390 _f2fs_get_encryption_info+0x97/0x260 [f2fs] f2fs_file_open+0x57/0x70 [f2fs] do_dentry_open+0x257/0x350 vfs_open+0x49/0x50 do_last+0x208/0x13e0 path_openat+0x84/0x660 do_filp_open+0x3a/0x90 do_sys_open+0x128/0x220 SyS_creat+0x1e/0x20 system_call_fastpath+0x16/0x7a INFO: Freed in f2fs_free_encryption_info+0x52/0x70 [f2fs] __slab_free+0x39/0x214 kmem_cache_free+0x394/0x3a0 f2fs_free_encryption_info+0x52/0x70 [f2fs] f2fs_evict_inode+0x15a/0x460 [f2fs] evict+0xb8/0x190 iput+0x30e/0x3f0 d_delete+0x178/0x1b0 vfs_rmdir+0x122/0x140 do_rmdir+0x1fb/0x220 SyS_rmdir+0x16/0x20 system_call_fastpath+0x16/0x7a > > If we do end up needing to serialize access to the tfm in the > i_crypt_info object for datapath reads/writes, then we might need a > mutex, but I think that should be it, no? Seems like we don't need to care about serialization on tfm. Thanks, ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <555B4A32.3000302@gmail.com>]
* Re: [f2fs-dev] [PATCH] f2fs crypto: add rwsem to avoid data races [not found] ` <555B4A32.3000302@gmail.com> @ 2015-05-20 0:38 ` Jaegeuk Kim 2015-05-20 4:35 ` Theodore Ts'o 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2015-05-20 0:38 UTC (permalink / raw) To: nick; +Cc: Theodore Ts'o, linux-kernel, linux-fsdevel, linux-f2fs-devel On Tue, May 19, 2015 at 10:35:30AM -0400, nick wrote: > > > On 2015-05-19 10:29 AM, Theodore Ts'o wrote: > > On Mon, May 18, 2015 at 10:36:41PM -0700, Jaegeuk Kim wrote: > >> Previoulsy, fi->i_crypt_info was not covered by any lock, resulting in > >> memory leak. > >> > >> This patch adds a rwsem to avoid leaking objects on i_crypt_info. > >> > >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > I'm not sure we need an rwsem to fix this issue. In terms of > > serializing the creation and deletion of the structure, it should be > > possible to use an cmpxchg() on the pointer itself. (e.g., if we lose > > the race on the creation side, we just release our structure and use > > the one that the winner allocated). > > > > If we do end up needing to serialize access to the tfm in the > > i_crypt_info object for datapath reads/writes, then we might need a > > mutex, but I think that should be it, no? > > > > - Ted > > > I have to agree with Ted here, as mutual exclusion locking is ideal > for the scenario here of a reader vs writer exclusion. My only concern What I'm saying is writer vs writer actually. > is that can there be many readers to one writer here as if so reader/writer > spin locks may be better. I could write another patch using a rwlock by removing needless down_write for f2fs_free_encryption_info. Thanks, > Nick ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs crypto: add rwsem to avoid data races 2015-05-20 0:38 ` [f2fs-dev] " Jaegeuk Kim @ 2015-05-20 4:35 ` Theodore Ts'o 2015-05-20 4:55 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: Theodore Ts'o @ 2015-05-20 4:35 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: nick, linux-kernel, linux-fsdevel, linux-f2fs-devel On Tue, May 19, 2015 at 05:38:59PM -0700, Jaegeuk Kim wrote: > > What I'm saying is writer vs writer actually. This is a rough draft of what I had in mind. This fixes the tfm allocation issue in the writepage path, as well as using a lockless cmpxchg algorithm to address the race you were concerned about. What do you think? I'm still running xfstests, and I want to do another desk check of the code tomorrow after I get a good night's sleep, but it does pass my quick smoketest script, and enough of xfstests's quick test group that any bugs left are probably the more subtle ones. - Ted >From 6a57d7bbf7bd2d31432e55f266543bb56bf7e1fc Mon Sep 17 00:00:00 2001 From: Theodore Ts'o <tytso@mit.edu> Date: Wed, 20 May 2015 00:27:45 -0400 Subject: [PATCH] ext4 crypto: use per-inode tfm structure As suggested by Herbert Xu, we shouldn't allocate a new tfm each time we read or write a page. Instead we can use a single tfm hanging off the inode's crypt_info structure for all of our encryption needs for that inode, since the tfm can be used by multiple crypto requests in parallel. Also use cmpxchg() to avoid races that could result in crypt_info structure getting doubly allocated or doubly freed. Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/crypto.c | 61 ++-------------------------- fs/ext4/crypto_fname.c | 48 +--------------------- fs/ext4/crypto_key.c | 108 +++++++++++++++++++++++++++++++++++-------------- fs/ext4/dir.c | 3 -- fs/ext4/ext4.h | 5 +-- fs/ext4/ext4_crypto.h | 3 -- fs/ext4/namei.c | 17 ++++---- fs/ext4/super.c | 2 +- fs/ext4/symlink.c | 2 +- 9 files changed, 95 insertions(+), 154 deletions(-) diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c index 1484b58..68c7ab8 100644 --- a/fs/ext4/crypto.c +++ b/fs/ext4/crypto.c @@ -80,8 +80,6 @@ void ext4_release_crypto_ctx(struct ext4_crypto_ctx *ctx) } ctx->w.control_page = NULL; if (ctx->flags & EXT4_CTX_REQUIRES_FREE_ENCRYPT_FL) { - if (ctx->tfm) - crypto_free_tfm(ctx->tfm); kmem_cache_free(ext4_crypto_ctx_cachep, ctx); } else { spin_lock_irqsave(&ext4_crypto_ctx_lock, flags); @@ -136,36 +134,6 @@ struct ext4_crypto_ctx *ext4_get_crypto_ctx(struct inode *inode) } ctx->flags &= ~EXT4_WRITE_PATH_FL; - /* Allocate a new Crypto API context if we don't already have - * one or if it isn't the right mode. */ - if (ctx->tfm && (ctx->mode != ci->ci_data_mode)) { - crypto_free_tfm(ctx->tfm); - ctx->tfm = NULL; - ctx->mode = EXT4_ENCRYPTION_MODE_INVALID; - } - if (!ctx->tfm) { - switch (ci->ci_data_mode) { - case EXT4_ENCRYPTION_MODE_AES_256_XTS: - ctx->tfm = crypto_ablkcipher_tfm( - crypto_alloc_ablkcipher("xts(aes)", 0, 0)); - break; - case EXT4_ENCRYPTION_MODE_AES_256_GCM: - /* TODO(mhalcrow): AEAD w/ gcm(aes); - * crypto_aead_setauthsize() */ - ctx->tfm = ERR_PTR(-ENOTSUPP); - break; - default: - BUG(); - } - if (IS_ERR_OR_NULL(ctx->tfm)) { - res = PTR_ERR(ctx->tfm); - ctx->tfm = NULL; - goto out; - } - ctx->mode = ci->ci_data_mode; - } - BUG_ON(ci->ci_size != ext4_encryption_key_size(ci->ci_data_mode)); - out: if (res) { if (!IS_ERR_OR_NULL(ctx)) @@ -195,8 +163,6 @@ void ext4_exit_crypto(void) ext4_bounce_page_pool); } } - if (pos->tfm) - crypto_free_tfm(pos->tfm); kmem_cache_free(ext4_crypto_ctx_cachep, pos); } INIT_LIST_HEAD(&ext4_free_crypto_ctxs); @@ -312,32 +278,11 @@ static int ext4_page_crypto(struct ext4_crypto_ctx *ctx, struct ablkcipher_request *req = NULL; DECLARE_EXT4_COMPLETION_RESULT(ecr); struct scatterlist dst, src; - struct ext4_inode_info *ei = EXT4_I(inode); - struct crypto_ablkcipher *atfm = __crypto_ablkcipher_cast(ctx->tfm); + struct ext4_crypt_info *ci = EXT4_I(inode)->i_crypt_info; + struct crypto_ablkcipher *tfm = ci->ci_ctfm; int res = 0; - BUG_ON(!ctx->tfm); - BUG_ON(ctx->mode != ei->i_crypt_info->ci_data_mode); - - if (ctx->mode != EXT4_ENCRYPTION_MODE_AES_256_XTS) { - printk_ratelimited(KERN_ERR - "%s: unsupported crypto algorithm: %d\n", - __func__, ctx->mode); - return -ENOTSUPP; - } - - crypto_ablkcipher_clear_flags(atfm, ~0); - crypto_tfm_set_flags(ctx->tfm, CRYPTO_TFM_REQ_WEAK_KEY); - - res = crypto_ablkcipher_setkey(atfm, ei->i_crypt_info->ci_raw, - ei->i_crypt_info->ci_size); - if (res) { - printk_ratelimited(KERN_ERR - "%s: crypto_ablkcipher_setkey() failed\n", - __func__); - return res; - } - req = ablkcipher_request_alloc(atfm, GFP_NOFS); + req = ablkcipher_request_alloc(tfm, GFP_NOFS); if (!req) { printk_ratelimited(KERN_ERR "%s: crypto_request_alloc() failed\n", diff --git a/fs/ext4/crypto_fname.c b/fs/ext4/crypto_fname.c index e63dd29..29a2dc9 100644 --- a/fs/ext4/crypto_fname.c +++ b/fs/ext4/crypto_fname.c @@ -252,52 +252,6 @@ static int digest_decode(const char *src, int len, char *dst) return cp - dst; } -int ext4_setup_fname_crypto(struct inode *inode) -{ - struct ext4_inode_info *ei = EXT4_I(inode); - struct ext4_crypt_info *ci = ei->i_crypt_info; - struct crypto_ablkcipher *ctfm; - int res; - - /* Check if the crypto policy is set on the inode */ - res = ext4_encrypted_inode(inode); - if (res == 0) - return 0; - - res = ext4_get_encryption_info(inode); - if (res < 0) - return res; - ci = ei->i_crypt_info; - - if (!ci || ci->ci_ctfm) - return 0; - - if (ci->ci_filename_mode != EXT4_ENCRYPTION_MODE_AES_256_CTS) { - printk_once(KERN_WARNING "ext4: unsupported key mode %d\n", - ci->ci_filename_mode); - return -ENOKEY; - } - - ctfm = crypto_alloc_ablkcipher("cts(cbc(aes))", 0, 0); - if (!ctfm || IS_ERR(ctfm)) { - res = ctfm ? PTR_ERR(ctfm) : -ENOMEM; - printk(KERN_DEBUG "%s: error (%d) allocating crypto tfm\n", - __func__, res); - return res; - } - crypto_ablkcipher_clear_flags(ctfm, ~0); - crypto_tfm_set_flags(crypto_ablkcipher_tfm(ctfm), - CRYPTO_TFM_REQ_WEAK_KEY); - - res = crypto_ablkcipher_setkey(ctfm, ci->ci_raw, ci->ci_size); - if (res) { - crypto_free_ablkcipher(ctfm); - return -EIO; - } - ci->ci_ctfm = ctfm; - return 0; -} - /** * ext4_fname_crypto_round_up() - * @@ -449,7 +403,7 @@ int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname, fname->disk_name.len = iname->len; goto out; } - ret = ext4_setup_fname_crypto(dir); + ret = ext4_get_encryption_info(dir); if (ret) return ret; ci = EXT4_I(dir)->i_crypt_info; diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c index 858d7d6..442d24e 100644 --- a/fs/ext4/crypto_key.c +++ b/fs/ext4/crypto_key.c @@ -84,20 +84,32 @@ out: return res; } -void ext4_free_encryption_info(struct inode *inode) +void ext4_free_crypt_info(struct ext4_crypt_info *ci) { - struct ext4_inode_info *ei = EXT4_I(inode); - struct ext4_crypt_info *ci = ei->i_crypt_info; - if (!ci) return; if (ci->ci_keyring_key) key_put(ci->ci_keyring_key); crypto_free_ablkcipher(ci->ci_ctfm); - memzero_explicit(&ci->ci_raw, sizeof(ci->ci_raw)); kmem_cache_free(ext4_crypt_info_cachep, ci); - ei->i_crypt_info = NULL; +} + +void ext4_free_encryption_info(struct inode *inode, + struct ext4_crypt_info *ci) +{ + struct ext4_inode_info *ei = EXT4_I(inode); + struct ext4_crypt_info *prev; + + if (ci == NULL) + ci = ACCESS_ONCE(ei->i_crypt_info); + if (ci == NULL) + return; + prev = cmpxchg(&ei->i_crypt_info, ci, NULL); + if (prev != ci) + return; + + ext4_free_crypt_info(ci); } int _ext4_get_encryption_info(struct inode *inode) @@ -111,6 +123,10 @@ int _ext4_get_encryption_info(struct inode *inode) struct ext4_encryption_context ctx; struct user_key_payload *ukp; struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); + struct crypto_ablkcipher *ctfm; + const char *cipher_str; + char raw_key[EXT4_MAX_KEY_SIZE]; + char mode; int res; if (!ext4_read_workqueue) { @@ -119,11 +135,14 @@ int _ext4_get_encryption_info(struct inode *inode) return res; } - if (ei->i_crypt_info) { - if (!ei->i_crypt_info->ci_keyring_key || - key_validate(ei->i_crypt_info->ci_keyring_key) == 0) +retry: + crypt_info = ACCESS_ONCE(ei->i_crypt_info); + if (crypt_info) { + if (!crypt_info->ci_keyring_key || + key_validate(crypt_info->ci_keyring_key) == 0) return 0; - ext4_free_encryption_info(inode); + ext4_free_encryption_info(inode, crypt_info); + goto retry; } res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION, @@ -144,26 +163,37 @@ int _ext4_get_encryption_info(struct inode *inode) if (!crypt_info) return -ENOMEM; - ei->i_crypt_policy_flags = ctx.flags; crypt_info->ci_flags = ctx.flags; crypt_info->ci_data_mode = ctx.contents_encryption_mode; crypt_info->ci_filename_mode = ctx.filenames_encryption_mode; crypt_info->ci_ctfm = NULL; + crypt_info->ci_keyring_key = NULL; memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor, sizeof(crypt_info->ci_master_key)); if (S_ISREG(inode->i_mode)) - crypt_info->ci_size = - ext4_encryption_key_size(crypt_info->ci_data_mode); + mode = crypt_info->ci_data_mode; else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) - crypt_info->ci_size = - ext4_encryption_key_size(crypt_info->ci_filename_mode); + mode = crypt_info->ci_filename_mode; else BUG(); - BUG_ON(!crypt_info->ci_size); - if (DUMMY_ENCRYPTION_ENABLED(sbi)) { - memset(crypt_info->ci_raw, 0x42, EXT4_AES_256_XTS_KEY_SIZE); + switch (mode) { + case EXT4_ENCRYPTION_MODE_AES_256_XTS: + cipher_str = "xts(aes)"; + break; + case EXT4_ENCRYPTION_MODE_AES_256_CTS: + cipher_str = "cts(cbc(aes))"; + break; + default: + printk_once(KERN_WARNING + "ext4: unsupported key mode %d (ino %u)\n", + mode, (unsigned) inode->i_ino); + res = -ENOKEY; goto out; } + if (DUMMY_ENCRYPTION_ENABLED(sbi)) { + memset(raw_key, 0x42, EXT4_AES_256_XTS_KEY_SIZE); + goto got_key; + } memcpy(full_key_descriptor, EXT4_KEY_DESC_PREFIX, EXT4_KEY_DESC_PREFIX_SIZE); sprintf(full_key_descriptor + EXT4_KEY_DESC_PREFIX_SIZE, @@ -177,6 +207,7 @@ int _ext4_get_encryption_info(struct inode *inode) keyring_key = NULL; goto out; } + crypt_info->ci_keyring_key = keyring_key; BUG_ON(keyring_key->type != &key_type_logon); ukp = ((struct user_key_payload *)keyring_key->payload.data); if (ukp->datalen != sizeof(struct ext4_encryption_key)) { @@ -188,19 +219,36 @@ int _ext4_get_encryption_info(struct inode *inode) EXT4_KEY_DERIVATION_NONCE_SIZE); BUG_ON(master_key->size != EXT4_AES_256_XTS_KEY_SIZE); res = ext4_derive_key_aes(ctx.nonce, master_key->raw, - crypt_info->ci_raw); -out: - if (res < 0) { - if (res == -ENOKEY) - res = 0; - kmem_cache_free(ext4_crypt_info_cachep, crypt_info); - } else { - ei->i_crypt_info = crypt_info; - crypt_info->ci_keyring_key = keyring_key; - keyring_key = NULL; + raw_key); +got_key: + ctfm = crypto_alloc_ablkcipher(cipher_str, 0, 0); + if (!ctfm || IS_ERR(ctfm)) { + res = ctfm ? PTR_ERR(ctfm) : -ENOMEM; + printk(KERN_DEBUG + "%s: error %d (inode %u) allocating crypto tfm\n", + __func__, res, (unsigned) inode->i_ino); + goto out; + } + crypt_info->ci_ctfm = ctfm; + crypto_ablkcipher_clear_flags(ctfm, ~0); + crypto_tfm_set_flags(crypto_ablkcipher_tfm(ctfm), + CRYPTO_TFM_REQ_WEAK_KEY); + res = crypto_ablkcipher_setkey(ctfm, raw_key, + ext4_encryption_key_size(mode)); + if (res) + goto out; + memzero_explicit(raw_key, sizeof(raw_key)); + if (cmpxchg(&ei->i_crypt_info, NULL, crypt_info) != NULL) { + ext4_free_crypt_info(crypt_info); + goto retry; } - if (keyring_key) - key_put(keyring_key); + return 0; + +out: + if (res == -ENOKEY) + res = 0; + ext4_free_crypt_info(crypt_info); + memzero_explicit(raw_key, sizeof(raw_key)); return res; } diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index 28cb94f..e11e6ae 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -133,9 +133,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) return err; } - err = ext4_setup_fname_crypto(inode); - if (err) - return err; if (ext4_encrypted_inode(inode)) { err = ext4_fname_crypto_alloc_buffer(inode, EXT4_NAME_LEN, &fname_crypto_str); diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 23e33fb..7435ff2 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -911,7 +911,6 @@ struct ext4_inode_info { /* on-disk additional length */ __u16 i_extra_isize; - char i_crypt_policy_flags; /* Indicate the inline data space. */ u16 i_inline_off; @@ -2105,7 +2104,6 @@ int ext4_fname_usr_to_disk(struct inode *inode, const struct qstr *iname, struct ext4_str *oname); #ifdef CONFIG_EXT4_FS_ENCRYPTION -int ext4_setup_fname_crypto(struct inode *inode); void ext4_fname_crypto_free_buffer(struct ext4_str *crypto_str); int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname, int lookup, struct ext4_filename *fname); @@ -2131,7 +2129,8 @@ static inline void ext4_fname_free_filename(struct ext4_filename *fname) { } /* crypto_key.c */ -void ext4_free_encryption_info(struct inode *inode); +void ext4_free_crypt_info(struct ext4_crypt_info *ci); +void ext4_free_encryption_info(struct inode *inode, struct ext4_crypt_info *ci); int _ext4_get_encryption_info(struct inode *inode); #ifdef CONFIG_EXT4_FS_ENCRYPTION diff --git a/fs/ext4/ext4_crypto.h b/fs/ext4/ext4_crypto.h index c5258f2..34e0d24 100644 --- a/fs/ext4/ext4_crypto.h +++ b/fs/ext4/ext4_crypto.h @@ -74,13 +74,11 @@ struct ext4_encryption_key { } __attribute__((__packed__)); struct ext4_crypt_info { - unsigned char ci_size; char ci_data_mode; char ci_filename_mode; char ci_flags; struct crypto_ablkcipher *ci_ctfm; struct key *ci_keyring_key; - char ci_raw[EXT4_MAX_KEY_SIZE]; char ci_master_key[EXT4_KEY_DESCRIPTOR_SIZE]; }; @@ -89,7 +87,6 @@ struct ext4_crypt_info { #define EXT4_WRITE_PATH_FL 0x00000004 struct ext4_crypto_ctx { - struct crypto_tfm *tfm; /* Crypto API context */ union { struct { struct page *bounce_page; /* Ciphertext page */ diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 9bed99f..6ab50f8 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -607,11 +607,12 @@ static struct stats dx_show_leaf(struct inode *dir, char *name; struct ext4_str fname_crypto_str = {.name = NULL, .len = 0}; - int res; + int res = 0; name = de->name; len = de->name_len; - res = ext4_setup_fname_crypto(dir); + if (ext4_encrypted_inode(inode)) + res = ext4_get_encryption_info(dir); if (res) { printk(KERN_WARNING "Error setting up" " fname crypto: %d\n", res); @@ -953,12 +954,12 @@ static int htree_dirblock_to_tree(struct file *dir_file, EXT4_DIR_REC_LEN(0)); #ifdef CONFIG_EXT4_FS_ENCRYPTION /* Check if the directory is encrypted */ - err = ext4_setup_fname_crypto(dir); - if (err) { - brelse(bh); - return err; - } if (ext4_encrypted_inode(dir)) { + err = ext4_get_encryption_info(dir); + if (err < 0) { + brelse(bh); + return err; + } err = ext4_fname_crypto_alloc_buffer(dir, EXT4_NAME_LEN, &fname_crypto_str); if (err < 0) { @@ -3108,7 +3109,7 @@ static int ext4_symlink(struct inode *dir, err = ext4_inherit_context(dir, inode); if (err) goto err_drop_inode; - err = ext4_setup_fname_crypto(inode); + err = ext4_get_encryption_info(inode); if (err) goto err_drop_inode; istr.name = (const unsigned char *) symname; diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b0bd1c1..56bfc2f 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -959,7 +959,7 @@ void ext4_clear_inode(struct inode *inode) } #ifdef CONFIG_EXT4_FS_ENCRYPTION if (EXT4_I(inode)->i_crypt_info) - ext4_free_encryption_info(inode); + ext4_free_encryption_info(inode, EXT4_I(inode)->i_crypt_info); #endif } diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c index 3287088..68e915a 100644 --- a/fs/ext4/symlink.c +++ b/fs/ext4/symlink.c @@ -37,7 +37,7 @@ static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd) if (!ext4_encrypted_inode(inode)) return page_follow_link_light(dentry, nd); - res = ext4_setup_fname_crypto(inode); + res = ext4_get_encryption_info(inode); if (res) return ERR_PTR(res); -- 2.3.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs crypto: add rwsem to avoid data races 2015-05-20 4:35 ` Theodore Ts'o @ 2015-05-20 4:55 ` Jaegeuk Kim 2015-05-20 12:38 ` Theodore Ts'o 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2015-05-20 4:55 UTC (permalink / raw) To: Theodore Ts'o, nick, linux-kernel, linux-fsdevel, linux-f2fs-devel On Wed, May 20, 2015 at 12:35:18AM -0400, Theodore Ts'o wrote: > On Tue, May 19, 2015 at 05:38:59PM -0700, Jaegeuk Kim wrote: > > > > What I'm saying is writer vs writer actually. > > This is a rough draft of what I had in mind. This fixes the tfm > allocation issue in the writepage path, as well as using a lockless > cmpxchg algorithm to address the race you were concerned about. > > What do you think? I'm still running xfstests, and I want to do > another desk check of the code tomorrow after I get a good night's > sleep, but it does pass my quick smoketest script, and enough of > xfstests's quick test group that any bugs left are probably the more > subtle ones. Looking at a glance, it's mostly same as what I wanted. The key is to share ci->ci_ctfm for regular file and the other dir/symlink files. So, ext4_get_encryption_info will handle most of cases. In ext4_readdir, it also needs ext4_get_encryption_info? Using cmpxchg is really cool to me. I'll test this too in f2fs tomorrow. Thanks, > > - Ted > > >From 6a57d7bbf7bd2d31432e55f266543bb56bf7e1fc Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Wed, 20 May 2015 00:27:45 -0400 > Subject: [PATCH] ext4 crypto: use per-inode tfm structure > > As suggested by Herbert Xu, we shouldn't allocate a new tfm each time > we read or write a page. Instead we can use a single tfm hanging off > the inode's crypt_info structure for all of our encryption needs for > that inode, since the tfm can be used by multiple crypto requests in > parallel. > > Also use cmpxchg() to avoid races that could result in crypt_info > structure getting doubly allocated or doubly freed. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > fs/ext4/crypto.c | 61 ++-------------------------- > fs/ext4/crypto_fname.c | 48 +--------------------- > fs/ext4/crypto_key.c | 108 +++++++++++++++++++++++++++++++++++-------------- > fs/ext4/dir.c | 3 -- > fs/ext4/ext4.h | 5 +-- > fs/ext4/ext4_crypto.h | 3 -- > fs/ext4/namei.c | 17 ++++---- > fs/ext4/super.c | 2 +- > fs/ext4/symlink.c | 2 +- > 9 files changed, 95 insertions(+), 154 deletions(-) > > diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c > index 1484b58..68c7ab8 100644 > --- a/fs/ext4/crypto.c > +++ b/fs/ext4/crypto.c > @@ -80,8 +80,6 @@ void ext4_release_crypto_ctx(struct ext4_crypto_ctx *ctx) > } > ctx->w.control_page = NULL; > if (ctx->flags & EXT4_CTX_REQUIRES_FREE_ENCRYPT_FL) { > - if (ctx->tfm) > - crypto_free_tfm(ctx->tfm); > kmem_cache_free(ext4_crypto_ctx_cachep, ctx); > } else { > spin_lock_irqsave(&ext4_crypto_ctx_lock, flags); > @@ -136,36 +134,6 @@ struct ext4_crypto_ctx *ext4_get_crypto_ctx(struct inode *inode) > } > ctx->flags &= ~EXT4_WRITE_PATH_FL; > > - /* Allocate a new Crypto API context if we don't already have > - * one or if it isn't the right mode. */ > - if (ctx->tfm && (ctx->mode != ci->ci_data_mode)) { > - crypto_free_tfm(ctx->tfm); > - ctx->tfm = NULL; > - ctx->mode = EXT4_ENCRYPTION_MODE_INVALID; > - } > - if (!ctx->tfm) { > - switch (ci->ci_data_mode) { > - case EXT4_ENCRYPTION_MODE_AES_256_XTS: > - ctx->tfm = crypto_ablkcipher_tfm( > - crypto_alloc_ablkcipher("xts(aes)", 0, 0)); > - break; > - case EXT4_ENCRYPTION_MODE_AES_256_GCM: > - /* TODO(mhalcrow): AEAD w/ gcm(aes); > - * crypto_aead_setauthsize() */ > - ctx->tfm = ERR_PTR(-ENOTSUPP); > - break; > - default: > - BUG(); > - } > - if (IS_ERR_OR_NULL(ctx->tfm)) { > - res = PTR_ERR(ctx->tfm); > - ctx->tfm = NULL; > - goto out; > - } > - ctx->mode = ci->ci_data_mode; > - } > - BUG_ON(ci->ci_size != ext4_encryption_key_size(ci->ci_data_mode)); > - > out: > if (res) { > if (!IS_ERR_OR_NULL(ctx)) > @@ -195,8 +163,6 @@ void ext4_exit_crypto(void) > ext4_bounce_page_pool); > } > } > - if (pos->tfm) > - crypto_free_tfm(pos->tfm); > kmem_cache_free(ext4_crypto_ctx_cachep, pos); > } > INIT_LIST_HEAD(&ext4_free_crypto_ctxs); > @@ -312,32 +278,11 @@ static int ext4_page_crypto(struct ext4_crypto_ctx *ctx, > struct ablkcipher_request *req = NULL; > DECLARE_EXT4_COMPLETION_RESULT(ecr); > struct scatterlist dst, src; > - struct ext4_inode_info *ei = EXT4_I(inode); > - struct crypto_ablkcipher *atfm = __crypto_ablkcipher_cast(ctx->tfm); > + struct ext4_crypt_info *ci = EXT4_I(inode)->i_crypt_info; > + struct crypto_ablkcipher *tfm = ci->ci_ctfm; > int res = 0; > > - BUG_ON(!ctx->tfm); > - BUG_ON(ctx->mode != ei->i_crypt_info->ci_data_mode); > - > - if (ctx->mode != EXT4_ENCRYPTION_MODE_AES_256_XTS) { > - printk_ratelimited(KERN_ERR > - "%s: unsupported crypto algorithm: %d\n", > - __func__, ctx->mode); > - return -ENOTSUPP; > - } > - > - crypto_ablkcipher_clear_flags(atfm, ~0); > - crypto_tfm_set_flags(ctx->tfm, CRYPTO_TFM_REQ_WEAK_KEY); > - > - res = crypto_ablkcipher_setkey(atfm, ei->i_crypt_info->ci_raw, > - ei->i_crypt_info->ci_size); > - if (res) { > - printk_ratelimited(KERN_ERR > - "%s: crypto_ablkcipher_setkey() failed\n", > - __func__); > - return res; > - } > - req = ablkcipher_request_alloc(atfm, GFP_NOFS); > + req = ablkcipher_request_alloc(tfm, GFP_NOFS); > if (!req) { > printk_ratelimited(KERN_ERR > "%s: crypto_request_alloc() failed\n", > diff --git a/fs/ext4/crypto_fname.c b/fs/ext4/crypto_fname.c > index e63dd29..29a2dc9 100644 > --- a/fs/ext4/crypto_fname.c > +++ b/fs/ext4/crypto_fname.c > @@ -252,52 +252,6 @@ static int digest_decode(const char *src, int len, char *dst) > return cp - dst; > } > > -int ext4_setup_fname_crypto(struct inode *inode) > -{ > - struct ext4_inode_info *ei = EXT4_I(inode); > - struct ext4_crypt_info *ci = ei->i_crypt_info; > - struct crypto_ablkcipher *ctfm; > - int res; > - > - /* Check if the crypto policy is set on the inode */ > - res = ext4_encrypted_inode(inode); > - if (res == 0) > - return 0; > - > - res = ext4_get_encryption_info(inode); > - if (res < 0) > - return res; > - ci = ei->i_crypt_info; > - > - if (!ci || ci->ci_ctfm) > - return 0; > - > - if (ci->ci_filename_mode != EXT4_ENCRYPTION_MODE_AES_256_CTS) { > - printk_once(KERN_WARNING "ext4: unsupported key mode %d\n", > - ci->ci_filename_mode); > - return -ENOKEY; > - } > - > - ctfm = crypto_alloc_ablkcipher("cts(cbc(aes))", 0, 0); > - if (!ctfm || IS_ERR(ctfm)) { > - res = ctfm ? PTR_ERR(ctfm) : -ENOMEM; > - printk(KERN_DEBUG "%s: error (%d) allocating crypto tfm\n", > - __func__, res); > - return res; > - } > - crypto_ablkcipher_clear_flags(ctfm, ~0); > - crypto_tfm_set_flags(crypto_ablkcipher_tfm(ctfm), > - CRYPTO_TFM_REQ_WEAK_KEY); > - > - res = crypto_ablkcipher_setkey(ctfm, ci->ci_raw, ci->ci_size); > - if (res) { > - crypto_free_ablkcipher(ctfm); > - return -EIO; > - } > - ci->ci_ctfm = ctfm; > - return 0; > -} > - > /** > * ext4_fname_crypto_round_up() - > * > @@ -449,7 +403,7 @@ int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname, > fname->disk_name.len = iname->len; > goto out; > } > - ret = ext4_setup_fname_crypto(dir); > + ret = ext4_get_encryption_info(dir); > if (ret) > return ret; > ci = EXT4_I(dir)->i_crypt_info; > diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c > index 858d7d6..442d24e 100644 > --- a/fs/ext4/crypto_key.c > +++ b/fs/ext4/crypto_key.c > @@ -84,20 +84,32 @@ out: > return res; > } > > -void ext4_free_encryption_info(struct inode *inode) > +void ext4_free_crypt_info(struct ext4_crypt_info *ci) > { > - struct ext4_inode_info *ei = EXT4_I(inode); > - struct ext4_crypt_info *ci = ei->i_crypt_info; > - > if (!ci) > return; > > if (ci->ci_keyring_key) > key_put(ci->ci_keyring_key); > crypto_free_ablkcipher(ci->ci_ctfm); > - memzero_explicit(&ci->ci_raw, sizeof(ci->ci_raw)); > kmem_cache_free(ext4_crypt_info_cachep, ci); > - ei->i_crypt_info = NULL; > +} > + > +void ext4_free_encryption_info(struct inode *inode, > + struct ext4_crypt_info *ci) > +{ > + struct ext4_inode_info *ei = EXT4_I(inode); > + struct ext4_crypt_info *prev; > + > + if (ci == NULL) > + ci = ACCESS_ONCE(ei->i_crypt_info); > + if (ci == NULL) > + return; > + prev = cmpxchg(&ei->i_crypt_info, ci, NULL); > + if (prev != ci) > + return; > + > + ext4_free_crypt_info(ci); > } > > int _ext4_get_encryption_info(struct inode *inode) > @@ -111,6 +123,10 @@ int _ext4_get_encryption_info(struct inode *inode) > struct ext4_encryption_context ctx; > struct user_key_payload *ukp; > struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); > + struct crypto_ablkcipher *ctfm; > + const char *cipher_str; > + char raw_key[EXT4_MAX_KEY_SIZE]; > + char mode; > int res; > > if (!ext4_read_workqueue) { > @@ -119,11 +135,14 @@ int _ext4_get_encryption_info(struct inode *inode) > return res; > } > > - if (ei->i_crypt_info) { > - if (!ei->i_crypt_info->ci_keyring_key || > - key_validate(ei->i_crypt_info->ci_keyring_key) == 0) > +retry: > + crypt_info = ACCESS_ONCE(ei->i_crypt_info); > + if (crypt_info) { > + if (!crypt_info->ci_keyring_key || > + key_validate(crypt_info->ci_keyring_key) == 0) > return 0; > - ext4_free_encryption_info(inode); > + ext4_free_encryption_info(inode, crypt_info); > + goto retry; > } > > res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION, > @@ -144,26 +163,37 @@ int _ext4_get_encryption_info(struct inode *inode) > if (!crypt_info) > return -ENOMEM; > > - ei->i_crypt_policy_flags = ctx.flags; > crypt_info->ci_flags = ctx.flags; > crypt_info->ci_data_mode = ctx.contents_encryption_mode; > crypt_info->ci_filename_mode = ctx.filenames_encryption_mode; > crypt_info->ci_ctfm = NULL; > + crypt_info->ci_keyring_key = NULL; > memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor, > sizeof(crypt_info->ci_master_key)); > if (S_ISREG(inode->i_mode)) > - crypt_info->ci_size = > - ext4_encryption_key_size(crypt_info->ci_data_mode); > + mode = crypt_info->ci_data_mode; > else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) > - crypt_info->ci_size = > - ext4_encryption_key_size(crypt_info->ci_filename_mode); > + mode = crypt_info->ci_filename_mode; > else > BUG(); > - BUG_ON(!crypt_info->ci_size); > - if (DUMMY_ENCRYPTION_ENABLED(sbi)) { > - memset(crypt_info->ci_raw, 0x42, EXT4_AES_256_XTS_KEY_SIZE); > + switch (mode) { > + case EXT4_ENCRYPTION_MODE_AES_256_XTS: > + cipher_str = "xts(aes)"; > + break; > + case EXT4_ENCRYPTION_MODE_AES_256_CTS: > + cipher_str = "cts(cbc(aes))"; > + break; > + default: > + printk_once(KERN_WARNING > + "ext4: unsupported key mode %d (ino %u)\n", > + mode, (unsigned) inode->i_ino); > + res = -ENOKEY; > goto out; > } > + if (DUMMY_ENCRYPTION_ENABLED(sbi)) { > + memset(raw_key, 0x42, EXT4_AES_256_XTS_KEY_SIZE); > + goto got_key; > + } > memcpy(full_key_descriptor, EXT4_KEY_DESC_PREFIX, > EXT4_KEY_DESC_PREFIX_SIZE); > sprintf(full_key_descriptor + EXT4_KEY_DESC_PREFIX_SIZE, > @@ -177,6 +207,7 @@ int _ext4_get_encryption_info(struct inode *inode) > keyring_key = NULL; > goto out; > } > + crypt_info->ci_keyring_key = keyring_key; > BUG_ON(keyring_key->type != &key_type_logon); > ukp = ((struct user_key_payload *)keyring_key->payload.data); > if (ukp->datalen != sizeof(struct ext4_encryption_key)) { > @@ -188,19 +219,36 @@ int _ext4_get_encryption_info(struct inode *inode) > EXT4_KEY_DERIVATION_NONCE_SIZE); > BUG_ON(master_key->size != EXT4_AES_256_XTS_KEY_SIZE); > res = ext4_derive_key_aes(ctx.nonce, master_key->raw, > - crypt_info->ci_raw); > -out: > - if (res < 0) { > - if (res == -ENOKEY) > - res = 0; > - kmem_cache_free(ext4_crypt_info_cachep, crypt_info); > - } else { > - ei->i_crypt_info = crypt_info; > - crypt_info->ci_keyring_key = keyring_key; > - keyring_key = NULL; > + raw_key); > +got_key: > + ctfm = crypto_alloc_ablkcipher(cipher_str, 0, 0); > + if (!ctfm || IS_ERR(ctfm)) { > + res = ctfm ? PTR_ERR(ctfm) : -ENOMEM; > + printk(KERN_DEBUG > + "%s: error %d (inode %u) allocating crypto tfm\n", > + __func__, res, (unsigned) inode->i_ino); > + goto out; > + } > + crypt_info->ci_ctfm = ctfm; > + crypto_ablkcipher_clear_flags(ctfm, ~0); > + crypto_tfm_set_flags(crypto_ablkcipher_tfm(ctfm), > + CRYPTO_TFM_REQ_WEAK_KEY); > + res = crypto_ablkcipher_setkey(ctfm, raw_key, > + ext4_encryption_key_size(mode)); > + if (res) > + goto out; > + memzero_explicit(raw_key, sizeof(raw_key)); > + if (cmpxchg(&ei->i_crypt_info, NULL, crypt_info) != NULL) { > + ext4_free_crypt_info(crypt_info); > + goto retry; > } > - if (keyring_key) > - key_put(keyring_key); > + return 0; > + > +out: > + if (res == -ENOKEY) > + res = 0; > + ext4_free_crypt_info(crypt_info); > + memzero_explicit(raw_key, sizeof(raw_key)); > return res; > } > > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index 28cb94f..e11e6ae 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -133,9 +133,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) > return err; > } > > - err = ext4_setup_fname_crypto(inode); > - if (err) > - return err; > if (ext4_encrypted_inode(inode)) { > err = ext4_fname_crypto_alloc_buffer(inode, EXT4_NAME_LEN, > &fname_crypto_str); > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 23e33fb..7435ff2 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -911,7 +911,6 @@ struct ext4_inode_info { > > /* on-disk additional length */ > __u16 i_extra_isize; > - char i_crypt_policy_flags; > > /* Indicate the inline data space. */ > u16 i_inline_off; > @@ -2105,7 +2104,6 @@ int ext4_fname_usr_to_disk(struct inode *inode, > const struct qstr *iname, > struct ext4_str *oname); > #ifdef CONFIG_EXT4_FS_ENCRYPTION > -int ext4_setup_fname_crypto(struct inode *inode); > void ext4_fname_crypto_free_buffer(struct ext4_str *crypto_str); > int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname, > int lookup, struct ext4_filename *fname); > @@ -2131,7 +2129,8 @@ static inline void ext4_fname_free_filename(struct ext4_filename *fname) { } > > > /* crypto_key.c */ > -void ext4_free_encryption_info(struct inode *inode); > +void ext4_free_crypt_info(struct ext4_crypt_info *ci); > +void ext4_free_encryption_info(struct inode *inode, struct ext4_crypt_info *ci); > int _ext4_get_encryption_info(struct inode *inode); > > #ifdef CONFIG_EXT4_FS_ENCRYPTION > diff --git a/fs/ext4/ext4_crypto.h b/fs/ext4/ext4_crypto.h > index c5258f2..34e0d24 100644 > --- a/fs/ext4/ext4_crypto.h > +++ b/fs/ext4/ext4_crypto.h > @@ -74,13 +74,11 @@ struct ext4_encryption_key { > } __attribute__((__packed__)); > > struct ext4_crypt_info { > - unsigned char ci_size; > char ci_data_mode; > char ci_filename_mode; > char ci_flags; > struct crypto_ablkcipher *ci_ctfm; > struct key *ci_keyring_key; > - char ci_raw[EXT4_MAX_KEY_SIZE]; > char ci_master_key[EXT4_KEY_DESCRIPTOR_SIZE]; > }; > > @@ -89,7 +87,6 @@ struct ext4_crypt_info { > #define EXT4_WRITE_PATH_FL 0x00000004 > > struct ext4_crypto_ctx { > - struct crypto_tfm *tfm; /* Crypto API context */ > union { > struct { > struct page *bounce_page; /* Ciphertext page */ > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 9bed99f..6ab50f8 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -607,11 +607,12 @@ static struct stats dx_show_leaf(struct inode *dir, > char *name; > struct ext4_str fname_crypto_str > = {.name = NULL, .len = 0}; > - int res; > + int res = 0; > > name = de->name; > len = de->name_len; > - res = ext4_setup_fname_crypto(dir); > + if (ext4_encrypted_inode(inode)) > + res = ext4_get_encryption_info(dir); > if (res) { > printk(KERN_WARNING "Error setting up" > " fname crypto: %d\n", res); > @@ -953,12 +954,12 @@ static int htree_dirblock_to_tree(struct file *dir_file, > EXT4_DIR_REC_LEN(0)); > #ifdef CONFIG_EXT4_FS_ENCRYPTION > /* Check if the directory is encrypted */ > - err = ext4_setup_fname_crypto(dir); > - if (err) { > - brelse(bh); > - return err; > - } > if (ext4_encrypted_inode(dir)) { > + err = ext4_get_encryption_info(dir); > + if (err < 0) { > + brelse(bh); > + return err; > + } > err = ext4_fname_crypto_alloc_buffer(dir, EXT4_NAME_LEN, > &fname_crypto_str); > if (err < 0) { > @@ -3108,7 +3109,7 @@ static int ext4_symlink(struct inode *dir, > err = ext4_inherit_context(dir, inode); > if (err) > goto err_drop_inode; > - err = ext4_setup_fname_crypto(inode); > + err = ext4_get_encryption_info(inode); > if (err) > goto err_drop_inode; > istr.name = (const unsigned char *) symname; > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index b0bd1c1..56bfc2f 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -959,7 +959,7 @@ void ext4_clear_inode(struct inode *inode) > } > #ifdef CONFIG_EXT4_FS_ENCRYPTION > if (EXT4_I(inode)->i_crypt_info) > - ext4_free_encryption_info(inode); > + ext4_free_encryption_info(inode, EXT4_I(inode)->i_crypt_info); > #endif > } > > diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c > index 3287088..68e915a 100644 > --- a/fs/ext4/symlink.c > +++ b/fs/ext4/symlink.c > @@ -37,7 +37,7 @@ static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd) > if (!ext4_encrypted_inode(inode)) > return page_follow_link_light(dentry, nd); > > - res = ext4_setup_fname_crypto(inode); > + res = ext4_get_encryption_info(inode); > if (res) > return ERR_PTR(res); > > -- > 2.3.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs crypto: add rwsem to avoid data races 2015-05-20 4:55 ` Jaegeuk Kim @ 2015-05-20 12:38 ` Theodore Ts'o 0 siblings, 0 replies; 7+ messages in thread From: Theodore Ts'o @ 2015-05-20 12:38 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: nick, linux-kernel, linux-fsdevel, linux-f2fs-devel On Tue, May 19, 2015 at 09:55:54PM -0700, Jaegeuk Kim wrote: > > Looking at a glance, it's mostly same as what I wanted. The key is to share > ci->ci_ctfm for regular file and the other dir/symlink files. > So, ext4_get_encryption_info will handle most of cases. Yeah, I noticed after sending out my changes that your patches did something really similar in that regard. "Great minds run in the same channels", as the (very) old saying goes... > In ext4_readdir, it also needs ext4_get_encryption_info? No, it not needed because when the directory is opened using opendir(3), we'll end up calling ext4_get_encryption_info() in ext4_file_open() --- yes, the name is a little misleading, but it gets used for all ext4 open(2)'s. One of the things which I've been mildly concerned about is that in the no key case, we end up calling ext4_get_encrpyion_info() over and over again, which means we end up calling request_key() over and over again as well. On the other hand, we do want to use the key as soon as it becomes available, there isn't a good way of doing a negative cache on the unavailability of the logon key that could be reliably revoked the moment the user does have the key --- and the no-key case isn't the common one, so I haven't worried too much about it. But I did want to remove it from the readdir() path, since it also means that we don't have to worry about oddities stemming from half the files being returned in encrypted form, and half the files in plaintext, because the user inserted an encryption key in the while there was an "ls" running in the background. Cheers, - Ted ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-05-20 12:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-19 5:36 [PATCH] f2fs crypto: add rwsem to avoid data races Jaegeuk Kim
2015-05-19 14:29 ` Theodore Ts'o
2015-05-19 17:42 ` Jaegeuk Kim
[not found] ` <555B4A32.3000302@gmail.com>
2015-05-20 0:38 ` [f2fs-dev] " Jaegeuk Kim
2015-05-20 4:35 ` Theodore Ts'o
2015-05-20 4:55 ` Jaegeuk Kim
2015-05-20 12:38 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).