| Message ID | 1432013801-39069-1-git-send-email-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)