[PATCH] ufs: free cylinder group structures on late mount failure

[PATCH] ufs: free cylinder group structures on late mount failure

[Joseph Qi]

ufs_fill_super() reads the cylinder group structures before it creates
the root dentry, so that a failure there no longer publishes a
superblock whose s_fs_info has already been freed.  The two failures
that remain after the read, a failed ufs_iget() and a failed
d_make_root(), still jump to the label that only frees the in-core
superblock information.

Everything ufs_read_cylinder_structures() allocated is released by
ufs_put_super_internal(), which runs from ufs_put_super(), and
generic_shutdown_super() calls that only inside its if (sb->s_root)
block.  With s_root left NULL, the s_cssize allocation behind s_csp, the
s_ncg buffer heads behind s_ucg[] and the UFS_MAX_GROUP_LOADED s_ucpi[]
entries are leaked, once per mount attempt.

Neither failure needs an I/O error to happen: ufs_iget() rejects a root
inode number that falls outside the s_ncg * s_ipg range the image
claims, and s_ipg is not validated.

Give those two paths their own label, and move the unwind that
ufs_read_cylinder_structures() already performs on its own failure into
a helper that both call.  The helper also clears the pointers it
releases.

Fixes: 55a4c98abb96 ("ufs: create the root dentry after loading cylinder metadata")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ufs/super.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index 3569ac92b065b..50adfef5f2aa9 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -448,6 +448,32 @@ static void ufs_setup_cstotal(struct super_block *sb)
 	UFSD("EXIT\n");
 }
 
+/*
+ * Release everything ufs_read_cylinder_structures() allocated.  Besides its
+ * own error path this is needed when a mount fails after the cylinder groups
+ * have been read: s_root is still NULL then, so generic_shutdown_super() never
+ * calls put_super and ufs_put_super_internal() does not run.
+ */
+static void ufs_free_cylinder_structures(struct super_block *sb)
+{
+	struct ufs_sb_info *sbi = UFS_SB(sb);
+	struct ufs_sb_private_info *uspi = sbi->s_uspi;
+	unsigned int i;
+
+	if (sbi->s_ucg) {
+		for (i = 0; i < uspi->s_ncg; i++)
+			brelse(sbi->s_ucg[i]);
+		kfree(sbi->s_ucg);
+		sbi->s_ucg = NULL;
+		for (i = 0; i < UFS_MAX_GROUP_LOADED; i++) {
+			kfree(sbi->s_ucpi[i]);
+			sbi->s_ucpi[i] = NULL;
+		}
+	}
+	kfree(sbi->s_csp);
+	sbi->s_csp = NULL;
+}
+
 /*
  * Read on-disk structures associated with cylinder groups
  */
@@ -511,15 +537,7 @@ static int ufs_read_cylinder_structures(struct super_block *sb)
 	return 1;
 
 failed:
-	kfree (base);
-	if (sbi->s_ucg) {
-		for (i = 0; i < uspi->s_ncg; i++)
-			if (sbi->s_ucg[i])
-				brelse (sbi->s_ucg[i]);
-		kfree (sbi->s_ucg);
-		for (i = 0; i < UFS_MAX_GROUP_LOADED; i++)
-			kfree (sbi->s_ucpi[i]);
-	}
+	ufs_free_cylinder_structures(sb);
 	UFSD("EXIT (FAILED)\n");
 	return 0;
 }
@@ -1211,17 +1229,19 @@ static int ufs_fill_super(struct super_block *sb, struct fs_context *fc)
 	inode = ufs_iget(sb, UFS_ROOTINO);
 	if (IS_ERR(inode)) {
 		ret = PTR_ERR(inode);
-		goto failed;
+		goto failed_cg;
 	}
 	sb->s_root = d_make_root(inode);
 	if (!sb->s_root) {
 		ret = -ENOMEM;
-		goto failed;
+		goto failed_cg;
 	}
 
 	UFSD("EXIT\n");
 	return 0;
 
+failed_cg:
+	ufs_free_cylinder_structures(sb);
 failed:
 	if (ubh)
 		ubh_brelse_uspi (uspi);
-- 
2.39.3

[PATCH v2] ufs: free cylinder group structures on late mount failure

[Joseph Qi]

ufs_fill_super() reads the cylinder group structures before it creates
the root dentry, and the two failures that remain after the read, a
failed ufs_iget() and a failed d_make_root(), jump to a label that only
frees the in-core superblock information.  generic_shutdown_super()
calls put_super only inside its if (sb->s_root) block, so with s_root
left NULL the s_csp allocation, the s_ncg buffer heads behind s_ucg[]
and the UFS_MAX_GROUP_LOADED s_ucpi[] entries are leaked, once per
mount attempt.  Neither failure needs an I/O error: ufs_iget() rejects
a root inode number outside the s_ncg * s_ipg range the image claims,
and s_ipg is not validated.

Move the unwind that ufs_read_cylinder_structures() already performs on
its own failure into ufs_free_cylinder_structures() and call it from
the two late mount failures as well.

The helper also clears everything it releases, which fixes a
stale-pointer bug on the remount path.  A read-only remount runs
ufs_put_super_internal(), which used to free s_csp, s_ucg and s_ucpi[]
without clearing them, and a read-write remount then re-reads the
structures.  If that re-read fails before reassigning the pointers,
the unwind frees the stale s_ucg and s_ucpi[] values a second time.
Have ufs_put_super_internal() flush the loaded cylinders and then call
the helper, so that every teardown clears what it frees, and reset
s_cg_loaded along with the pointers.

Fixes: 55a4c98abb96 ("ufs: create the root dentry after loading cylinder metadata")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ufs/super.c | 72 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 27 deletions(-)

diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index 3569ac92b065b..7b0a9929e7594 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -448,6 +448,37 @@ static void ufs_setup_cstotal(struct super_block *sb)
 	UFSD("EXIT\n");
 }
 
+/*
+ * Release everything ufs_read_cylinder_structures() allocated and clear
+ * everything released, so that no later error path can see a stale
+ * pointer: a read-only remount frees the structures through
+ * ufs_put_super_internal(), and a read-write remount re-reads them.
+ * Besides those two callers this is needed when a mount fails after the
+ * cylinder groups have been read: s_root is still NULL then, so
+ * generic_shutdown_super() never calls put_super and
+ * ufs_put_super_internal() does not run.
+ */
+static void ufs_free_cylinder_structures(struct super_block *sb)
+{
+	struct ufs_sb_info *sbi = UFS_SB(sb);
+	struct ufs_sb_private_info *uspi = sbi->s_uspi;
+	unsigned int i;
+
+	if (sbi->s_ucg) {
+		for (i = 0; i < uspi->s_ncg; i++)
+			brelse(sbi->s_ucg[i]);
+		kfree(sbi->s_ucg);
+		sbi->s_ucg = NULL;
+		for (i = 0; i < UFS_MAX_GROUP_LOADED; i++) {
+			kfree(sbi->s_ucpi[i]);
+			sbi->s_ucpi[i] = NULL;
+		}
+	}
+	sbi->s_cg_loaded = 0;
+	kfree(sbi->s_csp);
+	sbi->s_csp = NULL;
+}
+
 /*
  * Read on-disk structures associated with cylinder groups
  */
@@ -455,7 +486,7 @@ static int ufs_read_cylinder_structures(struct super_block *sb)
 {
 	struct ufs_sb_info *sbi = UFS_SB(sb);
 	struct ufs_sb_private_info *uspi = sbi->s_uspi;
-	unsigned char * base, * space;
+	unsigned char *space;
 	unsigned size, blks, i;
 
 	UFSD("ENTER\n");
@@ -466,9 +497,9 @@ static int ufs_read_cylinder_structures(struct super_block *sb)
 	 */
 	size = uspi->s_cssize;
 	blks = (size + uspi->s_fsize - 1) >> uspi->s_fshift;
-	base = space = kmalloc(size, GFP_NOFS);
-	if (!base)
-		goto failed; 
+	space = kmalloc(size, GFP_NOFS);
+	if (!space)
+		goto failed;
 	sbi->s_csp = (struct ufs_csum *)space;
 	for (i = 0; i < blks; i++) {
 		struct buffer_head *bh = sb_bread(sb, uspi->s_csaddr + i);
@@ -511,15 +542,7 @@ static int ufs_read_cylinder_structures(struct super_block *sb)
 	return 1;
 
 failed:
-	kfree (base);
-	if (sbi->s_ucg) {
-		for (i = 0; i < uspi->s_ncg; i++)
-			if (sbi->s_ucg[i])
-				brelse (sbi->s_ucg[i]);
-		kfree (sbi->s_ucg);
-		for (i = 0; i < UFS_MAX_GROUP_LOADED; i++)
-			kfree (sbi->s_ucpi[i]);
-	}
+	ufs_free_cylinder_structures(sb);
 	UFSD("EXIT (FAILED)\n");
 	return 0;
 }
@@ -585,7 +608,7 @@ static void ufs_put_super_internal(struct super_block *sb)
 {
 	struct ufs_sb_info *sbi = UFS_SB(sb);
 	struct ufs_sb_private_info *uspi = sbi->s_uspi;
-	unsigned char * base, * space;
+	unsigned char *space;
 	unsigned blks, size, i;
 
 	
@@ -594,7 +617,7 @@ static void ufs_put_super_internal(struct super_block *sb)
 	ufs_put_cstotal(sb);
 	size = uspi->s_cssize;
 	blks = (size + uspi->s_fsize - 1) >> uspi->s_fshift;
-	base = space = (char*) sbi->s_csp;
+	space = (char *)sbi->s_csp;
 	for (i = 0; i < blks; i++, space += uspi->s_fsize) {
 		struct buffer_head *bh = sb_bread(sb, uspi->s_csaddr + i);
 
@@ -607,16 +630,9 @@ static void ufs_put_super_internal(struct super_block *sb)
 		mark_buffer_dirty(bh);
 		brelse(bh);
 	}
-	for (i = 0; i < sbi->s_cg_loaded; i++) {
-		ufs_put_cylinder (sb, i);
-		kfree (sbi->s_ucpi[i]);
-	}
-	for (; i < UFS_MAX_GROUP_LOADED; i++) 
-		kfree (sbi->s_ucpi[i]);
-	for (i = 0; i < uspi->s_ncg; i++) 
-		brelse (sbi->s_ucg[i]);
-	kfree (sbi->s_ucg);
-	kfree (base);
+	for (i = 0; i < sbi->s_cg_loaded; i++)
+		ufs_put_cylinder(sb, i);
+	ufs_free_cylinder_structures(sb);
 
 	UFSD("EXIT\n");
 }
@@ -1211,17 +1227,19 @@ static int ufs_fill_super(struct super_block *sb, struct fs_context *fc)
 	inode = ufs_iget(sb, UFS_ROOTINO);
 	if (IS_ERR(inode)) {
 		ret = PTR_ERR(inode);
-		goto failed;
+		goto failed_cg;
 	}
 	sb->s_root = d_make_root(inode);
 	if (!sb->s_root) {
 		ret = -ENOMEM;
-		goto failed;
+		goto failed_cg;
 	}
 
 	UFSD("EXIT\n");
 	return 0;
 
+failed_cg:
+	ufs_free_cylinder_structures(sb);
 failed:
 	if (ubh)
 		ubh_brelse_uspi (uspi);
-- 
2.39.3