Linux-EROFS Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
@ 2026-07-29 19:52 Martin Pitt
  2026-08-02 13:25 ` Gao Xiang
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Pitt @ 2026-07-29 19:52 UTC (permalink / raw)
  To: linux-erofs; +Cc: Gao Xiang, Yifan Zhao

listxattr(2) makes no promise about the order it reports: while e.g.
ext4 returns a reproducible order, tmpfs varies it from inode to inode,
so building the same tree twice can lay the same set of xattrs out
differently and yield images that differ byte for byte. This makes the
erofs images unreproducible.

Insert into the inode's list ordered by attribute name instead, and move
inline attributes onto the on-stack list with list_add_tail() so the
emitted order matches. This is the same approach as the shared attribute
pool already does with comp_shared_xattritem().

Signed-off-by: Martin Pitt <martin@amutable.com>
---

See the attached reproducer below (that was written by Claude Opus 5). With
$TMPDIR pointing to tmpfs (most modern distros should have the default /tmp/ on
tmpfs), it does 8 runs with lots of jitter:

> run  1  listxattr: gpt_label gpt_type_uuid mount_point            sha256: d81df9744f252cf3
> run  4  listxattr: gpt_label gpt_type_uuid mount_point            sha256: 6bad887d59e1e404
> run  7  listxattr: gpt_label gpt_type_uuid mount_point            sha256: a16c9aca9449e61b
> [...]
> 
> FAIL: mkfs.erofs output depends on listxattr(2) order

With the fix, the shas are all identical.

fsck.erofs is happy with images carrying shared, inline and multi-prefix
attributes.

The script needs setfattr/getfattr from attr, and a tmpfs $TMPDIR to vary the
reported order; it says so when the order came out stable, so a pass on ext4
does not mean much. I have not proposed the reproducer as a patch since the
repo has no tests. But if you want me to do that, let me know.

Also: Kernel process newbie here, sorry if I messed up formatting/structure.

Thanks,

Martin


#!/bin/sh
# Reproducer: mkfs.erofs output depends on listxattr(2) order.
#
# mkfs.erofs stores an inode's extended attributes in the order listxattr(2)
# reports them. That order is not specified, and tmpfs varies it from inode to
# inode, so building the very same tree twice can produce different images.
#
# Every run below writes identical content and sets the same three xattrs on the
# root inode in the same order; only the staging directory (hence its inode)
# differs. That is exactly what systemd-repart does when it builds a DDI: it
# stages each partition in a fresh mkdtemp() directory and sets its
# user.validatefs.* xattrs there before calling mkfs.erofs.
#
# Usage: ./xattr-order-reproducer.sh [path-to-mkfs.erofs] [runs]
#
# Exit status: 0 output is independent of listxattr order, 1 it is not,
# 77 prerequisites missing (autotools "skipped" convention).

set -eu

MKFS=${1:-mkfs.erofs}
RUNS=${2:-8}

# -x first: command -v does not look up a bare relative path such as mkfs/mkfs.erofs.
[ -x "$MKFS" ] || command -v "$MKFS" >/dev/null 2>&1 || { echo "cannot run $MKFS"; exit 77; }
command -v setfattr >/dev/null 2>&1 || { echo "need setfattr from the attr package"; exit 77; }
command -v getfattr >/dev/null 2>&1 || { echo "need getfattr from the attr package"; exit 77; }

WORK=$(mktemp -d "${TMPDIR:-/tmp}/erofs-xattr-order.XXXXXX")
trap 'rm -rf "$WORK"' EXIT

echo "mkfs:     $MKFS"
echo "work dir: $WORK on a $(stat -f -c %T "$WORK") filesystem"
echo

i=1
while [ "$i" -le "$RUNS" ]; do
	src=$WORK/src$i
	mkdir -p "$src/dir"
	echo payload > "$src/dir/file"

	# Always set in this order; listxattr(2) need not report it back this way.
	setfattr -n user.validatefs.gpt_label -v root-x86-64 "$src"
	setfattr -n user.validatefs.gpt_type_uuid \
		-v 2c7357ed-ebd2-46d9-aec1-23d437ec2bf5 "$src"
	setfattr -n user.validatefs.mount_point -v / "$src"

	order=$(getfattr -d --absolute-names "$src" |
		sed -n 's/^user\.validatefs\.\([a-z_]*\)=.*/\1/p' | tr '\n' ' ')

	# -T and -U pin the timestamp and the filesystem UUID, so the xattr order
	# is the only thing that can differ between runs.
	SOURCE_DATE_EPOCH=1739577600 "$MKFS" --quiet -T1739577600 \
		-U 5230d7cf-f2ce-43ed-9ae2-39e7e2fe48ca "$WORK/img$i" "$src"

	printf 'run %2d  listxattr: %-46s sha256: %s\n' \
		"$i" "$order" "$(sha256sum < "$WORK/img$i" | cut -c1-16)"
	i=$((i + 1))
done

echo
distinct=$(sha256sum "$WORK"/img* | awk '{print $1}' | sort -u | wc -l)
orders=$(for f in "$WORK"/src*; do
		getfattr -d --absolute-names "$f" |
			sed -n 's/^user\.validatefs\.\([a-z_]*\)=.*/\1/p' | tr '\n' ' '
		echo
	done | sort -u | wc -l)
echo "$RUNS identical trees: $orders distinct listxattr order(s), $distinct distinct image(s)"

if [ "$distinct" -ne 1 ]; then
	echo "FAIL: mkfs.erofs output depends on listxattr(2) order"
	exit 1
fi

if [ "$orders" -eq 1 ]; then
	echo "PASS, but inconclusive: this filesystem reported one stable order."
	echo "Point TMPDIR at a tmpfs to vary it, e.g. TMPDIR=/dev/shm $0 $MKFS $RUNS"
else
	echo "PASS: output is independent of listxattr(2) order"
fi

 lib/xattr.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/lib/xattr.c b/lib/xattr.c
index 051fdd8..cc74e16 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -400,17 +400,39 @@ static struct erofs_xattritem *erofs_get_selabel_xattr(struct erofs_sb_info *sbi
 	return NULL;
 }
 
+static int comp_inode_xattritem(const struct erofs_xattritem *a,
+				const struct erofs_xattritem *b)
+{
+	int ret = memcmp(a->kvbuf, b->kvbuf, min(a->len[0], b->len[0]));
+
+	if (ret)
+		return ret;
+	if (a->len[0] != b->len[0])
+		return a->len[0] < b->len[0] ? -1 : 1;
+	return 0;
+}
+
 static int erofs_inode_xattr_add(struct list_head *hlist,
 				 struct erofs_xattritem *item)
 {
-	struct erofs_inode_xattr_node *node;
+	struct erofs_inode_xattr_node *node, *pos;
 
 	node = malloc(sizeof(*node));
 	if (!node)
 		return -ENOMEM;
 	init_list_head(&node->list);
 	node->item = item;
-	list_add(&node->list, hlist);
+
+	/*
+	 * Keep each inode's xattrs ordered by name.  listxattr(2) makes no
+	 * promise about the order it reports, and tmpfs varies it from inode
+	 * to inode, so appending in listing order would emit the same set of
+	 * xattrs differently from run to run and make images unreproducible.
+	 */
+	list_for_each_entry(pos, hlist, list)
+		if (comp_inode_xattritem(item, pos->item) < 0)
+			break;
+	list_add_tail(&node->list, &pos->list);
 	return 0;
 }
 
@@ -1091,10 +1113,10 @@ char *erofs_export_xattr_ibody(struct erofs_inode *inode)
 		item = node->item;
 		list_del(&node->list);
 
-		/* move inline xattrs to the onstack list */
+		/* move inline xattrs to the onstack list, order preserved */
 		if (item->shared_xattr_id < 0 ||
 		    header->h_shared_count >= UCHAR_MAX) {
-			list_add(&node->list, &ilst);
+			list_add_tail(&node->list, &ilst);
 			continue;
 		}
 
-- 
2.55.0



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

* Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
  2026-07-29 19:52 [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order Martin Pitt
@ 2026-08-02 13:25 ` Gao Xiang
  2026-08-02 13:30   ` Gao Xiang
  0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang @ 2026-08-02 13:25 UTC (permalink / raw)
  To: Martin Pitt; +Cc: linux-erofs, Gao Xiang, Yifan Zhao

Hi Martin,

On Wed, Jul 29, 2026 at 09:52:08PM +0200, Martin Pitt wrote:
> listxattr(2) makes no promise about the order it reports: while e.g.
> ext4 returns a reproducible order, tmpfs varies it from inode to inode,
> so building the same tree twice can lay the same set of xattrs out
> differently and yield images that differ byte for byte. This makes the
> erofs images unreproducible.
> 
> Insert into the inode's list ordered by attribute name instead, and move
> inline attributes onto the on-stack list with list_add_tail() so the
> emitted order matches. This is the same approach as the shared attribute
> pool already does with comp_shared_xattritem().
> 
> Signed-off-by: Martin Pitt <martin@amutable.com>

Thanks for the patch!

I wonder if the following diff works too (but untested):

diff --git a/lib/xattr.c b/lib/xattr.c
index a9486e4..ed53de9 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -400,17 +400,44 @@ static struct erofs_xattritem *erofs_get_selabel_xattr(struct erofs_sb_info *sbi
 	return NULL;
 }
 
+static int erofs_comp_xattritem(const void *a, const void *b)
+{
+	const struct erofs_xattritem *ia, *ib;
+	unsigned int la, lb;
+	int ret;
+
+	ia = *((const struct erofs_xattritem **)a);
+	ib = *((const struct erofs_xattritem **)b);
+	la = EROFS_XATTR_KVSIZE(ia->len);
+	lb = EROFS_XATTR_KVSIZE(ib->len);
+
+	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
+	if (ret != 0)
+		return ret;
+	return cmpsgn(la, lb);
+}
+
 static int erofs_inode_xattr_add(struct list_head *hlist,
 				 struct erofs_xattritem *item)
 {
-	struct erofs_inode_xattr_node *node;
+	struct erofs_inode_xattr_node *node, *pos;
 
 	node = malloc(sizeof(*node));
 	if (!node)
 		return -ENOMEM;
 	init_list_head(&node->list);
 	node->item = item;
-	list_add(&node->list, hlist);
+
+	/*
+	 * Keep each inode's xattrs ordered by name.  listxattr(2) makes no
+	 * promise about the order it reports, and tmpfs varies it from inode
+	 * to inode, so appending in listing order would emit the same set of
+	 * xattrs differently from run to run and make images unreproducible.
+	 */
+	list_for_each_entry(pos, hlist, list)
+		if (erofs_comp_xattritem(item, pos->item) < 0)
+			break;
+	list_add_tail(&node->list, &pos->list);
 	return 0;
 }
 
@@ -848,24 +875,6 @@ static unsigned int erofs_cleanxattrs(struct erofs_xattrmgr *xamgr,
 	return count;
 }
 
-static int comp_shared_xattritem(const void *a, const void *b)
-{
-	const struct erofs_xattritem *ia, *ib;
-	unsigned int la, lb;
-	int ret;
-
-	ia = *((const struct erofs_xattritem **)a);
-	ib = *((const struct erofs_xattritem **)b);
-	la = EROFS_XATTR_KVSIZE(ia->len);
-	lb = EROFS_XATTR_KVSIZE(ib->len);
-
-	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
-	if (ret != 0)
-		return ret;
-
-	return la > lb;
-}
-
 int erofs_xattr_flush_name_prefixes(struct erofs_importer *im, bool plain)
 {
 	const struct erofs_importer_params *params = im->params;
@@ -1015,7 +1024,7 @@ int erofs_load_shared_xattrs_from_path(struct erofs_sb_info *sbi, const char *pa
 	}
 	DBG_BUGON(i != sharedxattr_count);
 	sorted_n[i] = NULL;
-	qsort(sorted_n, sharedxattr_count, sizeof(n), comp_shared_xattritem);
+	qsort(sorted_n, sharedxattr_count, sizeof(n), erofs_comp_xattritem);
 
 	buf = calloc(1, shared_xattrs_size);
 	if (!buf) {


Since I'd like to unify comp_shared_xattritem, if yes, could you resend
a new version (or if some bug happens) as this so I could merge this.

Thanks,
Gao Xiang



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

* Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
  2026-08-02 13:25 ` Gao Xiang
