[PATCHSET] fuse2fs: more bug fixes

[PATCHSET] fuse2fs: more bug fixes

[Darrick J. Wong]

Hi all,

This series fixes more bugs in fuse2fs.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

Comments and questions are, as always, welcome.

e2fsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/e2fsprogs.git/log/?h=fuse2fs-fixes
---
Commits in this patchset:
 * libext2fs: fix spurious warnings from fallocate
 * fuse2fs: fix error bailout in op_create
 * fuse2fs: catch positive errnos coming from libext2fs
---
 lib/ext2fs/fallocate.c |    5 +++++
 misc/fuse2fs.c         |   19 ++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

[PATCH 1/3] libext2fs: fix spurious warnings from fallocate

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

generic/522 routinely produces error messages from fuse2fs like this:

FUSE2FS (sde): Illegal block number passed to ext2fs_test_block_bitmap #9321 for block bitmap for /dev/sde

Curiously, these don't actually result in errors being thrown up to the
kernel.  Digging into the program (which was more difficult than it
needed to be because of the weird bitmap base + errcode weirdness)
produced a left record:

e_lblk = 16
e_pblk = 9293
e_len = 6
e_flags = 0

and a right record:

e_lblk = 45
e_pblk = 9321
e_len = 6
e_flags = 0

Thus we end up in the "Merge both extents together, perhaps?" section of
ext_falloc_helper.  Unfortunately, the merge selection code isn't smart
enough to notice that the two mappings aren't actually physically
contiguous, so it scans the bitmap with a negative length, which is why
the assertion trips.

The simple fix here is not to try to merge the adjacent extents if
they're not actually physically contiguous.

Cc: <linux-ext4@vger.kernel.org> # v1.43
Fixes: 5aad5b8e0e3cfa ("libext2fs: implement fallocate")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 lib/ext2fs/fallocate.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/lib/ext2fs/fallocate.c b/lib/ext2fs/fallocate.c
index 5cde7d5c2dc28b..063242c5fa4e6b 100644
--- a/lib/ext2fs/fallocate.c
+++ b/lib/ext2fs/fallocate.c
@@ -276,6 +276,11 @@ static errcode_t ext_falloc_helper(ext2_filsys fs,
 				max_uninit_len : max_init_len))
 			goto try_left;
 
+		/* Would they even be physically contiguous if merged? */
+		if (left_ext->e_pblk + left_ext->e_len + range_len !=
+				right_ext->e_pblk)
+			goto try_left;
+
 		err = ext2fs_extent_goto(handle, left_ext->e_lblk);
 		if (err)
 			goto try_left;

[PATCH 2/3] fuse2fs: fix error bailout in op_create

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Tim Woodall pointed out that op_create returns garbage error codes if
the ext2fs_extent_open2 in op_create fails.  Worse than that, it also
neglects to drop the bfl and leaks temp_path.  Let's fix all that.

Cc: <linux-ext4@vger.kernel.org> # v1.43
Fixes: 81cbf1ef4f5dab ("misc: add fuse2fs, a FUSE server for e2fsprogs")
Reported-by: Tim Woodall <debianbugs@woodall.me.uk>
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 misc/fuse2fs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index ad4795ae4ed907..cc023065727fd5 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -3306,8 +3306,11 @@ static int op_create(const char *path, mode_t mode, struct fuse_file_info *fp)
 		inode.i_flags &= ~EXT4_EXTENTS_FL;
 		ret = ext2fs_extent_open2(fs, child,
 					  EXT2_INODE(&inode), &handle);
-		if (ret)
-			return ret;
+		if (ret) {
+			ret = translate_error(fs, child, err);
+			goto out2;
+		}
+
 		ext2fs_extent_free(handle);
 	}
 

[PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Something in libext2fs is returning EOVERFLOW (positive) to us.  We need
to pass negative errnos back to the fuse driver, so perform this
translation.

Cc: <linux-ext4@vger.kernel.org> # v1.43
Fixes: 81cbf1ef4f5dab ("misc: add fuse2fs, a FUSE server for e2fsprogs")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 misc/fuse2fs.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)


diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index cc023065727fd5..2bf7413bebb70c 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -4633,8 +4633,18 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
 	int is_err = 0;
 
 	/* Translate ext2 error to unix error code */
