[PATCH e2fsprogs 0/2] e2fsck: fix a self-deadlock that hangs fsck on corrupt images

[PATCH e2fsprogs 0/2] e2fsck: fix a self-deadlock that hangs fsck on corrupt images

[Matthias Goergens]

e2fsck can hang forever on a corrupt image.  The hang is a self-deadlock
in libext2fs: flush_cached_blocks() releases CACHE_MTX around the
write_error callback, re-acquires it, and then jumps to a label above
the loop whose first statement acquires it again.  The mutex is not
recursive, so the first flush that reports a write error through a
registered handler blocks the thread that already holds the lock.

Patch 1 removes the redundant acquisition.  Patch 2 adds a regression
test.

This matters beyond the fuzzer that found it: the deadlocked process
sits at 0% CPU and does not respond to SIGTERM, so an init script
waiting on fsck waits forever, and read-only checking (-fn) is enough
to reach it.

Patch 2 departs from the usual f_* shape twice, both times because the
bug is a hang rather than a wrong answer: the e2fsck run is wrapped in
timeout(1), or a failure would stop the suite indefinitely rather than
fail, and the transcript is not compared, because it is a thousand
lines of repeated write errors that say nothing about this bug.  Happy
to drop the test or shape it differently if you would rather not have
either of those in the f_* tests.

Matthias Goergens (2):
  libext2fs: fix self-deadlock in flush_cached_blocks() write-error
    retry
  tests: add f_cache_mtx_deadlock for the flush_cached_blocks() retry
    lock

 lib/ext2fs/unix_io.c                |   2 +-
 tests/f_cache_mtx_deadlock/expect   |   2 ++
 tests/f_cache_mtx_deadlock/image.gz | Bin 0 -> 695 bytes
 tests/f_cache_mtx_deadlock/name     |   1 +
 tests/f_cache_mtx_deadlock/script   |  44 ++++++++++++++++++++++++++++
 5 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 tests/f_cache_mtx_deadlock/expect
 create mode 100644 tests/f_cache_mtx_deadlock/image.gz
 create mode 100644 tests/f_cache_mtx_deadlock/name
 create mode 100644 tests/f_cache_mtx_deadlock/script


base-commit: 8fd79523d051d5ea881237f27eb1c664486e0078
-- 
2.55.0

[PATCH e2fsprogs 1/2] libext2fs: fix self-deadlock in flush_cached_blocks() write-error retry

[Matthias Goergens]

flush_cached_blocks() drops CACHE_MTX before invoking the channel's
write_error handler, then re-acquires it and jumps to the retry label.
The retry label is above the while loop, whose first statement acquires
CACHE_MTX again:

  retry:
	while (errors_found) {
		if ((flags & FLUSH_NOLOCK) == 0)
			mutex_lock(data, CACHE_MTX);		<- second
		...
				mutex_unlock(data, CACHE_MTX);
				(channel->write_error)(...);
				...
				mutex_lock(data, CACHE_MTX);		<- first
				goto retry;

The mutex is created with pthread_mutex_init(..., NULL), so it is not
recursive and the second acquisition blocks the thread that already
holds it.  The first flush that has to report a write error through a
registered handler deadlocks, every time.

Found by fuzzing e2fsck with corrupt images.  An image whose group
descriptors force bitmap relocation makes e2fsck -fn fail hundreds of
writes on the read-only device; the aborted run reaches fatal_error(),
whose io_channel_flush() enters this loop.  e2fsck then sleeps forever
at 0% CPU and does not respond to SIGTERM, so an init script waiting on
fsck waits indefinitely.  Read-only checking is enough to reach it.

  unfixed:  killed after 35s, no progress
  fixed:    exits 12 after 13ms

Drop the redundant acquisition; the loop head takes the lock.

Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 lib/ext2fs/unix_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index abd33ba..29968e3 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -738,7 +738,7 @@ retry:
 					retval2);
 				if (err_buf)
 					ext2fs_free_mem(&err_buf);
-				mutex_lock(data, CACHE_MTX);
+				/* the loop head re-acquires CACHE_MTX */
 				goto retry;
 			} else
 				cache->write_err = 0;
-- 
2.55.0

[PATCH e2fsprogs 2/2] tests: add f_cache_mtx_deadlock for the flush_cached_blocks() retry lock

[Matthias Goergens]

Regression test for the previous patch.  The image has group
descriptors that force bitmap and inode table relocation, so e2fsck -fn
fails its writes on the read-only test file and reaches the write-error
retry loop in flush_cached_blocks() with a handler registered.

