vfs: fix inode_init_always calling convention

Currently inode_init_always calls into ->destroy_inode if the additional initialization fails. That's not only counter-intuitive because inode_init_always did not allocate the inode structure, but in case of XFS it's actively harmful as ->destroy_inode might delete the inode from a radix-tree that has never been added. This in turn might end up deleting the inode for the same inum that has been instanciated by another process and cause lots of cause subtile problems. Also in the case of re-initializing a reclaimable inode in XFS it would free an inode we still want to keep alive. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

vfs: fix inode_init_always calling convention
Currently inode_init_always calls into ->destroy_inode if the additional initialization fails. That's not only counter-intuitive because inode_init_always did not allocate the inode structure, but in case of XFS it's actively harmful as ->destroy_inode might delete the inode from a radix-tree that has never been added. This in turn might end up deleting the inode for the same inum that has been instanciated by another process and cause lots of cause subtile problems. Also in the case of re-initializing a reclaimable inode in XFS it would free an inode we still want to keep alive. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Eric Sandeen <sandeen@sandeen.net>
Christoph Hellwig · Christoph Hellwig
1 parent 90bc1a658a
Showing 3 changed files with 23 additions and 26 deletions Side-by-side Diff
fs/inode.c
fs/xfs/xfs_iget.c
include/linux/fs.h
@@ -120,12 +120,11 @@
  * These are initializations that need to be done on every inode
  * allocation as the fields are not initialised by slab allocation.
  */
-struct inode *inode_init_always(struct super_block *sb, struct inode *inode)
+int inode_init_always(struct super_block *sb, struct inode *inode)
 {
 	static const struct address_space_operations empty_aops;
 	static struct inode_operations empty_iops;
 	static const struct file_operations empty_fops;
-
 	struct address_space *const mapping = &inode->i_data;
  
 	inode->i_sb = sb;
@@ -152,7 +151,7 @@
 	inode->dirtied_when = 0;
  
 	if (security_inode_alloc(inode))
-		goto out_free_inode;
+		goto out;
  
 	/* allocate and initialize an i_integrity */
 	if (ima_inode_alloc(inode))
  
@@ -198,16 +197,12 @@
 	inode->i_fsnotify_mask = 0;
 #endif
  
-	return inode;
+	return 0;
  
 out_free_security:
 	security_inode_free(inode);
-out_free_inode:
-	if (inode->i_sb->s_op->destroy_inode)
-		inode->i_sb->s_op->destroy_inode(inode);
-	else
-		kmem_cache_free(inode_cachep, (inode));
-	return NULL;
+out:
+	return -ENOMEM;
 }
 EXPORT_SYMBOL(inode_init_always);
  
@@ -220,9 +215,18 @@
 	else
 		inode = kmem_cache_alloc(inode_cachep, GFP_KERNEL);
  
-	if (inode)
-		return inode_init_always(sb, inode);
-	return NULL;
+	if (!inode)
+		return NULL;
+
+	if (unlikely(inode_init_always(sb, inode))) {
+		if (inode->i_sb->s_op->destroy_inode)
+			inode->i_sb->s_op->destroy_inode(inode);
+		else
+			kmem_cache_free(inode_cachep, inode);
+		return NULL;
+	}
+
+	return inode;
 }
  
 void destroy_inode(struct inode *inode)
@@ -64,6 +64,10 @@
 	ip = kmem_zone_alloc(xfs_inode_zone, KM_SLEEP);
 	if (!ip)
 		return NULL;
+	if (inode_init_always(mp->m_super, VFS_I(ip))) {
+		kmem_zone_free(xfs_inode_zone, ip);
+		return NULL;
+	}
  
 	ASSERT(atomic_read(&ip->i_iocount) == 0);
 	ASSERT(atomic_read(&ip->i_pincount) == 0);
@@ -105,17 +109,6 @@
 #ifdef XFS_DIR2_TRACE
 	ip->i_dir_trace = ktrace_alloc(XFS_DIR2_KTRACE_SIZE, KM_NOFS);
 #endif
-	/*
-	* Now initialise the VFS inode. We do this after the xfs_inode
-	* initialisation as internal failures will result in ->destroy_inode
-	* being called and that will pass down through the reclaim path and
-	* free the XFS inode. This path requires the XFS inode to already be
-	* initialised. Hence if this call fails, the xfs_inode has already
-	* been freed and we should not reference it at all in the error
-	* handling.
-	*/
-	if (!inode_init_always(mp->m_super, VFS_I(ip)))
-		return NULL;
  
 	/* prevent anyone from using this yet */
 	VFS_I(ip)->i_state = I_NEW|I_LOCK;
@@ -167,7 +160,7 @@
 		 * errors cleanly, then tag it so it can be set up correctly
 		 * later.
 		 */
