gfs2: Fix gfs2_lookup_by_inum lock inversion

The current gfs2_lookup_by_inum takes the glock of a presumed inode
identified by block number, verifies that the block is indeed an inode,
and then instantiates and reads the new inode via gfs2_inode_lookup.

However, instantiating a new inode may block on freeing a previous
instance of that inode (__wait_on_freeing_inode), and freeing an inode
requires to take the glock already held, leading to lock inversion and
deadlock.

Fix this by first instantiating the new inode, then verifying that the
block is an inode (if required), and then reading in the new inode, all
in gfs2_inode_lookup.

If the block we are looking for is not an inode, we discard the new
inode via iget_failed, which marks inodes as bad and unhashes them.
Other tasks waiting on that inode will get back a bad inode back from
ilookup or iget_locked; in that case, retry the lookup.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Bob Peterson <rpeterso@redhat.com>

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 4a01f30e99954616811a932dc1f54644f0e0f68d..1b02665357b22ae7a514ae028d2f9c68af5eeaa5 100644 (file)
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -1660,7 +1660,8 @@ struct inode *gfs2_dir_search(struct inode *dir, const struct qstr *name,
                brelse(bh);
                if (fail_on_exist)
                        return ERR_PTR(-EEXIST);
-               inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino);
+               inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino,
+                                         GFS2_BLKST_FREE /* ignore */);
                if (!IS_ERR(inode))
                        GFS2_I(inode)->i_rahead = rahead;
                return inode;

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 706fd9352f368818391ad79f4481feffe0809b60..ce4637518d3b6aa8af5c8dbb0caae4e43ae9127a 100644 (file)
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -576,7 +576,7 @@ static void delete_work_func(struct work_struct *work)
        struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
        struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
        struct gfs2_inode *ip;
-       struct inode *inode;
+       struct inode *inode = NULL;
        u64 no_addr = gl->gl_name.ln_number;
 
        /* If someone's using this glock to create a new dinode, the block must
@@ -590,7 +590,7 @@ static void delete_work_func(struct work_struct *work)
 
        if (ip)
                inode = gfs2_ilookup(sdp->sd_vfs, no_addr);
-       else
+       if (IS_ERR_OR_NULL(inode))
                inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
        if (inode && !IS_ERR(inode)) {
                d_prune_aliases(inode);

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 21dc784f66c2268d2e857314104847b600cc09de..6d5c6bbec41613db654d59e38055649f92a2651f 100644 (file)
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -39,7 +39,33 @@
 
 struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr)
 {
-       return ilookup(sb, (unsigned long)no_addr);
+       struct inode *inode;
+
+repeat:
+       inode = ilookup(sb, no_addr);
+       if (!inode)
+               return inode;
+       if (is_bad_inode(inode)) {
+               iput(inode);
+               goto repeat;
+       }
+       return inode;
+}
+
+static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr)
+{
+       struct inode *inode;
+
+repeat:
+       inode = iget_locked(sb, no_addr);
+       if (!inode)
+               return inode;
+       if (is_bad_inode(inode)) {
+               iput(inode);
+               goto repeat;
+       }
+       GFS2_I(inode)->i_no_addr = no_addr;
+       return inode;
 }
 
 /**
@@ -78,26 +104,37 @@ static void gfs2_set_iop(struct inode *inode)
 /**
  * gfs2_inode_lookup - Lookup an inode
  * @sb: The super block
- * @no_addr: The inode number
  * @type: The type of the inode
+ * @no_addr: The inode number
+ * @no_formal_ino: The inode generation number
+ * @blktype: Requested block type (GFS2_BLKST_DINODE or GFS2_BLKST_UNLINKED;
+ *           GFS2_BLKST_FREE do indicate not to verify)
+ *
+ * If @type is DT_UNKNOWN, the inode type is fetched from disk.
+ *
+ * If @blktype is anything other than GFS2_BLKST_FREE (which is used as a
+ * placeholder because it doesn't otherwise make sense), the on-disk block type
+ * is verified to be @blktype.
  *
  * Returns: A VFS inode, or an error
  */
 
 struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
