[PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super()

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

If no hidden directory exists, the hfsplus_fill_super() function will
create it. A delayed work is then queued to sync the superblock, which
is never canceled in case of failure.

Also, if the filesystem is corrupted in such a way that the hidden
directory is not of type HFSPLUS_FOLDER, the mount will fail without
throwing an error code. The vfs layer is then forced to dereference a
NULL root dentry.

To fix these issues, return an error and allow ->put_super() to take
care of most of the cleanup if failure occurs after sb->s_root has been
set.

Before this patch the volume was simply flagged as inconsistent if the
mount failed while creating the hidden directory. Since ->put_super()
will now toggle those flags, we need to correct the folder count before
returning, with a call to hfsplus_delete_inode(). Simplify this by using
a new hfsplus_create_inode() function that can handle its own cleanup.

Reported-by: syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com
Fixes: 5bd9d99d107c ("hfsplus: add error checking for hfs_find_init()")
Fixes: 9e6c5829b07c ("hfsplus: get rid of write_super")
Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
Changes in v3:
  Move the code that creates the hidden dir into a separate function. I
  think this is more reasonable. The new function can also be called by
  hfsplus_mknod().

 fs/hfsplus/dir.c        | 28 +++-----------------------
 fs/hfsplus/hfsplus_fs.h |  2 ++
 fs/hfsplus/inode.c      | 32 +++++++++++++++++++++++++++++
 fs/hfsplus/super.c      | 53 +++++++++++++------------------------------------
 4 files changed, 51 insertions(+), 64 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 15e06fb552da..2446e5db1dfb 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -483,37 +483,15 @@ static int hfsplus_mknod(struct inode *dir, struct dentry *dentry,
 {
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(dir->i_sb);
 	struct inode *inode;
-	int res = -ENOMEM;
+	int res;
 
 	mutex_lock(&sbi->vh_mutex);
-	inode = hfsplus_new_inode(dir->i_sb, dir, mode);
-	if (!inode)
-		goto out;
-
-	if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode))
-		init_special_inode(inode, mode, rdev);
-
-	res = hfsplus_create_cat(inode->i_ino, dir, &dentry->d_name, inode);
+	res = hfsplus_create_inode(dir, &dentry->d_name, mode, rdev, &inode);
 	if (res)
-		goto failed_mknod;
-
-	res = hfsplus_init_inode_security(inode, dir, &dentry->d_name);
-	if (res == -EOPNOTSUPP)
-		res = 0; /* Operation is not supported. */
-	else if (res) {
-		/* Try to delete anyway without error analysis. */
-		hfsplus_delete_cat(inode->i_ino, dir, &dentry->d_name);
-		goto failed_mknod;
-	}
-
+		goto out;
 	hfsplus_instantiate(dentry, inode, inode->i_ino);
 	mark_inode_dirty(inode);
-	goto out;
 
-failed_mknod:
-	clear_nlink(inode);
-	hfsplus_delete_inode(inode);
-	iput(inode);
 out:
 	mutex_unlock(&sbi->vh_mutex);
 	return res;
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index d9255abafb81..05f30ae788f0 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -480,6 +480,8 @@ extern const struct dentry_operations hfsplus_dentry_operations;
 
 struct inode *hfsplus_new_inode(struct super_block *sb, struct inode *dir,
 				umode_t mode);
+int hfsplus_create_inode(struct inode *dir, struct qstr *name, umode_t mode,
+			 dev_t rdev, struct inode **inode);
 void hfsplus_delete_inode(struct inode *inode);
 void hfsplus_inode_read_fork(struct inode *inode,
 			     struct hfsplus_fork_raw *fork);
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index c0c8d433864f..b15bb83deda1 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -413,6 +413,38 @@ struct inode *hfsplus_new_inode(struct super_block *sb, struct inode *dir,
 	return inode;
 }
 
+int hfsplus_create_inode(struct inode *dir, struct qstr *name, umode_t mode,
+			 dev_t rdev, struct inode **inode)
+{
+	int res;
+
+	*inode = hfsplus_new_inode(dir->i_sb, dir, mode);
+	if (!*inode)
+		return -ENOMEM;
+
+	if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode))
+		init_special_inode(*inode, mode, rdev);
+
+	res = hfsplus_create_cat((*inode)->i_ino, dir, name, *inode);
+	if (res)
+		goto fail;
+
+	res = hfsplus_init_inode_security(*inode, dir, name);
+	if (res && res != -EOPNOTSUPP) {
+		/* Try to delete anyway without error analysis. */
+		hfsplus_delete_cat((*inode)->i_ino, dir, name);
+		goto fail;
+	}
+	return 0;
+
+fail:
+	clear_nlink(*inode);
+	hfsplus_delete_inode(*inode);
+	iput(*inode);
+	*inode = NULL;
+	return res;
+}
+
 void hfsplus_delete_inode(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 513c357c734b..3749d9381c56 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -514,22 +514,26 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_put_alloc_file;
 	}
 
