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>
Subject: Re: [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails
Date: Mon, 7 May 2018 22:48:04 -0300	[thread overview]
Message-ID: <20180508014803.w7yqx5k6frk55ohy@eaf> (raw)
In-Reply-To: <20180503223115.GD30522@ZenIV.linux.org.uk>

On Thu, May 03, 2018 at 11:31:15PM +0100, Al Viro wrote:
> On Thu, May 03, 2018 at 07:08:22PM -0300, Ernesto A. Fernández wrote:
> > 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. Fix this.
> 
> Wouldn't it be simpler to avoid all the crap with clearing ->s_root
> on failure, letting ->put_super() take care of everything?  Or, better
> yet, take cleanups into ->kill_sb(), which is always called on
> superblock shutdown, ->s_root or no ->s_root...

If you mean that I move all cleanups from fill_super() to ->kill_sb(),
that sounds like a lot of trouble. Is this common in other filesystems?

As for letting ->put_super() do the cleanup, the problem is that it will
flag the volume as consistent, even if the directory count was increased
and the hidden dir was not added to the catalog. So we should correct
that before returning from fill_super(). I wouldn't call this simpler,
but I guess it's a bit better for consistency.

-- >8 --
Subject: [PATCH] hfsplus: fix cleanup for hfsplus_fill_super()

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, be sure to correct the folder count before
returning, with a call to hfsplus_delete_inode().

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>
---
 fs/hfsplus/super.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 513c357c734b..cd39f8d34241 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
@@ -551,16 +555,13 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 			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;
+				goto out_unlock_vh_mutex;
 			}
 			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;
-			}
+			if (err)
+				goto out_delete_hidden_dir;
 
 			err = hfsplus_init_inode_security(sbi->hidden_dir,
 								root, &str);
@@ -573,8 +574,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 				 */
 				hfsplus_delete_cat(sbi->hidden_dir->i_ino,
 							root, &str);
-				mutex_unlock(&sbi->vh_mutex);
-				goto out_put_hidden_dir;
+				goto out_delete_hidden_dir;
 			}
 
 			mutex_unlock(&sbi->vh_mutex);
@@ -587,11 +587,13 @@ 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_delete_hidden_dir:
+	clear_nlink(inode);
+	hfsplus_delete_inode(inode);
+out_unlock_vh_mutex:
+	mutex_unlock(&sbi->vh_mutex);
+	goto out;
+
 out_put_alloc_file:
 	iput(sbi->alloc_file);
 out_close_attr_tree:
@@ -605,9 +607,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-08  1:48 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03 22:08 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 [this message]
2018-05-21  2:06     ` [PATCH v3] hfsplus: fix cleanup for hfsplus_fill_super() Ernesto A. Fernández

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=20180508014803.w7yqx5k6frk55ohy@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=syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com \
    --cc=viro@ZenIV.linux.org.uk \
    --subject='Re: [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails' \
    /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).