All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] erofs-utils: lib: fix erofs_is_packed_inode() on the read path
@ 2026-08-13  9:38 seula2.lee
  2026-08-13 12:08 ` Gao Xiang
  0 siblings, 1 reply; 2+ messages in thread
From: seula2.lee @ 2026-08-13  9:38 UTC (permalink / raw)
  To: linux-erofs; +Cc: Gao Xiang, seula2.lee

From: Seula Lee <seula2.lee@lge.com>

erofs_is_packed_inode() compares i_srcpath against the global
EROFS_PACKED_INODE identifier.  i_srcpath is only assigned by
mkfs.erofs and has no on-disk counterpart, so it is never set for
inodes filled in by erofs_read_inode_from_disk(), and the helper always
returns false on the read path.

fsck.erofs therefore treats the packed inode as an ordinary regular
file at all four of its call sites, for any image with fragments and a
non-zero packed_nid:

- erofsfsck_extract_inode(): the guard never fires, so
erofs_extract_file() writes the packed data to fsckcfg.extract_path
itself, which is still the top-level directory because main()
checks the packed inode before the root inode. Extracting the root
then fails in erofs_extract_dir():

$ mkfs.erofs -zlz4hc -Efragments img.erofs dir
$ fsck.erofs --extract=out img.erofs
<E> erofs: erofs_extract_dir() Line[697] path is not a directory: out
<E> erofs: main() Line[1256] Failed to extract filesystem
$ file out
out: ASCII text # 603720 bytes, the packed inode itself

- erofsfsck_set_attributes(): the packed inode's mode, timestamps and
xattrs are applied to the extraction root directory.

- erofs_verify_inode_data(): the packed inode's own compressed data is
decoded and hashed as if it were file data.

- the -p accounting: fragment bytes are counted both as file data and
as packed inode data, so the reported ratio is too low.

Restore the nid-based test, which is how the packed inode is already
identified elsewhere on the read path: z_erofs_read_one_data() compares
inode->nid against sbi->packed_nid directly, and the
erofs_sb_has_fragments(sbi) && sbi->packed_nid > 0 idiom used here is
the same one in fuse/main.c, dump/main.c and fsck.erofs' own
erofs_packedfile_init() call.

The i_srcpath comparison has to stay first rather than be replaced:
while mkfs.erofs builds the packed inode its nid is still
EROFS_NID_UNALLOCATED and sbi->packed_nid is 0, so a nid-only test
would return false there and break fragment dedupe and
pclusterblks_packed handling in lib/compress.c.

This restores the v1.8.x behaviour at every call site, where the helper
was nid-based; in particular the packed inode is again excluded from the
decompression check that --extract performs, as it was before v1.9.

dump.erofs is affected the same way and prints "Path : (packed file)"
again as intended.

erofs_is_metabox_inode() shares the same idiom, but it has no read-path
caller (all callers are in lib/compress.c and lib/inode.c), so it is
left alone.

Fixes: 7928074b7643 ("erofs-utils: introduce metadata compression [metabox]")
Signed-off-by: Seula Lee <seula2.lee@lge.com>
---
Affects v1.9 through v1.9.3; v1.8.x used a nid-based helper in
include/erofs/compress.h and is fine.

Tested on v1.9.3:

- --extract now works on lz4/lz4hc/fragments/all-fragments/dedupe
images, with `diff -r` against the source tree clean;
- check-only runs and fragments-less images are unchanged;
- corruption injected into the packed inode is still detected in 18 of
20 cases, the same as before this patch -- it now surfaces via the
fragment reads rather than the direct decode of the packed inode;
- mkfs.erofs output is byte-identical, including --incremental onto an
existing fragments image. Note that comparing mkfs output requires
pinning the nondeterministic fields, e.g.
-U <fixed-uuid> -T0 --mkfs-time.

   	include/erofs/internal.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/erofs/internal.h b/include/erofs/internal.h
index 2cc9cc8..c9c7873 100644
--- a/include/erofs/internal.h
+++ b/include/erofs/internal.h
@@ -577,7 +577,11 @@ extern const char *erofs_frags_packedname;
 
 static inline bool erofs_is_packed_inode(struct erofs_inode *inode)
 {
-	return inode->i_srcpath == EROFS_PACKED_INODE;
+	if (inode->i_srcpath == EROFS_PACKED_INODE)
+		return true;
+	return erofs_sb_has_fragments(inode->sbi) &&
+		inode->sbi->packed_nid > 0 &&
+		inode->nid == inode->sbi->packed_nid;
 }
 
 int erofs_packedfile_init(struct erofs_sb_info *sbi, bool fragments_mkfs);

base-commit: 7db78788b000999e2de88decd2ba90654f26171c
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] erofs-utils: lib: fix erofs_is_packed_inode() on the read path
  2026-08-13  9:38 [PATCH] erofs-utils: lib: fix erofs_is_packed_inode() on the read path seula2.lee
@ 2026-08-13 12:08 ` Gao Xiang
  0 siblings, 0 replies; 2+ messages in thread