+	/* From here on, most of the cleanup is handled by ->put_super() */
+
 	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
 	str.name = HFSP_HIDDENDIR_NAME;
 	err = hfs_find_init(sbi->cat_tree, &fd);
 	if (err)
-		goto out_put_root;
+		goto out;
 	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
 	if (unlikely(err < 0))
-		goto out_put_root;
+		goto out;
 	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
 		hfs_find_exit(&fd);
-		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER))
-			goto out_put_root;
+		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
+			err = -EINVAL;
+			goto out;
+		}
 		inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
 		if (IS_ERR(inode)) {
 			err = PTR_ERR(inode);
-			goto out_put_root;
+			goto out;
 		}
 		sbi->hidden_dir = inode;
 	} else
@@ -549,35 +553,11 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 
 		if (!sbi->hidden_dir) {
 			mutex_lock(&sbi->vh_mutex);
-			sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
-			if (!sbi->hidden_dir) {
-				mutex_unlock(&sbi->vh_mutex);
-				err = -ENOMEM;
-				goto out_put_root;
-			}
-			err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
-						 &str, sbi->hidden_dir);
-			if (err) {
-				mutex_unlock(&sbi->vh_mutex);
-				goto out_put_hidden_dir;
-			}
-
-			err = hfsplus_init_inode_security(sbi->hidden_dir,
-								root, &str);
-			if (err == -EOPNOTSUPP)
-				err = 0; /* Operation is not supported. */
-			else if (err) {
-				/*
-				 * Try to delete anyway without
-				 * error analysis.
-				 */
-				hfsplus_delete_cat(sbi->hidden_dir->i_ino,
-							root, &str);
-				mutex_unlock(&sbi->vh_mutex);
-				goto out_put_hidden_dir;
-			}
-
+			err = hfsplus_create_inode(root, &str, S_IFDIR, 0,
+						   &sbi->hidden_dir);
 			mutex_unlock(&sbi->vh_mutex);
+			if (err)
+				goto out;
 			hfsplus_mark_inode_dirty(sbi->hidden_dir,
 						 HFSPLUS_I_CAT_DIRTY);
 		}
@@ -587,11 +567,6 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	sbi->nls = nls;
 	return 0;
 
-out_put_hidden_dir:
-	iput(sbi->hidden_dir);
-out_put_root:
-	dput(sb->s_root);
-	sb->s_root = NULL;
 out_put_alloc_file:
 	iput(sbi->alloc_file);
 out_close_attr_tree:
@@ -605,9 +580,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	kfree(sbi->s_backup_vhdr_buf);
 out_unload_nls:
 	unload_nls(sbi->nls);
-	unload_nls(nls);
 	kfree(sbi);
 out:
+	unload_nls(nls);
 	return err;
 }
 
-- 
2.11.0