@ 2026-08-02 13:30   ` Gao Xiang
  2026-08-03 11:49     ` Martin Pitt
  0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang @ 2026-08-02 13:30 UTC (permalink / raw)
  To: Martin Pitt, linux-erofs, Gao Xiang, Yifan Zhao

On Sun, Aug 02, 2026 at 09:25:07PM +0800, Gao Xiang wrote:
> Hi Martin,
> 
> On Wed, Jul 29, 2026 at 09:52:08PM +0200, Martin Pitt wrote:
> > listxattr(2) makes no promise about the order it reports: while e.g.
> > ext4 returns a reproducible order, tmpfs varies it from inode to inode,
> > so building the same tree twice can lay the same set of xattrs out
> > differently and yield images that differ byte for byte. This makes the
> > erofs images unreproducible.
> > 
> > Insert into the inode's list ordered by attribute name instead, and move
> > inline attributes onto the on-stack list with list_add_tail() so the
> > emitted order matches. This is the same approach as the shared attribute
> > pool already does with comp_shared_xattritem().
> > 
> > Signed-off-by: Martin Pitt <martin@amutable.com>
> 
> Thanks for the patch!
> 
> I wonder if the following diff works too (but untested):
> 

...

> 
> Since I'd like to unify comp_shared_xattritem, if yes, could you resend
> a new version (or if some bug happens) as this so I could merge this.
> 