Two departures from the usual f_* shape, both because the bug is a hang
rather than a wrong answer.

The e2fsck run is wrapped in timeout(1).  A deadlocked e2fsck ignores
SIGTERM, so without a bound this test does not fail against an affected
build, it stops the suite indefinitely.  The probe-then-use form is
taken from tests/r_corrupt_fs/script, with -k so the SIGTERM escalates
to SIGKILL.

The transcript is not compared.  It is about a thousand lines of
repeated "Error writing block N" that say nothing about this bug and
would need updating whenever those messages change.  The test records
the exit status and that -n left the image unmodified, which is what it
is actually asserting.  On an affected build the diff is

  -Exit status is 12
  +Exit status is 137

Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 tests/f_cache_mtx_deadlock/expect   |   2 ++
 tests/f_cache_mtx_deadlock/image.gz | Bin 0 -> 695 bytes
 tests/f_cache_mtx_deadlock/name     |   1 +
 tests/f_cache_mtx_deadlock/script   |  44 ++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)
 create mode 100644 tests/f_cache_mtx_deadlock/expect
 create mode 100644 tests/f_cache_mtx_deadlock/image.gz
 create mode 100644 tests/f_cache_mtx_deadlock/name
 create mode 100644 tests/f_cache_mtx_deadlock/script