-                               u64 no_addr, u64 no_formal_ino)
+                               u64 no_addr, u64 no_formal_ino,
+                               unsigned int blktype)
 {
        struct inode *inode;
        struct gfs2_inode *ip;
        struct gfs2_glock *io_gl = NULL;
+       struct gfs2_holder i_gh;
+       bool unlock = false;
        int error;
 
-       inode = iget_locked(sb, (unsigned long)no_addr);
+       inode = gfs2_iget(sb, no_addr);
        if (!inode)
                return ERR_PTR(-ENOMEM);
 
        ip = GFS2_I(inode);
-       ip->i_no_addr = no_addr;
 
        if (inode->i_state & I_NEW) {
                struct gfs2_sbd *sdp = GFS2_SB(inode);
@@ -112,10 +149,30 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
                if (unlikely(error))
                        goto fail_put;
 
+               if (type == DT_UNKNOWN || blktype != GFS2_BLKST_FREE) {
+                       /*
+                        * The GL_SKIP flag indicates to skip reading the inode
+                        * block.  We read the inode with gfs2_inode_refresh
+                        * after possibly checking the block type.
+                        */
+                       error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE,
+                                                  GL_SKIP, &i_gh);
+                       if (error)
+                               goto fail_put;
+                       unlock = true;
+
+                       if (blktype != GFS2_BLKST_FREE) {
+                               error = gfs2_check_blk_type(sdp, no_addr,
+                                                           blktype);
+                               if (error)
+                                       goto fail_put;
+                       }
+               }
+
                set_bit(GIF_INVALID, &ip->i_flags);
                error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT, &ip->i_iopen_gh);
                if (unlikely(error))
-                       goto fail_iopen;
+                       goto fail_put;
 
                ip->i_iopen_gh.gh_gl->gl_object = ip;
                gfs2_glock_put(io_gl);
@@ -134,6 +191,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
                unlock_new_inode(inode);
        }
 
+       if (unlock)
+               gfs2_glock_dq_uninit(&i_gh);
        return inode;
 
 fail_refresh:
@@ -141,10 +200,11 @@ fail_refresh:
        ip->i_iopen_gh.gh_gl->gl_object = NULL;
        gfs2_glock_dq_wait(&ip->i_iopen_gh);
        gfs2_holder_uninit(&ip->i_iopen_gh);
-fail_iopen:
+fail_put:
        if (io_gl)
                gfs2_glock_put(io_gl);
-fail_put:
+       if (unlock)
+               gfs2_glock_dq_uninit(&i_gh);
        ip->i_gl->gl_object = NULL;
 fail:
        iget_failed(inode);
@@ -155,23 +215,12 @@ struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
                                  u64 *no_formal_ino, unsigned int blktype)
 {
        struct super_block *sb = sdp->sd_vfs;
-       struct gfs2_holder i_gh;
-       struct inode *inode = NULL;
+       struct inode *inode;
        int error;
 
-       /* Must not read in block until block type is verified */
-       error = gfs2_glock_nq_num(sdp, no_addr, &gfs2_inode_glops,
-                                 LM_ST_EXCLUSIVE, GL_SKIP, &i_gh);
-       if (error)
-               return ERR_PTR(error);
-
-       error = gfs2_check_blk_type(sdp, no_addr, blktype);
-       if (error)
-               goto fail;
-
-       inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0);
+       inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0, blktype);
        if (IS_ERR(inode))
-               goto fail;
+               return inode;
 
        /* Two extra checks for NFS only */
        if (no_formal_ino) {
@@ -182,16 +231,12 @@ struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
                error = -EIO;
                if (GFS2_I(inode)->i_diskflags & GFS2_DIF_SYSTEM)
                        goto fail_iput;
-
-               error = 0;
        }
+       return inode;
 
-fail:
-       gfs2_glock_dq_uninit(&i_gh);
-       return error ? ERR_PTR(error) : inode;
 fail_iput:
        iput(inode);
-       goto fail;
+       return ERR_PTR(error);
 }
 
 

diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
index e1af0d4aa308ef83539771d0f73e5293cf557e69..443b46cfb8f233d6305f717ae3cb0c6687288f50 100644 (file)
--- a/fs/gfs2/inode.h
+++ b/fs/gfs2/inode.h
@@ -94,7 +94,8 @@ err:
 }
 
 extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type, 
-                                      u64 no_addr, u64 no_formal_ino);
+                                      u64 no_addr, u64 no_formal_ino,
+                                      unsigned int blktype);
 extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
                                         u64 *no_formal_ino,
                                         unsigned int blktype);

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 45463600fb81d34e48d328e63bf9ef389e0dd363..b8f6fc9513ef1e1aac4db567c8a4bdf003f2b30b 100644 (file)
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -454,7 +454,8 @@ static int gfs2_lookup_root(struct super_block *sb, struct dentry **dptr,
        struct dentry *dentry;
        struct inode *inode;
 
-       inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0);
+       inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0,
+                                 GFS2_BLKST_FREE /* ignore */);
        if (IS_ERR(inode)) {
                fs_err(sdp, "can't read in %s inode: %ld\n", name, PTR_ERR(inode));
                return PTR_ERR(inode);