Sorry... It should be 

diff --git a/lib/xattr.c b/lib/xattr.c
index a9486e4..6a8775b 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -400,17 +400,44 @@ static struct erofs_xattritem *erofs_get_selabel_xattr(struct erofs_sb_info *sbi
 	return NULL;
 }
 
+static int erofs_comp_xattritem(const void *a, const void *b)
+{
+	const struct erofs_xattritem *ia, *ib;
+	unsigned int la, lb;
+	int ret;
+
+	ia = *((const struct erofs_xattritem **)a);
+	ib = *((const struct erofs_xattritem **)b);
+	la = EROFS_XATTR_KVSIZE(ia->len);
+	lb = EROFS_XATTR_KVSIZE(ib->len);
+
+	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
+	if (ret != 0)
+		return ret;
+	return cmpsgn(la, lb);
+}
+
 static int erofs_inode_xattr_add(struct list_head *hlist,
 				 struct erofs_xattritem *item)
 {
-	struct erofs_inode_xattr_node *node;
+	struct erofs_inode_xattr_node *node, *pos;
 
 	node = malloc(sizeof(*node));
 	if (!node)
 		return -ENOMEM;
 	init_list_head(&node->list);
 	node->item = item;
-	list_add(&node->list, hlist);
+
+	/*
+	 * Keep each inode's xattrs ordered by name.  listxattr(2) makes no
+	 * promise about the order it reports, and tmpfs varies it from inode
+	 * to inode, so appending in listing order would emit the same set of
+	 * xattrs differently from run to run and make images unreproducible.
+	 */
+	list_for_each_entry(pos, hlist, list)
+		if (erofs_comp_xattritem(item, pos->item) < 0)
+			break;
+	list_add_tail(&node->list, &pos->list);
 	return 0;
 }
 
