[PATCH] affs: don't instantiate dentry if affs_insert_hash failed

Farhad Alemi posted 1 patch 1 week, 6 days ago
fs/affs/inode.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] affs: don't instantiate dentry if affs_insert_hash failed
Posted by Farhad Alemi 1 week, 6 days ago
affs_add_entry() calls d_instantiate() unconditionally after
affs_insert_hash() returns, even when the hash insert failed. When the
parent directory's block cannot be read (e.g. -EIO from affs_bread on a
crafted image), affs_insert_hash() returns the error, but the dentry
has already been bound to the freshly-created inode by d_instantiate().
The callers (affs_create / affs_mkdir / affs_symlink) then run iput()
on the inode in their error path, dropping its refcount to zero and
evicting it through affs_evict_inode().

The dentry still holds a pointer to that now-evicted inode. On umount,
shrink_dcache_for_umount() walks the dentry and __dentry_kill() calls
iput() on an inode already in I_FREEING|I_CLEAR, hitting the
VFS_BUG_ON_INODE() at fs/inode.c:1980:

  VFS_BUG_ON_INODE(inode_state_read_once(inode) & (I_FREEING | I_CLEAR))
    inode:... fs:affs mode:120777 ... state:0x300 count:0
  kernel BUG at fs/inode.c:1980!
  Call Trace:
   iput+0xe29/0xe80 fs/inode.c:1980
   __dentry_kill+0x20e/0x710 fs/dcache.c:718
   shrink_kill+0xa9/0x2c0 fs/dcache.c:1195
   shrink_dentry_list+0x2e5/0x5f0 fs/dcache.c:1222
   shrink_dcache_for_umount+0xa5/0x170 fs/dcache.c:1738
   generic_shutdown_super+0x74/0x2d0 fs/super.c:624
   kill_block_super+0x49/0xa0 fs/super.c:1725
   affs_kill_sb+0x4c/0x160 fs/affs/super.c:590

Reproduced by mounting a crafted AFFS image whose root-directory header
block is unreadable, calling symlinkat() to create an entry under it
(which reaches affs_add_entry() with retval != 0 from
affs_insert_hash()), and letting the mount tear down. Trigger requires
the ability to mount a crafted image (CAP_SYS_ADMIN or equivalent).

Only call d_instantiate() when affs_insert_hash() succeeded. On the
error path the dentry remains negative, so the caller's iput() correctly
disposes of the unbound inode and no stale dentry-to-evicted-inode
binding survives into umount.

Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Signed-off-by: Farhad Alemi <farhad.alemi@berkeley.edu>
---
 fs/affs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 5dd1b016bcb0..517d40c6c7df 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -407,7 +407,8 @@ affs_add_entry(struct inode *dir, struct inode
*inode, struct dentry *dentry, s3
 	affs_unlock_dir(dir);
 	affs_unlock_link(inode);

-	d_instantiate(dentry, inode);
+	if (!retval)
+		d_instantiate(dentry, inode);
 done:
 	affs_brelse(inode_bh);
 	affs_brelse(bh);
-- 
2.43.0