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>

diff --git a/fs/inode.c b/fs/inode.c
index 901bad1e5f1210cce8f2bc2a474345f5b1a4072a..af2c05235cc80c4c6025740804e320bb4b5febcc 100644 (file)
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -120,12 +120,11 @@ static void wake_up_inode(struct inode *inode)
  * 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 @@ struct inode *inode_init_always(struct super_block *sb, struct inode *inode)
        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 @@ struct inode *inode_init_always(struct super_block *sb, struct inode *inode)
        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 @@ static struct inode *alloc_inode(struct super_block *sb)
        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)

diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 5fcec6f020a7ddff25df7e54e58750ca651733b1..719c85b155f440d34e7dc882a3e200cbb022a644 100644 (file)
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -64,6 +64,10 @@ xfs_inode_alloc(
        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 @@ xfs_inode_alloc(
 #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 @@ xfs_iget_cache_hit(
                 * 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;
                }

diff --git a/include/linux/fs.h b/include/linux/fs.h
index a36ffa5a77a4b2cbc45628e5eda30d4071828f6a..0c3b5e58a986f35372ec13c48b3a949dcfd87eee 100644 (file)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2137,7 +2137,7 @@ extern loff_t default_llseek(struct file *file, loff_t offset, int origin);
 
 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 *);