@@ -848,24 +875,6 @@ static unsigned int erofs_cleanxattrs(struct erofs_xattrmgr *xamgr,
 	return count;
 }
 
-static int comp_shared_xattritem(const void *a, const void *b)
-{
-	const struct erofs_xattritem *ia, *ib;
-	unsigned int la, lb;
-	int ret;
-
-	ia = *((const struct erofs_xattritem **)a);
-	ib = *((const struct erofs_xattritem **)b);
-	la = EROFS_XATTR_KVSIZE(ia->len);
-	lb = EROFS_XATTR_KVSIZE(ib->len);
-
-	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
-	if (ret != 0)
-		return ret;
-
-	return la > lb;
-}
-
 int erofs_xattr_flush_name_prefixes(struct erofs_importer *im, bool plain)
 {
 	const struct erofs_importer_params *params = im->params;
@@ -1015,7 +1024,7 @@ int erofs_load_shared_xattrs_from_path(struct erofs_sb_info *sbi, const char *pa
 	}
 	DBG_BUGON(i != sharedxattr_count);
 	sorted_n[i] = NULL;
-	qsort(sorted_n, sharedxattr_count, sizeof(n), comp_shared_xattritem);
+	qsort(sorted_n, sharedxattr_count, sizeof(n), erofs_comp_xattritem);
 
 	buf = calloc(1, shared_xattrs_size);
 	if (!buf) {
@@ -1096,10 +1105,10 @@ char *erofs_export_xattr_ibody(struct erofs_inode *inode)
 		item = node->item;
 		list_del(&node->list);
 
-		/* move inline xattrs to the onstack list */
+		/* move inline xattrs to the onstack list, order preserved */
 		if (item->shared_xattr_id < 0 ||
 		    header->h_shared_count >= UCHAR_MAX) {
-			list_add(&node->list, &ilst);
+			list_add_tail(&node->list, &ilst);
 			continue;
 		}
 


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

* Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
  2026-08-02 13:30   ` Gao Xiang
@ 2026-08-03 11:49     ` Martin Pitt
  2026-08-03 23:14       ` Gao Xiang
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Pitt @ 2026-08-03 11:49 UTC (permalink / raw)
  To: linux-erofs, Gao Xiang, Yifan Zhao

Hello Gao,

Gao Xiang [2026-08-02 21:30 +0800]:
> > Thanks for the patch!
> > 
> > I wonder if the following diff works too (but untested):
> > 
> > Since I'd like to unify comp_shared_xattritem, if yes, could you resend
> > a new version (or if some bug happens) as this so I could merge this.
> > 
> 
> Sorry... It should be 

That's a nice idea, thanks!

> diff --git a/lib/xattr.c b/lib/xattr.c
> index a9486e4..6a8775b 100644
> --- a/lib/xattr.c
> +++ b/lib/xattr.c
> @@ -400,17 +400,44 @@ static struct erofs_xattritem *erofs_get_selabel_xattr(struct erofs_sb_info *sbi
>  	return NULL;
>  }
>  
> +static int erofs_comp_xattritem(const void *a, const void *b)
> +{
> [...]
> +	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
> +	if (ret != 0)
> +		return ret;
> +	return cmpsgn(la, lb);

This actually fixes the already existing sorting on main, too: The previous
`la > lb` never returned -1, so the sorting was half-broken.

> +	 * Keep each inode's xattrs ordered by name.  listxattr(2) makes no
> +	 * promise about the order it reports, and tmpfs varies it from inode
> +	 * to inode, so appending in listing order would emit the same set of
> +	 * xattrs differently from run to run and make images unreproducible.
> +	 */
> +	list_for_each_entry(pos, hlist, list)
> +		if (erofs_comp_xattritem(item, pos->item) < 0)

The items need &, otherwise it reinterprets the first 8 bytes of struct
erofs_xattritem, and you get bogus results. I fixed that.

There is a plot twist: Last week, when I investigated that and wrote the
reproducer, I was still on Fedora 44's 7.1.4 kernel, and the reproducer
reliably failed. Now I updated to 7.1.5, and it passes. This is probably the
effect of https://lkml.iu.edu/2602.2/00479.html and/or
https://lkml.org/lkml/2026/2/27/1141 , but it might explain why you may not
have seen the result. Funny timing! In other words, with recent kernels
tmpfs now reports the xattrs in insertion order instead of random.

But it still differs between running on *different* file systems, so the
justification stands, just the reproducer changed. I changed it to accept a set
of directories, defaulting to /tmp (which is usually tmpfs on modern distros)
and /var/tmp (which ought to be disk-backed, btrfs in my case). With the fix,
the erofs image comes out identical in both cases, while it differed between
backing file systems even on 7.1.5 (just that *within* the tmpfs runs it is
stable now). I also updated it to set 10 xattrs instead of 3, for more
confidence.

master (running against mkfs.erofs in $PATH, i.e. usr/bin/):

| ❱❱❱ ./xattr-order-reproducer.sh
| mkfs: mkfs.erofs
| 
| directory filesystem image sha256     listxattr order
| /tmp      tmpfs      52fae7f85e2a81fb weight mount_point mm aa_long_attribute_name growfs gpt_type_uuid zz verity gpt_label roothash
| /tmp      tmpfs      c55b5a4f737722f9 growfs aa_long_attribute_name mount_point weight gpt_label verity mm gpt_type_uuid roothash zz
| /tmp      tmpfs      f28ade3709861f8f mount_point mm growfs zz gpt_label aa_long_attribute_name roothash weight gpt_type_uuid verity
| /tmp      tmpfs      c5958ec278a8ab40 aa_long_attribute_name gpt_type_uuid growfs mm weight gpt_label roothash zz mount_point verity
| /var/tmp  btrfs      b4c0e159a27297f1 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      b4c0e159a27297f1 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      b4c0e159a27297f1 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      b4c0e159a27297f1 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
|
| 8 identical trees: 5 distinct listxattr order(s), 5 distinct image(s)
| FAIL: mkfs.erofs output depends on listxattr(2) order


this fix, in the built tree:

| ❱❱❱ ./xattr-order-reproducer.sh mkfs/mkfs.erofs
| mkfs: mkfs/mkfs.erofs
| 
| directory filesystem image sha256     listxattr order
| /tmp      tmpfs      3dc0a62cd704a430 gpt_label aa_long_attribute_name zz mm weight growfs verity gpt_type_uuid mount_point roothash
| /tmp      tmpfs      3dc0a62cd704a430 verity aa_long_attribute_name growfs mount_point mm gpt_label roothash gpt_type_uuid zz weight
| /tmp      tmpfs      3dc0a62cd704a430 zz weight verity gpt_type_uuid roothash mount_point aa_long_attribute_name growfs mm gpt_label
| /tmp      tmpfs      3dc0a62cd704a430 mount_point roothash weight verity aa_long_attribute_name zz growfs mm gpt_label gpt_type_uuid
| /var/tmp  btrfs      3dc0a62cd704a430 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      3dc0a62cd704a430 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      3dc0a62cd704a430 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
| /var/tmp  btrfs      3dc0a62cd704a430 mount_point gpt_label roothash mm aa_long_attribute_name growfs gpt_type_uuid weight verity zz
|
| 8 identical trees: 5 distinct listxattr order(s), 1 distinct image(s)
| PASS: output is independent of listxattr(2) order

I paste the updated reproducer here. (Still AI-slop-y, sorry -- this is
throwaway code, and I spent my time on cleaning up the actual patch)

I'll post a PATCH v2 now. I feel the urge to set "Co-Authored-By: you", as you
did the refactoring. Shall I do that? That would then require your S-o-B I
think? I added a Suggested-By: for now.

Thanks,

Martin


#!/bin/sh
# Reproducer: mkfs.erofs output depends on listxattr(2) order.
#
# mkfs.erofs stores an inode's extended attributes in the order listxattr(2)
# reports them. That order is not specified, and filesystems disagree about it:
# tmpfs reports the order the attributes were set in (and on some kernels a
# random order per inode), while ext4 and btrfs report their own on-disk order.
# So building the very same tree can produce different images.
#
# Every run below writes identical content and sets the same xattrs on the root
# inode in the same order; only the staging directory differs. That is what
# systemd-repart does when it builds a DDI: it stages each partition in a fresh
# mkdtemp() directory and sets its user.validatefs.* xattrs there before calling
# mkfs.erofs.
#
# It sets ten attributes where repart sets three, because with only three a
# broken comparison can still come out consistent by chance and pass this test.
#
# The staging directories default to /tmp and /var/tmp, which by convention are a
# tmpfs and a disk-backed filesystem. Pass other directories to compare a
# different pair.
#
# Usage: ./xattr-order-reproducer.sh [path-to-mkfs.erofs] [runs-per-dir] [dir...]
#
# Exit status: 0 output is independent of listxattr order, 1 it is not,
# 77 prerequisites missing (autotools "skipped" convention).

set -eu

MKFS=${1:-mkfs.erofs}
RUNS=${2:-4}
if [ $# -gt 2 ]; then
	shift 2
	DIRS=$*
else
	DIRS="/tmp /var/tmp"
fi

# -x first: command -v does not look up a bare relative path such as mkfs/mkfs.erofs.
[ -x "$MKFS" ] || command -v "$MKFS" >/dev/null 2>&1 || { echo "cannot run $MKFS"; exit 77; }
command -v setfattr >/dev/null 2>&1 || { echo "need setfattr from the attr package"; exit 77; }
# getfattr sorts its output, so it cannot show the order listxattr(2) reported.
command -v python3 >/dev/null 2>&1 || { echo "need python3 to read the listxattr order"; exit 77; }

# The first three are what repart sets; the rest vary in length and share prefixes,
# so a comparison that is subtly wrong cannot order them correctly by accident.
ATTRS="gpt_label=root-x86-64
	gpt_type_uuid=2c7357ed-ebd2-46d9-aec1-23d437ec2bf5
	mount_point=/
	roothash=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
	verity=yes
	growfs=no
	weight=1000
	mm=short
	zz=short
	aa_long_attribute_name=padding"

CLEAN=
trap 'rm -rf $CLEAN' EXIT

RESULTS=$(mktemp -d "${TMPDIR:-/tmp}/erofs-xattr-order.XXXXXX")
CLEAN=$RESULTS
: > "$RESULTS/images"
: > "$RESULTS/orders"

echo "mkfs: $MKFS"
echo
printf '%-9s %-10s %-16s %s\n' directory filesystem 'image sha256' 'listxattr order'

for dir in $DIRS; do
	[ -d "$dir" ] || { echo "$dir does not exist"; exit 77; }
	work=$(mktemp -d "$dir/erofs-xattr-order.XXXXXX")
	CLEAN="$CLEAN $work"

	i=1
	while [ "$i" -le "$RUNS" ]; do
		src=$work/src$i
		mkdir -p "$src/dir"
		echo payload > "$src/dir/file"

		# Always set in this order; listxattr(2) need not report it back this way.
		for kv in $ATTRS; do
			setfattr -n "user.validatefs.${kv%%=*}" -v "${kv#*=}" "$src"
		done

		order=$(python3 -c \
			'import os, sys; print(*(n.rsplit(".", 1)[-1] for n in os.listxattr(sys.argv[1])))' \
			"$src")

		# -T and -U pin the timestamp and the filesystem UUID, so the xattr order
		# is the only thing that can differ between runs.
		SOURCE_DATE_EPOCH=1739577600 "$MKFS" --quiet -T1739577600 \
			-U 5230d7cf-f2ce-43ed-9ae2-39e7e2fe48ca "$src.img" "$src"

		sha=$(sha256sum < "$src.img" | cut -d' ' -f1)
		echo "$sha" >> "$RESULTS/images"
		echo "$order" >> "$RESULTS/orders"
		printf '%-9s %-10s %-16s %s\n' \
			"$dir" "$(stat -f -c %T "$src")" "$(echo "$sha" | cut -c1-16)" "$order"
		i=$((i + 1))
	done
done

echo
distinct=$(sort -u "$RESULTS/images" | wc -l)
orders=$(sort -u "$RESULTS/orders" | wc -l)
echo "$(wc -l < "$RESULTS/images") identical trees: $orders distinct listxattr order(s), $distinct distinct image(s)"

if [ "$distinct" -ne 1 ]; then
	echo "FAIL: mkfs.erofs output depends on listxattr(2) order"
	exit 1
fi

if [ "$orders" -eq 1 ]; then
	echo "PASS, but inconclusive: every directory reported the same order."
	echo "Pass directories on filesystems that disagree, e.g. $0 $MKFS $RUNS /tmp \$HOME"
else
	echo "PASS: output is independent of listxattr(2) order"
fi


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

* Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
  2026-08-03 11:49     ` Martin Pitt
@ 2026-08-03 23:14       ` Gao Xiang
  2026-08-04  3:37         ` Martin Pitt
  0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang @ 2026-08-03 23:14 UTC (permalink / raw)
  To: Martin Pitt; +Cc: linux-erofs, Gao Xiang, Yifan Zhao

Hi Martin,

On Mon, Aug 03, 2026 at 01:49:05PM +0200, Martin Pitt wrote:
> Hello Gao,
> 
> Gao Xiang [2026-08-02 21:30 +0800]:
> > > Thanks for the patch!
> > > 
> > > I wonder if the following diff works too (but untested):
> > > 
> > > Since I'd like to unify comp_shared_xattritem, if yes, could you resend
> > > a new version (or if some bug happens) as this so I could merge this.
> > > 
> > 
> > Sorry... It should be 
> 
> That's a nice idea, thanks!
> 
> > diff --git a/lib/xattr.c b/lib/xattr.c
> > index a9486e4..6a8775b 100644
> > --- a/lib/xattr.c
> > +++ b/lib/xattr.c
> > @@ -400,17 +400,44 @@ static struct erofs_xattritem *erofs_get_selabel_xattr(struct erofs_sb_info *sbi
> >  	return NULL;
> >  }
> >  
> > +static int erofs_comp_xattritem(const void *a, const void *b)
> > +{
> > [...]
> > +	ret = memcmp(ia->kvbuf, ib->kvbuf, min(la, lb));
> > +	if (ret != 0)
> > +		return ret;
> > +	return cmpsgn(la, lb);
> 
> This actually fixes the already existing sorting on main, too: The previous
> `la > lb` never returned -1, so the sorting was half-broken.

Yes..

> 
> > +	 * Keep each inode's xattrs ordered by name.  listxattr(2) makes no
> > +	 * promise about the order it reports, and tmpfs varies it from inode
> > +	 * to inode, so appending in listing order would emit the same set of
> > +	 * xattrs differently from run to run and make images unreproducible.
> > +	 */
> > +	list_for_each_entry(pos, hlist, list)
> > +		if (erofs_comp_xattritem(item, pos->item) < 0)
> 
> The items need &, otherwise it reinterprets the first 8 bytes of struct
> erofs_xattritem, and you get bogus results. I fixed that.
> 
> There is a plot twist: Last week, when I investigated that and wrote the
> reproducer, I was still on Fedora 44's 7.1.4 kernel, and the reproducer
> reliably failed. Now I updated to 7.1.5, and it passes. This is probably the
> effect of https://lkml.iu.edu/2602.2/00479.html and/or
> https://lkml.org/lkml/2026/2/27/1141 , but it might explain why you may not
> have seen the result. Funny timing! In other words, with recent kernels
> tmpfs now reports the xattrs in insertion order instead of random.
> 
> But it still differs between running on *different* file systems, so the
> justification stands, just the reproducer changed. I changed it to accept a set
> of directories, defaulting to /tmp (which is usually tmpfs on modern distros)
> and /var/tmp (which ought to be disk-backed, btrfs in my case). With the fix,
> the erofs image comes out identical in both cases, while it differed between
> backing file systems even on 7.1.5 (just that *within* the tmpfs runs it is
> stable now). I also updated it to set 10 xattrs instead of 3, for more
> confidence.

I think we need to add a formal xattr reproducible testcase to
experimental-tests branch.  If you have time you could help add one
to ensure the order; or I could also find time too.

Thanks,
Gao Xiang


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

* Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order
  2026-08-03 23:14       ` Gao Xiang
@ 2026-08-04  3:37         ` Martin Pitt
  0 siblings, 0 replies; 6+ messages in thread
From: Martin Pitt @ 2026-08-04  3:37 UTC (permalink / raw)
  To: linux-erofs, Gao Xiang, Yifan Zhao

Hello Gao,

Gao Xiang [2026-08-04  7:14 +0800]:
> I think we need to add a formal xattr reproducible testcase to
> experimental-tests branch.  If you have time you could help add one
> to ensure the order; or I could also find time too.

Someone said "tests"! 😀

Sent: https://lore.kernel.org/linux-erofs/20260804033214.211267-1-martin@amutable.com/

Martin


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

end of thread, other threads:[~2026-08-04  3:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29 19:52 [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order Martin Pitt
2026-08-02 13:25 ` Gao Xiang
2026-08-02 13:30   ` Gao Xiang
2026-08-03 11:49     ` Martin Pitt
2026-08-03 23:14       ` Gao Xiang
2026-08-04  3:37         ` Martin Pitt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox