linux-kernel - Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 11 Jun 2023 23:04:05 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Baokun Li <libaokun1@...wei.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca, jack@...e.cz,
        ritesh.list@...il.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block()
 return void

On Sat, Jun 10, 2023 at 03:03:19PM -0400, Theodore Ts'o wrote:
> Unforuntately, the changes to ext4_insert_delayed_block() in this
> patch were buggy, and were causing tests to hang when running
> ext4/encrypt, ext4/bigalloc_4k, and ext4/bigalloc_1k test scenarios.
> A bisect using "gce-xfstests -c ext4/bigalloc_4k -C 5 generic/579"
> pinpointed the problem.
> 
> The problem is that ext4_clu_mapped can return a positive value, and
> so there are times when we do need to release the space even though
> there are no errors.
> 
> So I've fixed up your commit with the following changes.  With this
> change, the test regressions go away.

It turns out my fix was not correct, because I misread the fundamental
problem with the patch.  The issue was in the last patch hunk:
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
 errout:
 	return ret;
 }

The problem is that entering this code hunk, ret could be non-zero.
But when we made ext4_es_insert_delayed_block() to return void.  So
the changes to fs/ext4/inode.c needed to be replaced by this:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 129b9af53d62..7700db1782dd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1630,7 +1630,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	int ret;
 	bool allocated = false;
-	bool reserved = false;
 
 	/*
 	 * If the cluster containing lblk is shared with a delayed,
@@ -1646,8 +1645,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	if (sbi->s_cluster_ratio == 1) {
 		ret = ext4_da_reserve_space(inode);
 		if (ret != 0)   /* ENOSPC */
-			goto errout;
-		reserved = true;
+			return ret;
 	} else {   /* bigalloc */
 		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
 			if (!ext4_es_scan_clu(inode,
@@ -1655,12 +1653,11 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 				ret = ext4_clu_mapped(inode,
 						      EXT4_B2C(sbi, lblk));
 				if (ret < 0)
-					goto errout;
+					return ret;
 				if (ret == 0) {
 					ret = ext4_da_reserve_space(inode);
 					if (ret != 0)   /* ENOSPC */
-						goto errout;
-					reserved = true;
+						return ret;
 				} else {
 					allocated = true;
 				}
@@ -1670,12 +1667,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		}
 	}
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
-errout:
-	return ret;
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
+	return 0;
 }
 
 /*

						- Ted