-	if (err < EXT2_ET_BASE)
+	if (err < EXT2_ET_BASE) {
+		if (err > 0) {
+			/*
+			 * Apparently libext2fs can throw positive errno
+			 * numbers at us.  We need to return negative errnos
+			 * to the fuse driver.
+			 */
+			is_err = 1;
+			ret = -err;
+		}
 		goto no_translation;
+	}
 	switch (err) {
 	case EXT2_ET_NO_MEMORY:
 	case EXT2_ET_TDB_ERR_OOM:

Re: [PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

[Theodore Ts'o]

On Wed, Jun 11, 2025 at 09:44:17AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Something in libext2fs is returning EOVERFLOW (positive) to us.  We need
> to pass negative errnos back to the fuse driver, so perform this
> translation.

This isn't actually the best way to fix things.  The way the com_err
architecture works is that errcode_t is a 32-bit unsigned integer,
where the the top 24-bits is a subsystem identifier.  If the subsystem
identifier is zero, then the low 8 bits is presumed to be an errno
value.  Otherwise, the subsystem identifier is formed by taking a 4
character identifier from 62 valid code points A-Z, a-z, 0-9, and _,
where A is 1, and _ is 63.

Error table subsystems that are in common use and used by packages
shipped by most Linux distributions include "krb5" and "kadm" from the
MIT Kerberos implementation, and "ext2" and "prof" which ship as part
of e2fsprogs.  The original design came from Multics, and the idea is
that a library might call other libraries, and having a single unified
error code namespace can be super-useful.  Top-level callers can check
to see if an error is non-zero, and if so, call error_message(retval)
and get back a human-friendly string regardless of which library might
have originally generated the error.

CMU has a handy-dandy registry of the various libraries that use the
common error infrastructure here[1].

[1] https://www.central.org/frameless/numbers/errors.html

In the case of the ext2fs library, it doesn't actually call any AFS,
Kerberos, ASN.1, etc. libraries, so in practice the only valid error
codes that we should get back are either in the range 0..255 and
EXT2_ET_BASE..EXT2_ET_BASE+255.  But at least in theory, it's possible
that in the future, libext2fs might call some other library that might
return com_err error codes.

So a better, more idiomatic fix would be something like this below.

       	       		      	      	- Ted

P.S.  By the way, I'm not entirely convinced by the is_err vs !is_err
logic.  I get the idea is that we want to not log certain error cases
resulting from looking up a non-existing file name, but for example,
EXT2_ET_NO_MEMORY or any of the EXT2_TDB_* error messages should never
happen under normal circumstances, so if they do happen, they probably
should be logged, and so perhaps is_err=1 should be set for those
errors.  Similarly, I suspect that any MMP errors should probably also
be logged, but we can handle that as a separate commit.


commit 71f046a788adbae163c9398fccf50fff89bb9083
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Jun 12 14:03:44 2025 -0230

    fuse2fs: correctly handle system errno values in __translate_error()
    
    Fixes: 81cbf1ef4f5dab ("misc: add fuse2fs, a FUSE server for e2fsprogs")
    Reported-by: "Darrick J. Wong" <djwong@kernel.org>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index bc49edfe..97b1c5b5 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -4659,9 +4659,9 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
 	int is_err = 0;
 
 	/* Translate ext2 error to unix error code */
-	if (err < EXT2_ET_BASE)
-		goto no_translation;
 	switch (err) {
+	case 0:
+		break;
 	case EXT2_ET_NO_MEMORY:
 	case EXT2_ET_TDB_ERR_OOM:
 		ret = -ENOMEM;
@@ -4755,11 +4755,10 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
 		break;
 	default:
 		is_err = 1;
-		ret = -EIO;
+		ret = (err < 256) ? -err : -EIO;
 		break;
 	}
 
-no_translation:
 	if (!is_err)
 		return ret;
 

Re: [PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

[Darrick J. Wong]

On Thu, Jun 12, 2025 at 02:13:04PM -0230, Theodore Ts'o wrote:
> On Wed, Jun 11, 2025 at 09:44:17AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Something in libext2fs is returning EOVERFLOW (positive) to us.  We need
> > to pass negative errnos back to the fuse driver, so perform this
> > translation.
> 
> This isn't actually the best way to fix things.  The way the com_err
> architecture works is that errcode_t is a 32-bit unsigned integer,
> where the the top 24-bits is a subsystem identifier.  If the subsystem
> identifier is zero, then the low 8 bits is presumed to be an errno
> value.  Otherwise, the subsystem identifier is formed by taking a 4
> character identifier from 62 valid code points A-Z, a-z, 0-9, and _,
> where A is 1, and _ is 63.

I.... had no idea that errcode_t's were actually segmented numbers.  Is
there a way to figure out the subsystem from one of them?  Or will
fuse2fs just have to "know" that !(errcode & 0xFFFFFF00) means "errno"?

> Error table subsystems that are in common use and used by packages
> shipped by most Linux distributions include "krb5" and "kadm" from the
> MIT Kerberos implementation, and "ext2" and "prof" which ship as part
> of e2fsprogs.  The original design came from Multics, and the idea is
> that a library might call other libraries, and having a single unified
> error code namespace can be super-useful.  Top-level callers can check
> to see if an error is non-zero, and if so, call error_message(retval)
> and get back a human-friendly string regardless of which library might
> have originally generated the error.
> 
> CMU has a handy-dandy registry of the various libraries that use the
> common error infrastructure here[1].
> 
> [1] https://www.central.org/frameless/numbers/errors.html
> 
> In the case of the ext2fs library, it doesn't actually call any AFS,
> Kerberos, ASN.1, etc. libraries, so in practice the only valid error
> codes that we should get back are either in the range 0..255 and
> EXT2_ET_BASE..EXT2_ET_BASE+255.  But at least in theory, it's possible
> that in the future, libext2fs might call some other library that might
> return com_err error codes.
> 
> So a better, more idiomatic fix would be something like this below.
> 
>        	       		      	      	- Ted
> 
> P.S.  By the way, I'm not entirely convinced by the is_err vs !is_err
> logic.  I get the idea is that we want to not log certain error cases
> resulting from looking up a non-existing file name, but for example,
> EXT2_ET_NO_MEMORY or any of the EXT2_TDB_* error messages should never
> happen under normal circumstances, so if they do happen, they probably
> should be logged, and so perhaps is_err=1 should be set for those
> errors.  Similarly, I suspect that any MMP errors should probably also
> be logged, but we can handle that as a separate commit.

Hrm -- if MMP fails, that implies that we might not be the owner of
this filesystem, right?  Doesn't that means we should be careful about
not scribbling on the superblock?

> commit 71f046a788adbae163c9398fccf50fff89bb9083
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu Jun 12 14:03:44 2025 -0230
> 
>     fuse2fs: correctly handle system errno values in __translate_error()
>     
>     Fixes: 81cbf1ef4f5dab ("misc: add fuse2fs, a FUSE server for e2fsprogs")
>     Reported-by: "Darrick J. Wong" <djwong@kernel.org>
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
> index bc49edfe..97b1c5b5 100644
> --- a/misc/fuse2fs.c
> +++ b/misc/fuse2fs.c
> @@ -4659,9 +4659,9 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
>  	int is_err = 0;
>  
>  	/* Translate ext2 error to unix error code */
> -	if (err < EXT2_ET_BASE)
> -		goto no_translation;
>  	switch (err) {
> +	case 0:
> +		break;
>  	case EXT2_ET_NO_MEMORY:
>  	case EXT2_ET_TDB_ERR_OOM:
>  		ret = -ENOMEM;
> @@ -4755,11 +4755,10 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
>  		break;
>  	default:
>  		is_err = 1;
> -		ret = -EIO;
> +		ret = (err < 256) ? -err : -EIO;

Ok I'll rework the patch with this logic, but add a fat comment about
all this.

--D

>  		break;
>  	}
>  
> -no_translation:
>  	if (!is_err)
>  		return ret;
>  
> 

Re: [PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

[Theodore Ts'o]

On Thu, Jun 12, 2025 at 03:15:52PM -0700, Darrick J. Wong wrote:
> 
> I.... had no idea that errcode_t's were actually segmented numbers.  Is
> there a way to figure out the subsystem from one of them?

Sure, you can take the high 24-bits, and group them into 4 chunks of
6-bit numbers, and then apply the lookup table found in
lib/et/et_c.awk:

## "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_";
c2n["A"]=1
c2n["B"]=2
c2n["C"]=3
 ...
c2n["7"]=60
c2n["8"]=61
c2n["9"]=62
c2n["_"]=63

> Or will fuse2fs just have to "know" that !(errcode & 0xFFFFFF00)
> means "errno"?

How error codes get translated into strings is implemented by
lib/et/error_message.c.  If the high 24-bits are zero, then
error_message.c will call strerror(code), because it's assumed that
it's an errno.

Fuse2fs's __translate_error() function is also trying to interpret
error codes, and normally, most code paths just either (a) call
error_message(code) or (b) compare the code for equality against a
specific code point.  But __translate_error() needs to know the
internal underlying structure of the error code so it can do its own
translation, so it has to peer behimd the abstraction barrier
implemented by the com_err library.

I'll note that this scheme is a little fragile, because POSIX does not
guarantee that errno's have to be small integers that fit in the low 8
bits of an integer.  In practice this is true, but there is nothing
stopping a confirming POSIX implementation of some OS to use, say, to
have errnos between 0 and 1024.  Or perhaps some OS might try to
implement their own segmented error code space for errno's.  But in
practice, this has worked out for all the various subsystems that use
the com_err infrastructure, and like libext2fs, the krb5 library has
been ported to zillions of environments.

> Hrm -- if MMP fails, that implies that we might not be the owner of
> this filesystem, right?  Doesn't that means we should be careful about
> not scribbling on the superblock?

Well, the only time we check against MMP is when the file system is
opened.  That's because e2fsprogs doesn't implemented the full MMP
protocol as is found in the kernel.  In order to do that, we'd have to
spawn a separate thread which is periodically checking the superblock
to make sure no other node on the shared block file system has tried
to modify the file system out from under us.

Since historically e2fsprogs is single-threaded, what we do instead is
we write a magic value into the MMP sequence number,
EXT4_MMP_SEQ_FSCK, and if you are unfortunate enough to crash while
fsck.ext4 (or fuse2fs or other e2fsprogs program) is operating on a
file system, then a system adminsitrator would have to recover the
system manually using debugfs -c ("catastrophic recovery" mode) and
its set_super_value command to manually clear the MMP fields.

If we really care about trying to use fuse2fs in a primary/secondary
failover setup using a shared block device, we probably should put a
first class MMP implementation into libext2fs if threading is enabled.
However, the use of MMP is rare enough that it's probably not a high
priority to implement, at least not immediately.

					- Ted

[PATCH 4/3] libext2fs: fix bounding error in the extent fallocate code

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

generic/361 popped up this weird error:

generic/361       [failed, exit status 1]- output mismatch (see /var/tmp/fstests/generic/361.out.bad)
    --- tests/generic/361.out   2025-04-30 16:20:44.563589363 -0700
    +++ /var/tmp/fstests/generic/361.out.bad    2025-06-11 10:40:07.475036412 -0700
    @@ -1,2 +1,2 @@
     QA output created by 361
    -Silence is golden
    +mkfs.fuse.ext4: Input/output error while writing out and closing file system
    ...
    (Run 'diff -u /run/fstests/bin/tests/generic/361.out /var/tmp/fstests/generic/361.out.bad'  to see the entire diff)

The test formats a small filesystem, creates a larger sparse file, loop
mounts it, and tries to format an ext4 filesystem on the loopdev.  The
loop driver sends fallocate zero_range requests to fuse2fs, but stumbles
over this extent tree layout when fallocating 16 blocks at offset 145:

EXTENTS:
(262128-262143[u]):2127-2142

fallocate goes to offset 145, and sees the right-extent at 262128.
Oddly, it then tries to allocate 262128-145 blocks instead of the 16
that were asked for, so it tries to allocate a huge number of blocks
but then crashes and burns when it runs out of space.

Fix this by constraining the len parameter to ext_falloc_helper to the
correct value.

Cc: <linux-ext4@vger.kernel.org> # v1.43
Fixes: 5aad5b8e0e3cfa ("libext2fs: implement fallocate")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 lib/ext2fs/fallocate.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ext2fs/fallocate.c b/lib/ext2fs/fallocate.c
index 063242c5fa4e6b..1ef989cd38214d 100644
--- a/lib/ext2fs/fallocate.c
+++ b/lib/ext2fs/fallocate.c
@@ -718,7 +718,8 @@ static errcode_t extent_fallocate(ext2_filsys fs, int flags, ext2_ino_t ino,
 		goal = left_extent.e_pblk - (left_extent.e_lblk - start);
 		err = ext_falloc_helper(fs, flags, ino, inode, handle, NULL,
 					&left_extent, start,
-					left_extent.e_lblk - start, goal);
+					min(len, left_extent.e_lblk - start),
+					goal);
 		if (err)
 			goto errout;
 

Re: [PATCHSET] fuse2fs: more bug fixes

[Theodore Ts'o]

On Wed, 11 Jun 2025 09:43:39 -0700, Darrick J. Wong wrote:
> This series fixes more bugs in fuse2fs.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> Comments and questions are, as always, welcome.
> 
> [...]

Applied, thanks!

[1/3] libext2fs: fix spurious warnings from fallocate
      commit: 459efa346874a11b4911809e75d6eee157792ca5
[2/3] fuse2fs: fix error bailout in op_create
      commit: 9f6d1bf3bbf16cef4a08b67894c827059dd401a8

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

Re: [PATCHSET] fuse2fs: more bug fixes

[Theodore Ts'o]

On Wed, 11 Jun 2025 09:43:39 -0700, Darrick J. Wong wrote:
> This series fixes more bugs in fuse2fs.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> Comments and questions are, as always, welcome.
> 
> [...]

Applied, thanks!

[4/3] libext2fs: fix bounding error in the extent fallocate code
      commit: 149805bf64e81de61cc027bc43e9b480c4392800

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>