[PATCH 2/4] f2fs crypto: add two spinlocks to avoid data races

[Jaegeuk Kim]

Previoulsy, fi->i_crypt_info and tfm allocation were not covered by any lock,
resulting in memory leak when multiple threads try to allocate at the same time.

=============================================================================
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

This patch adds one rwlock and one spinlock to avoid leaking objects.

Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
---
 fs/f2fs/crypto.c       | 11 ++++++++++-
 fs/f2fs/crypto_fname.c | 10 +++++++++-
 fs/f2fs/crypto_key.c   | 32 +++++++++++++++++++++++---------
 fs/f2fs/f2fs.h         |  2 ++
 fs/f2fs/super.c        |  2 ++
 5 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/crypto.c b/fs/f2fs/crypto.c
index c0c5fd2..5a614fe 100644
--- a/fs/f2fs/crypto.c
+++ b/fs/f2fs/crypto.c
@@ -104,6 +104,7 @@ int f2fs_setup_crypto(struct inode *inode)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	struct f2fs_crypt_info *ci;
 	struct crypto_ablkcipher *ctfm;
+	bool free = false;
 	int res;
 
 	res = f2fs_get_encryption_info(inode);
@@ -140,7 +141,15 @@ int f2fs_setup_crypto(struct inode *inode)
 		crypto_free_ablkcipher(ctfm);
 		return -EIO;
 	}
-	ci->ci_ctfm = ctfm;
+
+	spin_lock(&fi->crypto_tfmlock);
+	if (ci->ci_ctfm)
+		free = true;
+	else
+		ci->ci_ctfm = ctfm;
+	spin_unlock(&fi->crypto_tfmlock);
+	if (free)
+		crypto_free_ablkcipher(ctfm);
 	return 0;
 }
 
diff --git a/fs/f2fs/crypto_fname.c b/fs/f2fs/crypto_fname.c
index 016c4b6..dd73c81 100644
--- a/fs/f2fs/crypto_fname.c
+++ b/fs/f2fs/crypto_fname.c
@@ -254,6 +254,7 @@ int f2fs_setup_fname_crypto(struct inode *inode)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	struct f2fs_crypt_info *ci = fi->i_crypt_info;
 	struct crypto_ablkcipher *ctfm;
+	bool free = false;
 	int res;
 
 	/* Check if the crypto policy is set on the inode */
@@ -291,7 +292,14 @@ int f2fs_setup_fname_crypto(struct inode *inode)
 		crypto_free_ablkcipher(ctfm);
 		return -EIO;
 	}
-	ci->ci_ctfm = ctfm;
+	spin_lock(&fi->crypto_tfmlock);
+	if (ci->ci_ctfm)
+		free = true;
+	else
+		ci->ci_ctfm = ctfm;
+	spin_unlock(&fi->crypto_tfmlock);
+	if (free)
+		crypto_free_ablkcipher(ctfm);
 	return 0;
 }
 
diff --git a/fs/f2fs/crypto_key.c b/fs/f2fs/crypto_key.c
index 8a10569..2f5a45b 100644
--- a/fs/f2fs/crypto_key.c
+++ b/fs/f2fs/crypto_key.c
@@ -114,17 +114,19 @@ int _f2fs_get_encryption_info(struct inode *inode)
 	struct f2fs_encryption_context ctx;
 	struct user_key_payload *ukp;
 	int res;
+	bool drop = false;
 
 	res = f2fs_crypto_initialize();
 	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);
+	read_lock(&fi->crypto_lock);
+	if (fi->i_crypt_info && (!fi->i_crypt_info->ci_keyring_key ||
+			key_validate(fi->i_crypt_info->ci_keyring_key) == 0)) {
+		read_unlock(&fi->crypto_lock);
+		return 0;
 	}
+	read_unlock(&fi->crypto_lock);
 
 	res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION,
 				F2FS_XATTR_NAME_ENCRYPTION_CONTEXT,
@@ -187,18 +189,30 @@ out:
 			res = 0;
 		kmem_cache_free(f2fs_crypt_info_cachep, crypt_info);
 	} else {
-		fi->i_crypt_info = crypt_info;
-		crypt_info->ci_keyring_key = keyring_key;
-		keyring_key = NULL;
+		write_lock(&fi->crypto_lock);
+		if (fi->i_crypt_info) {
+			drop = true;
+		} else {
+			fi->i_crypt_info = crypt_info;
+			crypt_info->ci_keyring_key = keyring_key;
+			keyring_key = NULL;
+		}
+		write_unlock(&fi->crypto_lock);
 	}
 	if (keyring_key)
 		key_put(keyring_key);
+	if (drop)
+		kmem_cache_free(f2fs_crypt_info_cachep, crypt_info);
 	return res;
 }
 
 int f2fs_has_encryption_key(struct inode *inode)
 {
 	struct f2fs_inode_info *fi = F2FS_I(inode);
+	int ret;
 
-	return (fi->i_crypt_info != NULL);
+	read_lock(&fi->crypto_lock);
+	ret = (fi->i_crypt_info != NULL);
+	read_unlock(&fi->crypto_lock);
+	return ret;
 }
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 801afcb..34a968b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -431,6 +431,8 @@ struct f2fs_inode_info {
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
 	/* Encryption params */
 	struct f2fs_crypt_info *i_crypt_info;
+	rwlock_t crypto_lock;		/* lock for crypt_info */
+	spinlock_t crypto_tfmlock;	/* lock for crypto tfm allocation */
 #endif
 };
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index bbeb6d7..ae14fc4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -418,6 +418,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
 	fi->i_crypt_info = NULL;
+	rwlock_init(&fi->crypto_lock);
+	spin_lock_init(&fi->crypto_tfmlock);
 #endif
 	return &fi->vfs_inode;
 }
-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/