From: Gao Xiang @ 2026-08-13 12:08 UTC (permalink / raw)
  To: seula2.lee; +Cc: linux-erofs, Gao Xiang

Hi Seula,

On Thu, Aug 13, 2026 at 06:38:49PM +0900, seula2.lee@lge.com wrote:
> From: Seula Lee <seula2.lee@lge.com>
> 
> erofs_is_packed_inode() compares i_srcpath against the global
> EROFS_PACKED_INODE identifier.  i_srcpath is only assigned by
> mkfs.erofs and has no on-disk counterpart, so it is never set for
> inodes filled in by erofs_read_inode_from_disk(), and the helper always
> returns false on the read path.
> 
> fsck.erofs therefore treats the packed inode as an ordinary regular
> file at all four of its call sites, for any image with fragments and a
> non-zero packed_nid:
> 
> - erofsfsck_extract_inode(): the guard never fires, so
> erofs_extract_file() writes the packed data to fsckcfg.extract_path
> itself, which is still the top-level directory because main()
> checks the packed inode before the root inode. Extracting the root
> then fails in erofs_extract_dir():
> 
> $ mkfs.erofs -zlz4hc -Efragments img.erofs dir
> $ fsck.erofs --extract=out img.erofs
> <E> erofs: erofs_extract_dir() Line[697] path is not a directory: out
> <E> erofs: main() Line[1256] Failed to extract filesystem
> $ file out
> out: ASCII text # 603720 bytes, the packed inode itself
> 
> - erofsfsck_set_attributes(): the packed inode's mode, timestamps and
> xattrs are applied to the extraction root directory.
> 
> - erofs_verify_inode_data(): the packed inode's own compressed data is
> decoded and hashed as if it were file data.
> 
> - the -p accounting: fragment bytes are counted both as file data and
> as packed inode data, so the reported ratio is too low.
> 
> Restore the nid-based test, which is how the packed inode is already
> identified elsewhere on the read path: z_erofs_read_one_data() compares
> inode->nid against sbi->packed_nid directly, and the
> erofs_sb_has_fragments(sbi) && sbi->packed_nid > 0 idiom used here is
> the same one in fuse/main.c, dump/main.c and fsck.erofs' own
> erofs_packedfile_init() call.
> 
> The i_srcpath comparison has to stay first rather than be replaced:
> while mkfs.erofs builds the packed inode its nid is still
> EROFS_NID_UNALLOCATED and sbi->packed_nid is 0, so a nid-only test
> would return false there and break fragment dedupe and
> pclusterblks_packed handling in lib/compress.c.
> 
> This restores the v1.8.x behaviour at every call site, where the helper
> was nid-based; in particular the packed inode is again excluded from the
> decompression check that --extract performs, as it was before v1.9.
> 
> dump.erofs is affected the same way and prints "Path : (packed file)"
> again as intended.
> 
> erofs_is_metabox_inode() shares the same idiom, but it has no read-path
> caller (all callers are in lib/compress.c and lib/inode.c), so it is
> left alone.
> 
> Fixes: 7928074b7643 ("erofs-utils: introduce metadata compression [metabox]")
> Signed-off-by: Seula Lee <seula2.lee@lge.com>

Thanks for the patch, that is indeed a bug and the fix looks fine,
but the commit message is too long to understand the impacts (it seems
in a LLM-generated-style.)

I will rephrase a short commit message instead.

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-08-13 12:08 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-13  9:38 [PATCH] erofs-utils: lib: fix erofs_is_packed_inode() on the read path seula2.lee
2026-08-13 12:08 ` Gao Xiang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.