linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: syzbot <syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Christoph Hellwig" <hch@lst.de>,
	"Alexey Khoroshilov" <khoroshilov@ispras.ru>,
	"Artem Bityutskiy" <artem.bityutskiy@linux.intel.com>,
	linux-fsdevel@vger.kernel.org,
	"Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>,
	"Tetsuo Handa" <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super()
Date: Sun, 20 May 2018 23:06:27 -0300	[thread overview]
Message-ID: <20180521020626.2u7tp7fi5uv2iztk@eaf> (raw)
In-Reply-To: <20180508014803.w7yqx5k6frk55ohy@eaf>

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

      reply	other threads:[~2018-05-21  2:06 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03 22:08 [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails Ernesto A. Fernández
2018-05-03 22:10 ` [PATCH 2/2] hfsplus: always return error " Ernesto A. Fernández
2018-05-03 22:31 ` [PATCH 1/2] hfsplus: clean up delayed work " Al Viro
2018-05-08  1:48   ` Ernesto A. Fernández
2018-05-21  2:06     ` Ernesto A. Fernández [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180521020626.2u7tp7fi5uv2iztk@eaf \
    --to=ernesto.mnd.fernandez@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=artem.bityutskiy@linux.intel.com \
    --cc=hch@lst.de \
    --cc=khoroshilov@ispras.ru \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com \
    --cc=viro@ZenIV.linux.org.uk \
    --subject='Re: [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).