Re: [PATCH] erofs-utils: mkfs: emit an inode's xattrs in a canonical order

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 <[email protected]>
> 
> 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;
 		}