-		if (!inode_init_always(mp->m_super, VFS_I(ip))) {
+		if (inode_init_always(mp->m_super, VFS_I(ip))) {
 			error = ENOMEM;
 			goto out_error;
 		}
@@ -2137,7 +2137,7 @@
  
 extern loff_t vfs_llseek(struct file *file, loff_t offset, int origin);
  
-extern struct inode * inode_init_always(struct super_block *, struct inode *);
+extern int inode_init_always(struct super_block *, struct inode *);
 extern void inode_init_once(struct inode *);
 extern void inode_add_to_lists(struct super_block *, struct inode *);
 extern void iput(struct inode *);
...	...	@@ -120,12 +120,11 @@
120	120	* These are initializations that need to be done on every inode
121	121	* allocation as the fields are not initialised by slab allocation.
122	122	*/
123		-struct inode inode_init_always(struct super_block sb, struct inode *inode)
	123	+int inode_init_always(struct super_block sb, struct inode inode)
124	124	{
125	125	static const struct address_space_operations empty_aops;
126	126	static struct inode_operations empty_iops;
127	127	static const struct file_operations empty_fops;
128		-
129	128	struct address_space *const mapping = &inode->i_data;
130	129
131	130	inode->i_sb = sb;
...	...	@@ -152,7 +151,7 @@
152	151	inode->dirtied_when = 0;
153	152
154	153	if (security_inode_alloc(inode))
155		- goto out_free_inode;
	154	+ goto out;
156	155
157	156	/* allocate and initialize an i_integrity */
158	157	if (ima_inode_alloc(inode))
159	158
...	...	@@ -198,16 +197,12 @@
198	197	inode->i_fsnotify_mask = 0;
199	198	#endif
200	199
201		- return inode;
	200	+ return 0;
202	201
203	202	out_free_security:
204	203	security_inode_free(inode);
205		-out_free_inode:
206		- if (inode->i_sb->s_op->destroy_inode)
207		- inode->i_sb->s_op->destroy_inode(inode);
208		- else
209		- kmem_cache_free(inode_cachep, (inode));
210		- return NULL;
	204	+out:
	205	+ return -ENOMEM;
211	206	}
212	207	EXPORT_SYMBOL(inode_init_always);
213	208
...	...	@@ -220,9 +215,18 @@
220	215	else
221	216	inode = kmem_cache_alloc(inode_cachep, GFP_KERNEL);
222	217
223		- if (inode)
224		- return inode_init_always(sb, inode);
225		- return NULL;
	218	+ if (!inode)
	219	+ return NULL;
	220	+
	221	+ if (unlikely(inode_init_always(sb, inode))) {
	222	+ if (inode->i_sb->s_op->destroy_inode)
	223	+ inode->i_sb->s_op->destroy_inode(inode);
	224	+ else
	225	+ kmem_cache_free(inode_cachep, inode);
	226	+ return NULL;
	227	+ }
	228	+
	229	+ return inode;
226	230	}
227	231
228	232	void destroy_inode(struct inode *inode)
...	...	@@ -64,6 +64,10 @@
64	64	ip = kmem_zone_alloc(xfs_inode_zone, KM_SLEEP);
65	65	if (!ip)
66	66	return NULL;
	67	+ if (inode_init_always(mp->m_super, VFS_I(ip))) {
	68	+ kmem_zone_free(xfs_inode_zone, ip);
	69	+ return NULL;
	70	+ }
67	71
68	72	ASSERT(atomic_read(&ip->i_iocount) == 0);
69	73	ASSERT(atomic_read(&ip->i_pincount) == 0);
...	...	@@ -105,17 +109,6 @@
105	109	#ifdef XFS_DIR2_TRACE
106	110	ip->i_dir_trace = ktrace_alloc(XFS_DIR2_KTRACE_SIZE, KM_NOFS);
107	111	#endif
108		- /*
109		- * Now initialise the VFS inode. We do this after the xfs_inode
110		- * initialisation as internal failures will result in ->destroy_inode
111		- * being called and that will pass down through the reclaim path and
112		- * free the XFS inode. This path requires the XFS inode to already be
113		- * initialised. Hence if this call fails, the xfs_inode has already
114		- * been freed and we should not reference it at all in the error
115		- * handling.
116		- */
117		- if (!inode_init_always(mp->m_super, VFS_I(ip)))
118		- return NULL;
119	112
120	113	/* prevent anyone from using this yet */
121	114	VFS_I(ip)->i_state = I_NEW\|I_LOCK;
...	...	@@ -167,7 +160,7 @@
167	160	* errors cleanly, then tag it so it can be set up correctly
168	161	* later.
169	162	*/
170		- if (!inode_init_always(mp->m_super, VFS_I(ip))) {
	163	+ if (inode_init_always(mp->m_super, VFS_I(ip))) {
171	164	error = ENOMEM;
172	165	goto out_error;
173	166	}
...	...	@@ -2137,7 +2137,7 @@
2137	2137
2138	2138	extern loff_t vfs_llseek(struct file *file, loff_t offset, int origin);
2139	2139
2140		-extern struct inode * inode_init_always(struct super_block , struct inode );
	2140	+extern int inode_init_always(struct super_block , struct inode );
2141	2141	extern void inode_init_once(struct inode *);
2142	2142	extern void inode_add_to_lists(struct super_block , struct inode );
2143	2143	extern void iput(struct inode *);