diff --git a/tests/f_cache_mtx_deadlock/expect b/tests/f_cache_mtx_deadlock/expect
new file mode 100644
index 0000000..6dd5466
--- /dev/null
+++ b/tests/f_cache_mtx_deadlock/expect
@@ -0,0 +1,2 @@
+Exit status is 12
+crc did not change.
diff --git a/tests/f_cache_mtx_deadlock/image.gz b/tests/f_cache_mtx_deadlock/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..956a1774ec0c88a1f5ed54b968acce77c77e61b9
GIT binary patch
literal 695
zcmb2|=HPf6wK0o{xhS=uC|@r#H=W___0#zwsWQhOmM>GD&LqYnIw8w(!t6^*CuF@A
z#ykn~ir87sta`WPVBd*DI|^RLAFyZW%obzWrgLY9@f%J?;X{)m?mMYVwg_;V@Yc*<
z{`2gUCv(nyne+1b*_lBN5#6t4bW)?kHIu8oEAIq*EiSIV|K&{eHp{B-w_1bL)X)FD
zTTylA#O1f?xAt#Os7em6&APTyKlWc$QU2>6=hb)D?=F3C?#SYg^6Ae~*y4AVKYsey
z-e!-<m)Gy>@8$m~eEjY3%KhaJzwA5rd4^~0*X4qr)_?W(j(fkzxs>m^<htU$-#^|-
ztNhn&6LP)zaB17?J1^!&N!DEWu<ymIVqS$%@Bad?ng2SkvlL_KFb)4NkUj68;<cK;
z&TI1iI<H~<)gHn9m4BnyOZlYQ74;og7yti$BWQX<c+{Huj;${L1zs!vb$(~=^4EDy
z>aX?)`PcGEwTtRIzOMYIc&+HK^BU1#?Gf5v`8V2Kwm%ZJw7%o&>iUkaJO3$O6aUp7
zQTwYsV)a-4jn`htC)KX3@7NmlUm$z#KgDa|zuF^Sf92n3_Ch`>_lo_ITTALYwnqLJ
zc%A*%c}@No{*BvS$R~Y$EuX{-QgGG&$S;u2+5Z%;mHl;I^ZpC}#<CakNpC?qm)3V|
zjs7o?-TzPVTHIgfH6S~Gf92oE_d-62_mzB->=pYXQkU$H%&N(j(o@@${r7vse}&N9
z%@cQD*W04L@;|%t*M(m<eSWWXw(oJ<Y~#=W{{Q_owd`->&Ci#o{xZ0bwRguq(^K~O
zKW?S}KRzq-Z}ZLK;#dFDAEbV_Kf7tq;~mOdpDr%#yJ{Kt{I`|*t2t|mSDkxlrCfUM
zieznX_0pT~>OW;^7QgzOQG5T}mh1cd{tAnq@9#hR=kNU$U-=mr7#J$7|FPD4_*#e<
HFfsrD8IOQ`

literal 0
HcmV?d00001

diff --git a/tests/f_cache_mtx_deadlock/name b/tests/f_cache_mtx_deadlock/name
new file mode 100644
index 0000000..6911823
--- /dev/null
+++ b/tests/f_cache_mtx_deadlock/name
@@ -0,0 +1 @@
+e2fsck self-deadlocks flushing the write-error cache (regression test for libext2fs CACHE_MTX retry double-lock)
diff --git a/tests/f_cache_mtx_deadlock/script b/tests/f_cache_mtx_deadlock/script
new file mode 100644
index 0000000..8de8533
--- /dev/null
+++ b/tests/f_cache_mtx_deadlock/script
@@ -0,0 +1,44 @@
+FSCK_OPT=-fn
+OUT=$test_name.log
+EXP=$test_dir/expect
+
+# This test is about termination, not output: an unfixed e2fsck
+# self-deadlocks here and ignores SIGTERM, so bound the run or a failure
+# stops the whole suite indefinitely.  The probe-then-use form is from
+# tests/r_corrupt_fs/script; -k escalates to SIGKILL.
+if timeout -v 1s true > /dev/null 2>&1 ; then
+	TIMEOUT="timeout -v -k 5s 30s"
+else
+	TIMEOUT=
+fi
+
+gzip -d < $test_dir/image.gz > $TMPFILE
+old="$($CRCSUM < $TMPFILE)"
+
+# The transcript is a thousand lines of repeated write errors and is not
+# what is under test; only record that e2fsck came back, with the status
+# it should have, and that -n left the image alone.
+$TIMEOUT $FSCK $FSCK_OPT -N test_filesys $TMPFILE > /dev/null 2>&1
+status=$?
+echo "Exit status is $status" > $OUT
+
+new="$($CRCSUM < $TMPFILE)"
+if [ "${old}" != "${new}" ]; then
+	echo "ERROR: crc mismatch!  ${old} ${new}" >> $OUT
+else
+	echo "crc did not change." >> $OUT
+fi
+rm -f $TMPFILE
+
+cmp -s $OUT $EXP
+status=$?
+
+if [ "$status" = 0 ] ; then
+	echo "$test_name: $test_description: ok"
+	touch $test_name.ok
+else
+	echo "$test_name: $test_description: failed"
+	diff $DIFF_OPTS $EXP $OUT > $test_name.failed
+fi
+
+unset FSCK_OPT OUT EXP old new status TIMEOUT
-- 
2.55.0

Re: [PATCH e2fsprogs 1/2] libext2fs: fix self-deadlock in flush_cached_blocks() write-error retry

[Darrick J. Wong]

On Tue, Sep 22, 2026 at 07:04:34PM +0800, Matthias Goergens wrote:
> flush_cached_blocks() drops CACHE_MTX before invoking the channel's
> write_error handler, then re-acquires it and jumps to the retry label.
> The retry label is above the while loop, whose first statement acquires
> CACHE_MTX again:
> 
>   retry:
> 	while (errors_found) {
> 		if ((flags & FLUSH_NOLOCK) == 0)
> 			mutex_lock(data, CACHE_MTX);		<- second
> 		...
> 				mutex_unlock(data, CACHE_MTX);
> 				(channel->write_error)(...);
> 				...
> 				mutex_lock(data, CACHE_MTX);		<- first
> 				goto retry;
> 
> The mutex is created with pthread_mutex_init(..., NULL), so it is not
> recursive and the second acquisition blocks the thread that already
> holds it.  The first flush that has to report a write error through a
> registered handler deadlocks, every time.
> 
> Found by fuzzing e2fsck with corrupt images.  An image whose group
> descriptors force bitmap relocation makes e2fsck -fn fail hundreds of
> writes on the read-only device; the aborted run reaches fatal_error(),
> whose io_channel_flush() enters this loop.  e2fsck then sleeps forever
> at 0% CPU and does not respond to SIGTERM, so an init script waiting on
> fsck waits indefinitely.  Read-only checking is enough to reach it.
> 
>   unfixed:  killed after 35s, no progress
>   fixed:    exits 12 after 13ms
> 
> Drop the redundant acquisition; the loop head takes the lock.
> 
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
> ---
>  lib/ext2fs/unix_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> index abd33ba..29968e3 100644
> --- a/lib/ext2fs/unix_io.c
> +++ b/lib/ext2fs/unix_io.c
> @@ -738,7 +738,7 @@ retry:
>  					retval2);
>  				if (err_buf)
>  					ext2fs_free_mem(&err_buf);
> -				mutex_lock(data, CACHE_MTX);
> +				/* the loop head re-acquires CACHE_MTX */

Yep, looks correct.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  				goto retry;
>  			} else
>  				cache->write_err = 0;
> -- 
> 2.55.0
> 
>