[PATCH v4 0/5] erofs: cleanup of xattr handling

[PATCH v4 0/5] erofs: cleanup of xattr handling

[Jingbo Xu]

changes since v3:
- patch 1: make a unified erofs_xattr_iter_fixup() API with newly
  introduced "bool nospan" argument; call erofs_init_metabuf() and
  erofs_bread() separately instead of erofs_read_metabuf()
- patch 2: avoid duplicated strlen() calculation in erofs_getxattr(); no
  need zeroing other fields when initializing 'struct erofs_xattr_iter'
- patch 4: don't explode 'struct erofs_xattr_iter' with inode/getxattr
  fields; instead pass inode/getxattr parameters through function
  parameters of erofs_iter_[inline|shared]_xattr()
- patch 5: don't explode 'struct erofs_xattr_iter' with remaining field;
  instead  calculate and check the remaining inside
  erofs_iter_inline_xattr()

changes since v2:
- rebase to v6.4-rc2
- passes xattr tests (erofs/019,020,021) of erofs-utils [1]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/log/?h=experimental-tests

v3: https://lore.kernel.org/lkml/20230518024551.123990-1-jefflexu@linux.alibaba.com/
v2: https://lore.kernel.org/all/20230330082910.125374-1-jefflexu@linux.alibaba.com/
v1: https://lore.kernel.org/all/20230323000949.57608-1-jefflexu@linux.alibaba.com/

Jingbo Xu (5):
  erofs: enhance erofs_xattr_iter_fixup() helper
  erofs: unify xattr_iter structures
  erofs: make the size of read data stored in buffer_ofs
  erofs: unify inline/share xattr iterators for listxattr/getxattr
  erofs: use separate xattr parsers for listxattr/getxattr

 fs/erofs/xattr.c | 683 ++++++++++++++++++-----------------------------
 1 file changed, 265 insertions(+), 418 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH v4 1/5] erofs: enhance erofs_xattr_iter_fixup() helper

[Jingbo Xu]

Enhance erofs_xattr_iter_fixup() helper so that it could be reused in
the situation where it.ofs could not span the block boundary.

Besides call erofs_init_metabuf() and erofs_bread() separately to avoid
the repetition of assigning buf->inode when iterating xattrs.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 82 +++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 47 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index bbfe7ce170d2..df6c4e6f1f4e 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -29,6 +29,24 @@ struct xattr_iter {
 	unsigned int ofs;
 };
 
+static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
+{
+	if (it->ofs < it->sb->s_blocksize)
+		return 0;
+
+	if (nospan && it->ofs != it->sb->s_blocksize) {
+		DBG_BUGON(1);
+		return -EFSCORRUPTED;
+	}
+
+	it->blkaddr += erofs_blknr(it->sb, it->ofs);
+	it->kaddr = erofs_bread(&it->buf, it->blkaddr, EROFS_KMAP);
+	if (IS_ERR(it->kaddr))
+		return PTR_ERR(it->kaddr);
+	it->ofs = erofs_blkoff(it->sb, it->ofs);
+	return 0;
+}
+
 static int erofs_init_inode_xattrs(struct inode *inode)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
@@ -80,6 +98,7 @@ static int erofs_init_inode_xattrs(struct inode *inode)
 		goto out_unlock;
 	}
 
+	it.sb = sb;
 	it.buf = __EROFS_BUF_INITIALIZER;
 	it.blkaddr = erofs_blknr(sb, erofs_iloc(inode) + vi->inode_isize);
 	it.ofs = erofs_blkoff(sb, erofs_iloc(inode) + vi->inode_isize);
@@ -105,19 +124,11 @@ static int erofs_init_inode_xattrs(struct inode *inode)
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
-		if (it.ofs >= sb->s_blocksize) {
-			/* cannot be unaligned */
-			DBG_BUGON(it.ofs != sb->s_blocksize);
-
-			it.kaddr = erofs_read_metabuf(&it.buf, sb, ++it.blkaddr,
-						      EROFS_KMAP);
-			if (IS_ERR(it.kaddr)) {
-				kfree(vi->xattr_shared_xattrs);
-				vi->xattr_shared_xattrs = NULL;
-				ret = PTR_ERR(it.kaddr);
-				goto out_unlock;
-			}
-			it.ofs = 0;
+		ret = erofs_xattr_iter_fixup(&it, true);
+		if (ret) {
+			kfree(vi->xattr_shared_xattrs);
+			vi->xattr_shared_xattrs = NULL;
+			goto out_unlock;
 		}
 		vi->xattr_shared_xattrs[i] =
 			le32_to_cpu(*(__le32 *)(it.kaddr + it.ofs));
@@ -150,20 +161,6 @@ struct xattr_iter_handlers {
 		      unsigned int len);
 };
 
-static inline int xattr_iter_fixup(struct xattr_iter *it)
-{
-	if (it->ofs < it->sb->s_blocksize)
-		return 0;
-
-	it->blkaddr += erofs_blknr(it->sb, it->ofs);
-	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr,
-				       EROFS_KMAP);
-	if (IS_ERR(it->kaddr))
-		return PTR_ERR(it->kaddr);
-	it->ofs = erofs_blkoff(it->sb, it->ofs);
-	return 0;
-}
-
 static int inline_xattr_iter_begin(struct xattr_iter *it,
 				   struct inode *inode)
 {
@@ -201,7 +198,7 @@ static int xattr_foreach(struct xattr_iter *it,
 	int err;
 
 	/* 0. fixup blkaddr, ofs, ipage */
-	err = xattr_iter_fixup(it);
+	err = erofs_xattr_iter_fixup(it, false);
 	if (err)
 		return err;
 
@@ -236,14 +233,9 @@ static int xattr_foreach(struct xattr_iter *it,
 	processed = 0;
 
 	while (processed < entry.e_name_len) {
-		if (it->ofs >= it->sb->s_blocksize) {
-			DBG_BUGON(it->ofs > it->sb->s_blocksize);
-
-			err = xattr_iter_fixup(it);
-			if (err)
-				goto out;
-			it->ofs = 0;
-		}
+		err = erofs_xattr_iter_fixup(it, true);
+		if (err)
+			goto out;
 
 		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
 			      entry.e_name_len - processed);
@@ -271,14 +263,9 @@ static int xattr_foreach(struct xattr_iter *it,
 	}
 
 	while (processed < value_sz) {
-		if (it->ofs >= it->sb->s_blocksize) {
-			DBG_BUGON(it->ofs > it->sb->s_blocksize);
-
-			err = xattr_iter_fixup(it);
-			if (err)
-				goto out;
-			it->ofs = 0;
-		}
+		err = erofs_xattr_iter_fixup(it, true);
+		if (err)
+			goto out;
 
 		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
 			      value_sz - processed);
@@ -444,13 +431,14 @@ int erofs_getxattr(struct inode *inode, int index,
 	if (it.name.len > EROFS_NAME_LEN)
 		return -ERANGE;
 
+	it.it.sb = inode->i_sb;
 	it.it.buf = __EROFS_BUF_INITIALIZER;
+	erofs_init_metabuf(&it.it.buf, it.it.sb);
 	it.name.name = name;
 
 	it.buffer = buffer;
 	it.buffer_size = buffer_size;
 
-	it.it.sb = inode->i_sb;
 	ret = inline_getxattr(inode, &it);
 	if (ret == -ENOATTR)
 		ret = shared_getxattr(inode, &it);
@@ -632,14 +620,14 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	if (ret)
 		return ret;
 
+	it.it.sb = dentry->d_sb;
 	it.it.buf = __EROFS_BUF_INITIALIZER;
+	erofs_init_metabuf(&it.it.buf, it.it.sb);
 	it.dentry = dentry;
 	it.buffer = buffer;
 	it.buffer_size = buffer_size;
 	it.buffer_ofs = 0;
 
-	it.it.sb = dentry->d_sb;
-
 	ret = inline_listxattr(&it);
 	if (ret >= 0 || ret == -ENOATTR)
 		ret = shared_listxattr(&it);
-- 
2.19.1.6.gb485710b

[PATCH v4 2/5] erofs: unify xattr_iter structures

[Jingbo Xu]

Unify xattr_iter/listxattr_iter/getxattr_iter structures into
erofs_xattr_iter structure.

This is in preparation for the following further cleanup.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 155 ++++++++++++++++++++---------------------------
 1 file changed, 65 insertions(+), 90 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index df6c4e6f1f4e..dffca38a46fd 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -20,16 +20,25 @@ static inline unsigned int erofs_xattr_blkoff(struct super_block *sb,
 	return erofs_blkoff(sb, xattr_id * sizeof(__u32));
 }
 
-struct xattr_iter {
+struct erofs_xattr_iter {
 	struct super_block *sb;
 	struct erofs_buf buf;
 	void *kaddr;
-
 	erofs_blk_t blkaddr;
 	unsigned int ofs;
+
+	char *buffer;
+	int buffer_size, buffer_ofs;
+
+	/* getxattr */
+	int index, infix_len;
+	struct qstr name;
+
+	/* listxattr */
+	struct dentry *dentry;
 };
 
-static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
+static inline int erofs_xattr_iter_fixup(struct erofs_xattr_iter *it, bool nospan)
 {
 	if (it->ofs < it->sb->s_blocksize)
 		return 0;
@@ -50,7 +59,7 @@ static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
 static int erofs_init_inode_xattrs(struct inode *inode)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
-	struct xattr_iter it;
+	struct erofs_xattr_iter it;
 	unsigned int i;
 	struct erofs_xattr_ibody_header *ih;
 	struct super_block *sb = inode->i_sb;
@@ -153,15 +162,15 @@ static int erofs_init_inode_xattrs(struct inode *inode)
  *                            and need to be handled
  */
 struct xattr_iter_handlers {
-	int (*entry)(struct xattr_iter *_it, struct erofs_xattr_entry *entry);
-	int (*name)(struct xattr_iter *_it, unsigned int processed, char *buf,
+	int (*entry)(struct erofs_xattr_iter *it, struct erofs_xattr_entry *entry);
+	int (*name)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
 		    unsigned int len);
-	int (*alloc_buffer)(struct xattr_iter *_it, unsigned int value_sz);
-	void (*value)(struct xattr_iter *_it, unsigned int processed, char *buf,
+	int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int value_sz);
+	void (*value)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
 		      unsigned int len);
 };
 
-static int inline_xattr_iter_begin(struct xattr_iter *it,
+static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
 				   struct inode *inode)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
@@ -189,7 +198,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
  * Regardless of success or failure, `xattr_foreach' will end up with
  * `ofs' pointing to the next xattr item rather than an arbitrary position.
  */
-static int xattr_foreach(struct xattr_iter *it,
+static int xattr_foreach(struct erofs_xattr_iter *it,
 			 const struct xattr_iter_handlers *op,
 			 unsigned int *tlimit)
 {
@@ -280,18 +289,10 @@ static int xattr_foreach(struct xattr_iter *it,
 	return err < 0 ? err : 0;
 }
 
-struct getxattr_iter {
-	struct xattr_iter it;
-
-	char *buffer;
-	int buffer_size, index, infix_len;
-	struct qstr name;
-};
-
-static int erofs_xattr_long_entrymatch(struct getxattr_iter *it,
+static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
 				       struct erofs_xattr_entry *entry)
 {
-	struct erofs_sb_info *sbi = EROFS_SB(it->it.sb);
+	struct erofs_sb_info *sbi = EROFS_SB(it->sb);
 	struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
 		(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
 
@@ -309,11 +310,9 @@ static int erofs_xattr_long_entrymatch(struct getxattr_iter *it,
 	return 0;
 }
 
-static int xattr_entrymatch(struct xattr_iter *_it,
+static int xattr_entrymatch(struct erofs_xattr_iter *it,
 			    struct erofs_xattr_entry *entry)
 {
-	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
-
 	/* should also match the infix for long name prefixes */
 	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
 		return erofs_xattr_long_entrymatch(it, entry);
@@ -325,32 +324,27 @@ static int xattr_entrymatch(struct xattr_iter *_it,
 	return 0;
 }
 
-static int xattr_namematch(struct xattr_iter *_it,
+static int xattr_namematch(struct erofs_xattr_iter *it,
 			   unsigned int processed, char *buf, unsigned int len)
 {
-	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
-
 	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
 		return -ENOATTR;
 	return 0;
 }
 
-static int xattr_checkbuffer(struct xattr_iter *_it,
+static int xattr_checkbuffer(struct erofs_xattr_iter *it,
 			     unsigned int value_sz)
 {
-	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
 	int err = it->buffer_size < value_sz ? -ERANGE : 0;
 
 	it->buffer_size = value_sz;
 	return !it->buffer ? 1 : err;
 }
 
-static void xattr_copyvalue(struct xattr_iter *_it,
+static void xattr_copyvalue(struct erofs_xattr_iter *it,
 			    unsigned int processed,
 			    char *buf, unsigned int len)
 {
-	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
-
 	memcpy(it->buffer + processed, buf, len);
 }
 
@@ -361,41 +355,40 @@ static const struct xattr_iter_handlers find_xattr_handlers = {
 	.value = xattr_copyvalue
 };
 
-static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
+static int inline_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
 {
 	int ret;
 	unsigned int remaining;
 
-	ret = inline_xattr_iter_begin(&it->it, inode);
+	ret = inline_xattr_iter_begin(it, inode);
 	if (ret < 0)
 		return ret;
 
 	remaining = ret;
 	while (remaining) {
-		ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
+		ret = xattr_foreach(it, &find_xattr_handlers, &remaining);
 		if (ret != -ENOATTR)
 			break;
 	}
 	return ret ? ret : it->buffer_size;
 }
 
-static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
+static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
-	struct super_block *const sb = it->it.sb;
+	struct super_block *const sb = it->sb;
 	unsigned int i, xsid;
 	int ret = -ENOATTR;
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
 		xsid = vi->xattr_shared_xattrs[i];
-		it->it.blkaddr = erofs_xattr_blkaddr(sb, xsid);
-		it->it.ofs = erofs_xattr_blkoff(sb, xsid);
-		it->it.kaddr = erofs_read_metabuf(&it->it.buf, sb,
-						  it->it.blkaddr, EROFS_KMAP);
-		if (IS_ERR(it->it.kaddr))
-			return PTR_ERR(it->it.kaddr);
-
-		ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
+		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
+		it->ofs = erofs_xattr_blkoff(sb, xsid);
+		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);
+		if (IS_ERR(it->kaddr))
+			return PTR_ERR(it->kaddr);
+
+		ret = xattr_foreach(it, &find_xattr_handlers, NULL);
 		if (ret != -ENOATTR)
 			break;
 	}
@@ -417,7 +410,7 @@ int erofs_getxattr(struct inode *inode, int index,
 		   void *buffer, size_t buffer_size)
 {
 	int ret;
-	struct getxattr_iter it;
+	struct erofs_xattr_iter it;
 
 	if (!name)
 		return -EINVAL;
@@ -427,22 +420,21 @@ int erofs_getxattr(struct inode *inode, int index,
 		return ret;
 
 	it.index = index;
-	it.name.len = strlen(name);
+	it.name = (struct qstr)QSTR_INIT(name, strlen(name));
 	if (it.name.len > EROFS_NAME_LEN)
 		return -ERANGE;
 
-	it.it.sb = inode->i_sb;
-	it.it.buf = __EROFS_BUF_INITIALIZER;
-	erofs_init_metabuf(&it.it.buf, it.it.sb);
-	it.name.name = name;
-
+	it.sb = inode->i_sb;
+	it.buf = __EROFS_BUF_INITIALIZER;
+	erofs_init_metabuf(&it.buf, it.sb);
 	it.buffer = buffer;
 	it.buffer_size = buffer_size;
+	it.buffer_ofs = 0;
 
 	ret = inline_getxattr(inode, &it);
 	if (ret == -ENOATTR)
 		ret = shared_getxattr(inode, &it);
-	erofs_put_metabuf(&it.it.buf);
+	erofs_put_metabuf(&it.buf);
 	return ret;
 }
 
@@ -488,25 +480,15 @@ const struct xattr_handler *erofs_xattr_handlers[] = {
 	NULL,
 };
 
-struct listxattr_iter {
-	struct xattr_iter it;
-
-	struct dentry *dentry;
-	char *buffer;
-	int buffer_size, buffer_ofs;
-};
-
-static int xattr_entrylist(struct xattr_iter *_it,
+static int xattr_entrylist(struct erofs_xattr_iter *it,
 			   struct erofs_xattr_entry *entry)
 {
-	struct listxattr_iter *it =
-		container_of(_it, struct listxattr_iter, it);
 	unsigned int base_index = entry->e_name_index;
 	unsigned int prefix_len, infix_len = 0;
 	const char *prefix, *infix = NULL;
 
 	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX) {
-		struct erofs_sb_info *sbi = EROFS_SB(_it->sb);
+		struct erofs_sb_info *sbi = EROFS_SB(it->sb);
 		struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
 			(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
 
@@ -538,23 +520,17 @@ static int xattr_entrylist(struct xattr_iter *_it,
 	return 0;
 }
 
-static int xattr_namelist(struct xattr_iter *_it,
+static int xattr_namelist(struct erofs_xattr_iter *it,
 			  unsigned int processed, char *buf, unsigned int len)
 {
-	struct listxattr_iter *it =
-		container_of(_it, struct listxattr_iter, it);
-
 	memcpy(it->buffer + it->buffer_ofs, buf, len);
 	it->buffer_ofs += len;
 	return 0;
 }
 
-static int xattr_skipvalue(struct xattr_iter *_it,
+static int xattr_skipvalue(struct erofs_xattr_iter *it,
 			   unsigned int value_sz)
 {
-	struct listxattr_iter *it =
-		container_of(_it, struct listxattr_iter, it);
-
 	it->buffer[it->buffer_ofs++] = '\0';
 	return 1;
 }
@@ -566,42 +542,41 @@ static const struct xattr_iter_handlers list_xattr_handlers = {
 	.value = NULL
 };
 
-static int inline_listxattr(struct listxattr_iter *it)
+static int inline_listxattr(struct erofs_xattr_iter *it)
 {
 	int ret;
 	unsigned int remaining;
 
-	ret = inline_xattr_iter_begin(&it->it, d_inode(it->dentry));
+	ret = inline_xattr_iter_begin(it, d_inode(it->dentry));
 	if (ret < 0)
 		return ret;
 
 	remaining = ret;
 	while (remaining) {
-		ret = xattr_foreach(&it->it, &list_xattr_handlers, &remaining);
+		ret = xattr_foreach(it, &list_xattr_handlers, &remaining);
 		if (ret)
 			break;
 	}
 	return ret ? ret : it->buffer_ofs;
 }
 
-static int shared_listxattr(struct listxattr_iter *it)
+static int shared_listxattr(struct erofs_xattr_iter *it)
 {
 	struct inode *const inode = d_inode(it->dentry);
 	struct erofs_inode *const vi = EROFS_I(inode);
-	struct super_block *const sb = it->it.sb;
+	struct super_block *const sb = it->sb;
 	unsigned int i, xsid;
 	int ret = 0;
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
 		xsid = vi->xattr_shared_xattrs[i];
-		it->it.blkaddr = erofs_xattr_blkaddr(sb, xsid);
-		it->it.ofs = erofs_xattr_blkoff(sb, xsid);
-		it->it.kaddr = erofs_read_metabuf(&it->it.buf, sb,
-						  it->it.blkaddr, EROFS_KMAP);
-		if (IS_ERR(it->it.kaddr))
-			return PTR_ERR(it->it.kaddr);
-
-		ret = xattr_foreach(&it->it, &list_xattr_handlers, NULL);
+		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
+		it->ofs = erofs_xattr_blkoff(sb, xsid);
+		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);
+		if (IS_ERR(it->kaddr))
+			return PTR_ERR(it->kaddr);
+
+		ret = xattr_foreach(it, &list_xattr_handlers, NULL);
 		if (ret)
 			break;
 	}
@@ -612,7 +587,7 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 			char *buffer, size_t buffer_size)
 {
 	int ret;
-	struct listxattr_iter it;
+	struct erofs_xattr_iter it;
 
 	ret = erofs_init_inode_xattrs(d_inode(dentry));
 	if (ret == -ENOATTR)
@@ -620,9 +595,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	if (ret)
 		return ret;
 
-	it.it.sb = dentry->d_sb;
-	it.it.buf = __EROFS_BUF_INITIALIZER;
-	erofs_init_metabuf(&it.it.buf, it.it.sb);
+	it.sb = dentry->d_sb;
+	it.buf = __EROFS_BUF_INITIALIZER;
+	erofs_init_metabuf(&it.buf, it.sb);
 	it.dentry = dentry;
 	it.buffer = buffer;
 	it.buffer_size = buffer_size;
@@ -631,7 +606,7 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	ret = inline_listxattr(&it);
 	if (ret >= 0 || ret == -ENOATTR)
 		ret = shared_listxattr(&it);
-	erofs_put_metabuf(&it.it.buf);
+	erofs_put_metabuf(&it.buf);
 	return ret;
 }
 
-- 
2.19.1.6.gb485710b

[PATCH v4 3/5] erofs: make the size of read data stored in buffer_ofs

[Jingbo Xu]

Since now xattr_iter structures have been unified, make the size of the
read data stored in buffer_ofs.  Don't bother reusing buffer_size for
this use, which may be confusing.

This is in preparation for the following further cleanup.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index dffca38a46fd..7c76b4d31920 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -337,7 +337,7 @@ static int xattr_checkbuffer(struct erofs_xattr_iter *it,
 {
 	int err = it->buffer_size < value_sz ? -ERANGE : 0;
 
-	it->buffer_size = value_sz;
+	it->buffer_ofs = value_sz;
 	return !it->buffer ? 1 : err;
 }
 
@@ -370,7 +370,7 @@ static int inline_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
 		if (ret != -ENOATTR)
 			break;
 	}
-	return ret ? ret : it->buffer_size;
+	return ret ? ret : it->buffer_ofs;
 }
 
 static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
@@ -392,7 +392,7 @@ static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
 		if (ret != -ENOATTR)
 			break;
 	}
-	return ret ? ret : it->buffer_size;
+	return ret ? ret : it->buffer_ofs;
 }
 
 static bool erofs_xattr_user_list(struct dentry *dentry)
-- 
2.19.1.6.gb485710b

[PATCH v4 4/5] erofs: unify inline/share xattr iterators for listxattr/getxattr

[Jingbo Xu]

Make inline_getxattr() and inline_listxattr() unified as
iter_inline_xattr(), shared_getxattr() and shared_listxattr() unified as
iter_shared_xattr().

After the unification, both iter_inline_xattr() and iter_shared_xattr()
return 0 on success, and negative error on failure.

One thing worth noting is that, the logic of returning it->buffer_ofs
when there's no shared xattrs in shared_listxattr() is moved to
erofs_listxattr() to make the unification possible.  The only difference
is that, semantically the old behavior will return ENOATTR rather than
it->buffer_ofs if ENOATTR encountered when listxattr is parsing upon a
specific shared xattr, while now the new behavior will return
it->buffer_ofs in this case.  This is not an issue, as listxattr upon a
specific xattr won't return ENOATTR.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 210 ++++++++++++++++++-----------------------------
 1 file changed, 78 insertions(+), 132 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 7c76b4d31920..074743e2b271 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -7,19 +7,6 @@
 #include <linux/security.h>
 #include "xattr.h"
 
-static inline erofs_blk_t erofs_xattr_blkaddr(struct super_block *sb,
-					      unsigned int xattr_id)
-{
-	return EROFS_SB(sb)->xattr_blkaddr +
-	       erofs_blknr(sb, xattr_id * sizeof(__u32));
-}
-
-static inline unsigned int erofs_xattr_blkoff(struct super_block *sb,
-					      unsigned int xattr_id)
-{
-	return erofs_blkoff(sb, xattr_id * sizeof(__u32));
-}
-
 struct erofs_xattr_iter {
 	struct super_block *sb;
 	struct erofs_buf buf;
@@ -170,30 +157,6 @@ struct xattr_iter_handlers {
 		      unsigned int len);
 };
 
-static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
-				   struct inode *inode)
-{
-	struct erofs_inode *const vi = EROFS_I(inode);
-	unsigned int xattr_header_sz, inline_xattr_ofs;
-
-	xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
-			  sizeof(u32) * vi->xattr_shared_count;
-	if (xattr_header_sz >= vi->xattr_isize) {
-		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
-		return -ENOATTR;
-	}
-
-	inline_xattr_ofs = vi->inode_isize + xattr_header_sz;
-
-	it->blkaddr = erofs_blknr(it->sb, erofs_iloc(inode) + inline_xattr_ofs);
-	it->ofs = erofs_blkoff(it->sb, erofs_iloc(inode) + inline_xattr_ofs);
-	it->kaddr = erofs_read_metabuf(&it->buf, inode->i_sb, it->blkaddr,
-				       EROFS_KMAP);
-	if (IS_ERR(it->kaddr))
-		return PTR_ERR(it->kaddr);
-	return vi->xattr_isize - xattr_header_sz;
-}
-
 /*
  * Regardless of success or failure, `xattr_foreach' will end up with
  * `ofs' pointing to the next xattr item rather than an arbitrary position.
@@ -355,46 +318,6 @@ static const struct xattr_iter_handlers find_xattr_handlers = {
 	.value = xattr_copyvalue
 };
 
-static int inline_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
-{
-	int ret;
-	unsigned int remaining;
-
-	ret = inline_xattr_iter_begin(it, inode);
-	if (ret < 0)
-		return ret;
-
-	remaining = ret;
-	while (remaining) {
-		ret = xattr_foreach(it, &find_xattr_handlers, &remaining);
-		if (ret != -ENOATTR)
-			break;
-	}
-	return ret ? ret : it->buffer_ofs;
-}
-
-static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
-{
-	struct erofs_inode *const vi = EROFS_I(inode);
-	struct super_block *const sb = it->sb;
-	unsigned int i, xsid;
-	int ret = -ENOATTR;
-
-	for (i = 0; i < vi->xattr_shared_count; ++i) {
-		xsid = vi->xattr_shared_xattrs[i];
-		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
-		it->ofs = erofs_xattr_blkoff(sb, xsid);
-		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);
-		if (IS_ERR(it->kaddr))
-			return PTR_ERR(it->kaddr);
-
-		ret = xattr_foreach(it, &find_xattr_handlers, NULL);
-		if (ret != -ENOATTR)
-			break;
-	}
-	return ret ? ret : it->buffer_ofs;
-}
-
 static bool erofs_xattr_user_list(struct dentry *dentry)
 {
 	return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
@@ -405,39 +328,6 @@ static bool erofs_xattr_trusted_list(struct dentry *dentry)
 	return capable(CAP_SYS_ADMIN);
 }
 
-int erofs_getxattr(struct inode *inode, int index,
-		   const char *name,
-		   void *buffer, size_t buffer_size)
-{
-	int ret;
-	struct erofs_xattr_iter it;
-
-	if (!name)
-		return -EINVAL;
-
-	ret = erofs_init_inode_xattrs(inode);
-	if (ret)
-		return ret;
-
-	it.index = index;
-	it.name = (struct qstr)QSTR_INIT(name, strlen(name));
-	if (it.name.len > EROFS_NAME_LEN)
-		return -ERANGE;
-
-	it.sb = inode->i_sb;
-	it.buf = __EROFS_BUF_INITIALIZER;
-	erofs_init_metabuf(&it.buf, it.sb);
-	it.buffer = buffer;
-	it.buffer_size = buffer_size;
-	it.buffer_ofs = 0;
-
-	ret = inline_getxattr(inode, &it);
-	if (ret == -ENOATTR)
-		ret = shared_getxattr(inode, &it);
-	erofs_put_metabuf(&it.buf);
-	return ret;
-}
-
 static int erofs_xattr_generic_get(const struct xattr_handler *handler,
 				   struct dentry *unused, struct inode *inode,
 				   const char *name, void *buffer, size_t size)
@@ -542,45 +432,98 @@ static const struct xattr_iter_handlers list_xattr_handlers = {
 	.value = NULL
 };
 
-static int inline_listxattr(struct erofs_xattr_iter *it)
+static int erofs_iter_inline_xattr(struct erofs_xattr_iter *it,
+				   struct inode *inode, bool getxattr)
 {
+	struct erofs_inode *const vi = EROFS_I(inode);
+	const struct xattr_iter_handlers *op;
+	unsigned int xattr_header_sz, remaining;
+	erofs_off_t pos;
 	int ret;
-	unsigned int remaining;
 
-	ret = inline_xattr_iter_begin(it, d_inode(it->dentry));
-	if (ret < 0)
-		return ret;
+	xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
+			  sizeof(u32) * vi->xattr_shared_count;
+	if (xattr_header_sz >= vi->xattr_isize) {
+		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
+		return -ENOATTR;
+	}
+
+	pos = erofs_iloc(inode) + vi->inode_isize + xattr_header_sz;
+	it->blkaddr = erofs_blknr(it->sb, pos);
+	it->ofs = erofs_blkoff(it->sb, pos);
+	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr, EROFS_KMAP);
+	if (IS_ERR(it->kaddr))
+		return PTR_ERR(it->kaddr);
+
+	remaining = vi->xattr_isize - xattr_header_sz;
+	op = getxattr ? &find_xattr_handlers : &list_xattr_handlers;
 
-	remaining = ret;
 	while (remaining) {
-		ret = xattr_foreach(it, &list_xattr_handlers, &remaining);
-		if (ret)
+		ret = xattr_foreach(it, op, &remaining);
+		if ((getxattr && ret != -ENOATTR) || (!getxattr && ret))
 			break;
 	}
-	return ret ? ret : it->buffer_ofs;
+	return ret;
 }
 
-static int shared_listxattr(struct erofs_xattr_iter *it)
+static int erofs_iter_shared_xattr(struct erofs_xattr_iter *it,
+				   struct inode *inode, bool getxattr)
 {
-	struct inode *const inode = d_inode(it->dentry);
 	struct erofs_inode *const vi = EROFS_I(inode);
 	struct super_block *const sb = it->sb;
+	const struct xattr_iter_handlers *op;
 	unsigned int i, xsid;
-	int ret = 0;
+	int ret = -ENOATTR;
+
+	op = getxattr ? &find_xattr_handlers : &list_xattr_handlers;
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
 		xsid = vi->xattr_shared_xattrs[i];
-		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
-		it->ofs = erofs_xattr_blkoff(sb, xsid);
+		it->blkaddr = EROFS_SB(sb)->xattr_blkaddr +
+			      erofs_blknr(sb, xsid * sizeof(__u32));
+		it->ofs = erofs_blkoff(sb, xsid * sizeof(__u32));
 		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);
 		if (IS_ERR(it->kaddr))
 			return PTR_ERR(it->kaddr);
 
-		ret = xattr_foreach(it, &list_xattr_handlers, NULL);
-		if (ret)
+		ret = xattr_foreach(it, op, NULL);
+		if ((getxattr && ret != -ENOATTR) || (!getxattr && ret))
 			break;
 	}
-	return ret ? ret : it->buffer_ofs;
+	return ret;
+}
+
+int erofs_getxattr(struct inode *inode, int index,
+		   const char *name,
+		   void *buffer, size_t buffer_size)
+{
+	int ret;
+	struct erofs_xattr_iter it;
+
+	if (!name)
+		return -EINVAL;
+
+	ret = erofs_init_inode_xattrs(inode);
+	if (ret)
+		return ret;
+
+	it.index = index;
+	it.name = (struct qstr)QSTR_INIT(name, strlen(name));
+	if (it.name.len > EROFS_NAME_LEN)
+		return -ERANGE;
+
+	it.sb = inode->i_sb;
+	it.buf = __EROFS_BUF_INITIALIZER;
+	erofs_init_metabuf(&it.buf, it.sb);
+	it.buffer = buffer;
+	it.buffer_size = buffer_size;
+	it.buffer_ofs = 0;
+
+	ret = erofs_iter_inline_xattr(&it, inode, true);
+	if (ret == -ENOATTR)
+		ret = erofs_iter_shared_xattr(&it, inode, true);
+	erofs_put_metabuf(&it.buf);
+	return ret ? ret : it.buffer_ofs;
 }
 
 ssize_t erofs_listxattr(struct dentry *dentry,
@@ -588,8 +531,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 {
 	int ret;
 	struct erofs_xattr_iter it;
+	struct inode *inode = d_inode(dentry);
 
-	ret = erofs_init_inode_xattrs(d_inode(dentry));
+	ret = erofs_init_inode_xattrs(inode);
 	if (ret == -ENOATTR)
 		return 0;
 	if (ret)
@@ -603,11 +547,13 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	it.buffer_size = buffer_size;
 	it.buffer_ofs = 0;
 
-	ret = inline_listxattr(&it);
-	if (ret >= 0 || ret == -ENOATTR)
-		ret = shared_listxattr(&it);
+	ret = erofs_iter_inline_xattr(&it, inode, false);
+	if (!ret || ret == -ENOATTR)
+		ret = erofs_iter_shared_xattr(&it, inode, false);
+	if (ret == -ENOATTR)
+		ret = 0;
 	erofs_put_metabuf(&it.buf);
-	return ret;
+	return ret ? ret : it.buffer_ofs;
 }
 
 void erofs_xattr_prefixes_cleanup(struct super_block *sb)
-- 
2.19.1.6.gb485710b

[PATCH v4 5/5] erofs: use separate xattr parsers for listxattr/getxattr

[Jingbo Xu]

There's a callback styled xattr parser, i.e. xattr_foreach(), which is
shared among listxattr and getxattr.  Convert it to two separate xattr
parsers for listxattr and getxattr.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 372 ++++++++++++++++++++---------------------------
 1 file changed, 155 insertions(+), 217 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 074743e2b271..438fcf22b230 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -12,7 +12,7 @@ struct erofs_xattr_iter {
 	struct erofs_buf buf;
 	void *kaddr;
 	erofs_blk_t blkaddr;
-	unsigned int ofs;
+	unsigned int ofs, next_ofs;
 
 	char *buffer;
 	int buffer_size, buffer_ofs;
@@ -141,183 +141,6 @@ static int erofs_init_inode_xattrs(struct inode *inode)
 	return ret;
 }
 
-/*
- * the general idea for these return values is
- * if    0 is returned, go on processing the current xattr;
- *       1 (> 0) is returned, skip this round to process the next xattr;
- *    -err (< 0) is returned, an error (maybe ENOXATTR) occurred
- *                            and need to be handled
- */
-struct xattr_iter_handlers {
-	int (*entry)(struct erofs_xattr_iter *it, struct erofs_xattr_entry *entry);
-	int (*name)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
-		    unsigned int len);
-	int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int value_sz);
-	void (*value)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
-		      unsigned int len);
-};
-
-/*
- * Regardless of success or failure, `xattr_foreach' will end up with
- * `ofs' pointing to the next xattr item rather than an arbitrary position.
- */
-static int xattr_foreach(struct erofs_xattr_iter *it,
-			 const struct xattr_iter_handlers *op,
-			 unsigned int *tlimit)
-{
-	struct erofs_xattr_entry entry;
-	unsigned int value_sz, processed, slice;
-	int err;
-
-	/* 0. fixup blkaddr, ofs, ipage */
-	err = erofs_xattr_iter_fixup(it, false);
-	if (err)
-		return err;
-
-	/*
-	 * 1. read xattr entry to the memory,
-	 *    since we do EROFS_XATTR_ALIGN
-	 *    therefore entry should be in the page
-	 */
-	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
-	if (tlimit) {
-		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
-
-		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
-		if (*tlimit < entry_sz) {
-			DBG_BUGON(1);
-			return -EFSCORRUPTED;
-		}
-		*tlimit -= entry_sz;
-	}
-
-	it->ofs += sizeof(struct erofs_xattr_entry);
-	value_sz = le16_to_cpu(entry.e_value_size);
-
-	/* handle entry */
-	err = op->entry(it, &entry);
-	if (err) {
-		it->ofs += entry.e_name_len + value_sz;
-		goto out;
-	}
-
-	/* 2. handle xattr name (ofs will finally be at the end of name) */
-	processed = 0;
-
-	while (processed < entry.e_name_len) {
-		err = erofs_xattr_iter_fixup(it, true);
-		if (err)
-			goto out;
-
-		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
-			      entry.e_name_len - processed);
-
-		/* handle name */
-		err = op->name(it, processed, it->kaddr + it->ofs, slice);
-		if (err) {
-			it->ofs += entry.e_name_len - processed + value_sz;
-			goto out;
-		}
-
-		it->ofs += slice;
-		processed += slice;
-	}
-
-	/* 3. handle xattr value */
-	processed = 0;
-
-	if (op->alloc_buffer) {
-		err = op->alloc_buffer(it, value_sz);
-		if (err) {
-			it->ofs += value_sz;
-			goto out;
-		}
-	}
-
-	while (processed < value_sz) {
-		err = erofs_xattr_iter_fixup(it, true);
-		if (err)
-			goto out;
-
-		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
-			      value_sz - processed);
-		op->value(it, processed, it->kaddr + it->ofs, slice);
-		it->ofs += slice;
-		processed += slice;
-	}
-
-out:
-	/* xattrs should be 4-byte aligned (on-disk constraint) */
-	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
-	return err < 0 ? err : 0;
-}
-
-static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
-				       struct erofs_xattr_entry *entry)
-{
-	struct erofs_sb_info *sbi = EROFS_SB(it->sb);
-	struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
-		(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
-
-	if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
-		return -ENOATTR;
-
-	if (it->index != pf->prefix->base_index ||
-	    it->name.len != entry->e_name_len + pf->infix_len)
-		return -ENOATTR;
-
-	if (memcmp(it->name.name, pf->prefix->infix, pf->infix_len))
-		return -ENOATTR;
-
-	it->infix_len = pf->infix_len;
-	return 0;
-}
-
-static int xattr_entrymatch(struct erofs_xattr_iter *it,
-			    struct erofs_xattr_entry *entry)
-{
-	/* should also match the infix for long name prefixes */
-	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
-		return erofs_xattr_long_entrymatch(it, entry);
-
-	if (it->index != entry->e_name_index ||
-	    it->name.len != entry->e_name_len)
-		return -ENOATTR;
-	it->infix_len = 0;
-	return 0;
-}
-
-static int xattr_namematch(struct erofs_xattr_iter *it,
-			   unsigned int processed, char *buf, unsigned int len)
-{
-	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
-		return -ENOATTR;
-	return 0;
-}
-
-static int xattr_checkbuffer(struct erofs_xattr_iter *it,
-			     unsigned int value_sz)
-{
-	int err = it->buffer_size < value_sz ? -ERANGE : 0;
-
-	it->buffer_ofs = value_sz;
-	return !it->buffer ? 1 : err;
-}
-
-static void xattr_copyvalue(struct erofs_xattr_iter *it,
-			    unsigned int processed,
-			    char *buf, unsigned int len)
-{
-	memcpy(it->buffer + processed, buf, len);
-}
-
-static const struct xattr_iter_handlers find_xattr_handlers = {
-	.entry = xattr_entrymatch,
-	.name = xattr_namematch,
-	.alloc_buffer = xattr_checkbuffer,
-	.value = xattr_copyvalue
-};
-
 static bool erofs_xattr_user_list(struct dentry *dentry)
 {
 	return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
@@ -370,20 +193,70 @@ const struct xattr_handler *erofs_xattr_handlers[] = {
 	NULL,
 };
 
-static int xattr_entrylist(struct erofs_xattr_iter *it,
-			   struct erofs_xattr_entry *entry)
+typedef int (*erofs_xattr_body_handler)(struct erofs_xattr_iter *it,
+		unsigned int processed, char *buf, unsigned int len);
+
+static int erofs_xattr_namematch(struct erofs_xattr_iter *it,
+		unsigned int processed, char *buf, unsigned int len)
+{
+	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
+		return -ENOATTR;
+	return 0;
+}
+
+static int erofs_xattr_copy(struct erofs_xattr_iter *it,
+		unsigned int unused, char *buf, unsigned int len)
+{
+	memcpy(it->buffer + it->buffer_ofs, buf, len);
+	it->buffer_ofs += len;
+	return 0;
+}
+
+static int erofs_xattr_body(struct erofs_xattr_iter *it, unsigned int len,
+			    erofs_xattr_body_handler handler)
+{
+	unsigned int slice, processed = 0;
+
+	while (processed < len) {
+		int err = erofs_xattr_iter_fixup(it, true);
+		if (err)
+			return err;
+
+		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
+			      len - processed);
+		err = handler(it, processed, it->kaddr + it->ofs, slice);
+		if (err)
+			return err;
+
+		it->ofs += slice;
+		processed += slice;
+	}
+	return 0;
+}
+
+/*
+ * Wen returning 0 or ENOATTR, erofs_[list|get]xattr_foreach() will end up
+ * with `ofs' pointing to the next xattr item rather than an arbitrary position.
+ */
+static int erofs_listxattr_foreach(struct erofs_xattr_iter *it)
 {
-	unsigned int base_index = entry->e_name_index;
-	unsigned int prefix_len, infix_len = 0;
+	struct erofs_xattr_entry entry;
+	unsigned int base_index, prefix_len, infix_len = 0;
 	const char *prefix, *infix = NULL;
+	int err;
+
+	/* 1. handle xattr entry */
+	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
+	it->ofs += sizeof(struct erofs_xattr_entry);
+	base_index = entry.e_name_index;
 
-	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX) {
+	if (entry.e_name_index & EROFS_XATTR_LONG_PREFIX) {
 		struct erofs_sb_info *sbi = EROFS_SB(it->sb);
 		struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
-			(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
+			(entry.e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
 
 		if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
-			return 1;
+			goto out;
 		infix = pf->prefix->infix;
 		infix_len = pf->infix_len;
 		base_index = pf->prefix->base_index;
@@ -391,53 +264,99 @@ static int xattr_entrylist(struct erofs_xattr_iter *it,
 
 	prefix = erofs_xattr_prefix(base_index, it->dentry);
 	if (!prefix)
-		return 1;
+		goto out;
 	prefix_len = strlen(prefix);
 
 	if (!it->buffer) {
-		it->buffer_ofs += prefix_len + infix_len +
-					entry->e_name_len + 1;
-		return 1;
+		it->buffer_ofs += prefix_len + infix_len + entry.e_name_len + 1;
+		goto out;
 	}
 
 	if (it->buffer_ofs + prefix_len + infix_len +
-		+ entry->e_name_len + 1 > it->buffer_size)
+		entry.e_name_len + 1 > it->buffer_size)
 		return -ERANGE;
 
 	memcpy(it->buffer + it->buffer_ofs, prefix, prefix_len);
 	memcpy(it->buffer + it->buffer_ofs + prefix_len, infix, infix_len);
 	it->buffer_ofs += prefix_len + infix_len;
-	return 0;
-}
 
-static int xattr_namelist(struct erofs_xattr_iter *it,
-			  unsigned int processed, char *buf, unsigned int len)
-{
-	memcpy(it->buffer + it->buffer_ofs, buf, len);
-	it->buffer_ofs += len;
+	/* 2. handle xattr name (err can't be ENOATTR) */
+	err = erofs_xattr_body(it, entry.e_name_len, erofs_xattr_copy);
+	if (err)
+		return err;
+
+	it->buffer[it->buffer_ofs++] = '\0';
+	it->ofs += le16_to_cpu(entry.e_value_size);
+	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
+	return 0;
+out:
+	it->ofs = it->next_ofs;
 	return 0;
 }
 
-static int xattr_skipvalue(struct erofs_xattr_iter *it,
-			   unsigned int value_sz)
+static int erofs_getxattr_foreach(struct erofs_xattr_iter *it)
 {
-	it->buffer[it->buffer_ofs++] = '\0';
-	return 1;
-}
+	struct erofs_xattr_entry entry;
+	unsigned int value_sz;
+	int err;
 
-static const struct xattr_iter_handlers list_xattr_handlers = {
-	.entry = xattr_entrylist,
-	.name = xattr_namelist,
-	.alloc_buffer = xattr_skipvalue,
-	.value = NULL
-};
+	/* 1. handle xattr entry */
+	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
+	it->ofs += sizeof(struct erofs_xattr_entry);
+	value_sz = le16_to_cpu(entry.e_value_size);
+
+	err = -ENOATTR;
+	/* should also match the infix for long name prefixes */
+	if (entry.e_name_index & EROFS_XATTR_LONG_PREFIX) {
+		struct erofs_sb_info *sbi = EROFS_SB(it->sb);
+		struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
+			(entry.e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
+
+		if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
+			goto out;
+
+		if (it->index != pf->prefix->base_index ||
+		    it->name.len != entry.e_name_len + pf->infix_len)
+			goto out;
+
+		if (memcmp(it->name.name, pf->prefix->infix, pf->infix_len))
+			goto out;
+
+		it->infix_len = pf->infix_len;
+	} else {
+		if (it->index != entry.e_name_index ||
+		    it->name.len != entry.e_name_len)
+			goto out;
+		it->infix_len = 0;
+	}
+
+	/* 2. handle xattr name */
+	err = erofs_xattr_body(it, entry.e_name_len, erofs_xattr_namematch);
+	if (err)
+		goto out;
+
+	/* 3. handle xattr value */
+	if (!it->buffer) {
+		it->buffer_ofs = value_sz;
+		goto out; /* err == 0 */
+	}
+	if (it->buffer_size < value_sz)
+		return -ERANGE;
+
+	/* no need normalizing ofs on error (err can't be ENOATTR) */
+	err = erofs_xattr_body(it, value_sz, erofs_xattr_copy);
+	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
+	return err;
+out:
+	it->ofs = it->next_ofs;
+	return err;
+}
 
 static int erofs_iter_inline_xattr(struct erofs_xattr_iter *it,
 				   struct inode *inode, bool getxattr)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
-	const struct xattr_iter_handlers *op;
-	unsigned int xattr_header_sz, remaining;
+	unsigned int xattr_header_sz, remaining, entry_sz;
 	erofs_off_t pos;
 	int ret;
 
@@ -456,10 +375,24 @@ static int erofs_iter_inline_xattr(struct erofs_xattr_iter *it,
 		return PTR_ERR(it->kaddr);
 
 	remaining = vi->xattr_isize - xattr_header_sz;
-	op = getxattr ? &find_xattr_handlers : &list_xattr_handlers;
-
 	while (remaining) {
-		ret = xattr_foreach(it, op, &remaining);
+		ret = erofs_xattr_iter_fixup(it, false);
+		if (ret)
+			return ret;
+
+		entry_sz = erofs_xattr_entry_size(it->kaddr + it->ofs);
+		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
+		if (remaining < entry_sz) {
+			DBG_BUGON(1);
+			return -EFSCORRUPTED;
+		}
+		remaining -= entry_sz;
+		it->next_ofs = it->ofs + entry_sz;
+
+		if (getxattr)
+			ret = erofs_getxattr_foreach(it);
+		else
+			ret = erofs_listxattr_foreach(it);
 		if ((getxattr && ret != -ENOATTR) || (!getxattr && ret))
 			break;
 	}
@@ -471,12 +404,9 @@ static int erofs_iter_shared_xattr(struct erofs_xattr_iter *it,
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
 	struct super_block *const sb = it->sb;
-	const struct xattr_iter_handlers *op;
 	unsigned int i, xsid;
 	int ret = -ENOATTR;
 
-	op = getxattr ? &find_xattr_handlers : &list_xattr_handlers;
-
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
 		xsid = vi->xattr_shared_xattrs[i];
 		it->blkaddr = EROFS_SB(sb)->xattr_blkaddr +
@@ -486,7 +416,15 @@ static int erofs_iter_shared_xattr(struct erofs_xattr_iter *it,
 		if (IS_ERR(it->kaddr))
 			return PTR_ERR(it->kaddr);
 
-		ret = xattr_foreach(it, op, NULL);
+		ret = erofs_xattr_iter_fixup(it, false);
+		if (ret)
+			return ret;
+		it->next_ofs = it->ofs + erofs_xattr_entry_size(it->kaddr + it->ofs);
+
+		if (getxattr)
+			ret = erofs_getxattr_foreach(it);
+		else
+			ret = erofs_listxattr_foreach(it);
 		if ((getxattr && ret != -ENOATTR) || (!getxattr && ret))
 			break;
 	}
-- 
2.19.1.6.gb485710b

Re: [PATCH v4 1/5] erofs: enhance erofs_xattr_iter_fixup() helper

[Gao Xiang]

On 2023/5/31 11:13, Jingbo Xu wrote:
> Enhance erofs_xattr_iter_fixup() helper so that it could be reused in
> the situation where it.ofs could not span the block boundary.
> 
> Besides call erofs_init_metabuf() and erofs_bread() separately to avoid
> the repetition of assigning buf->inode when iterating xattrs.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>   fs/erofs/xattr.c | 82 +++++++++++++++++++++---------------------------
>   1 file changed, 35 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index bbfe7ce170d2..df6c4e6f1f4e 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -29,6 +29,24 @@ struct xattr_iter {
>   	unsigned int ofs;
>   };
>   
> +static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
> +{
> +	if (it->ofs < it->sb->s_blocksize)
> +		return 0;
> +
> +	if (nospan && it->ofs != it->sb->s_blocksize) {
> +		DBG_BUGON(1);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	it->blkaddr += erofs_blknr(it->sb, it->ofs);
> +	it->kaddr = erofs_bread(&it->buf, it->blkaddr, EROFS_KMAP);
> +	if (IS_ERR(it->kaddr))
> +		return PTR_ERR(it->kaddr);
> +	it->ofs = erofs_blkoff(it->sb, it->ofs);
> +	return 0;
> +}
> +
>   static int erofs_init_inode_xattrs(struct inode *inode)
>   {
>   	struct erofs_inode *const vi = EROFS_I(inode);
> @@ -80,6 +98,7 @@ static int erofs_init_inode_xattrs(struct inode *inode)
>   		goto out_unlock;
>   	}
>   
> +	it.sb = sb;
>   	it.buf = __EROFS_BUF_INITIALIZER;
>   	it.blkaddr = erofs_blknr(sb, erofs_iloc(inode) + vi->inode_isize);
>   	it.ofs = erofs_blkoff(sb, erofs_iloc(inode) + vi->inode_isize);
> @@ -105,19 +124,11 @@ static int erofs_init_inode_xattrs(struct inode *inode)
>   	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>   
>   	for (i = 0; i < vi->xattr_shared_count; ++i) {
> -		if (it.ofs >= sb->s_blocksize) {
> -			/* cannot be unaligned */
> -			DBG_BUGON(it.ofs != sb->s_blocksize);
> -
> -			it.kaddr = erofs_read_metabuf(&it.buf, sb, ++it.blkaddr,
> -						      EROFS_KMAP);
> -			if (IS_ERR(it.kaddr)) {
> -				kfree(vi->xattr_shared_xattrs);
> -				vi->xattr_shared_xattrs = NULL;
> -				ret = PTR_ERR(it.kaddr);
> -				goto out_unlock;
> -			}
> -			it.ofs = 0;
> +		ret = erofs_xattr_iter_fixup(&it, true);
> +		if (ret) {
> +			kfree(vi->xattr_shared_xattrs);
> +			vi->xattr_shared_xattrs = NULL;
> +			goto out_unlock;
>   		}
>   		vi->xattr_shared_xattrs[i] =
>   			le32_to_cpu(*(__le32 *)(it.kaddr + it.ofs));
> @@ -150,20 +161,6 @@ struct xattr_iter_handlers {
>   		      unsigned int len);
>   };
>   
> -static inline int xattr_iter_fixup(struct xattr_iter *it)
> -{
> -	if (it->ofs < it->sb->s_blocksize)
> -		return 0;
> -
> -	it->blkaddr += erofs_blknr(it->sb, it->ofs);
> -	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr,
> -				       EROFS_KMAP);
> -	if (IS_ERR(it->kaddr))
> -		return PTR_ERR(it->kaddr);
> -	it->ofs = erofs_blkoff(it->sb, it->ofs);
> -	return 0;
> -}
> -
>   static int inline_xattr_iter_begin(struct xattr_iter *it,
>   				   struct inode *inode)
>   {
> @@ -201,7 +198,7 @@ static int xattr_foreach(struct xattr_iter *it,
>   	int err;
>   
>   	/* 0. fixup blkaddr, ofs, ipage */
> -	err = xattr_iter_fixup(it);
> +	err = erofs_xattr_iter_fixup(it, false);
>   	if (err)
>   		return err;
>   
> @@ -236,14 +233,9 @@ static int xattr_foreach(struct xattr_iter *it,
>   	processed = 0;
>   
>   	while (processed < entry.e_name_len) {
> -		if (it->ofs >= it->sb->s_blocksize) {
> -			DBG_BUGON(it->ofs > it->sb->s_blocksize);
> -
> -			err = xattr_iter_fixup(it);
> -			if (err)
> -				goto out;
> -			it->ofs = 0;
> -		}
> +		err = erofs_xattr_iter_fixup(it, true);
> +		if (err)
> +			goto out;
>   
>   		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
>   			      entry.e_name_len - processed);
> @@ -271,14 +263,9 @@ static int xattr_foreach(struct xattr_iter *it,
>   	}
>   
>   	while (processed < value_sz) {
> -		if (it->ofs >= it->sb->s_blocksize) {
> -			DBG_BUGON(it->ofs > it->sb->s_blocksize);
> -
> -			err = xattr_iter_fixup(it);
> -			if (err)
> -				goto out;
> -			it->ofs = 0;
> -		}
> +		err = erofs_xattr_iter_fixup(it, true);
> +		if (err)
> +			goto out;
>   
>   		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
>   			      value_sz - processed);
> @@ -444,13 +431,14 @@ int erofs_getxattr(struct inode *inode, int index,
>   	if (it.name.len > EROFS_NAME_LEN)
>   		return -ERANGE;
>   
> +	it.it.sb = inode->i_sb;
>   	it.it.buf = __EROFS_BUF_INITIALIZER;
> +	erofs_init_metabuf(&it.it.buf, it.it.sb);
>   	it.name.name = name;
>   
>   	it.buffer = buffer;
>   	it.buffer_size = buffer_size;
>   
> -	it.it.sb = inode->i_sb;
>   	ret = inline_getxattr(inode, &it);
>   	if (ret == -ENOATTR)
>   		ret = shared_getxattr(inode, &it);
> @@ -632,14 +620,14 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>   	if (ret)
>   		return ret;
>   
> +	it.it.sb = dentry->d_sb;
>   	it.it.buf = __EROFS_BUF_INITIALIZER;
> +	erofs_init_metabuf(&it.it.buf, it.it.sb);
>   	it.dentry = dentry;
>   	it.buffer = buffer;
>   	it.buffer_size = buffer_size;
>   	it.buffer_ofs = 0;
>   
> -	it.it.sb = dentry->d_sb;
> -
>   	ret = inline_listxattr(&it);
>   	if (ret >= 0 || ret == -ENOATTR)
>   		ret = shared_listxattr(&it);

Re: [PATCH v4 2/5] erofs: unify xattr_iter structures

[Gao Xiang]

On 2023/5/31 11:13, Jingbo Xu wrote:
> Unify xattr_iter/listxattr_iter/getxattr_iter structures into
> erofs_xattr_iter structure.
> 
> This is in preparation for the following further cleanup.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>   fs/erofs/xattr.c | 155 ++++++++++++++++++++---------------------------
>   1 file changed, 65 insertions(+), 90 deletions(-)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index df6c4e6f1f4e..dffca38a46fd 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -20,16 +20,25 @@ static inline unsigned int erofs_xattr_blkoff(struct super_block *sb,
>   	return erofs_blkoff(sb, xattr_id * sizeof(__u32));
>   }
>   
> -struct xattr_iter {
> +struct erofs_xattr_iter {
>   	struct super_block *sb;
>   	struct erofs_buf buf;
>   	void *kaddr;
> -
>   	erofs_blk_t blkaddr;
>   	unsigned int ofs;
> +
> +	char *buffer;
> +	int buffer_size, buffer_ofs;
> +
> +	/* getxattr */
> +	int index, infix_len;
> +	struct qstr name;
> +
> +	/* listxattr */
> +	struct dentry *dentry;
>   };
>   
> -static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
> +static inline int erofs_xattr_iter_fixup(struct erofs_xattr_iter *it, bool nospan)
>   {
>   	if (it->ofs < it->sb->s_blocksize)
>   		return 0;
> @@ -50,7 +59,7 @@ static inline int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
>   static int erofs_init_inode_xattrs(struct inode *inode)
>   {
>   	struct erofs_inode *const vi = EROFS_I(inode);
> -	struct xattr_iter it;
> +	struct erofs_xattr_iter it;
>   	unsigned int i;
>   	struct erofs_xattr_ibody_header *ih;
>   	struct super_block *sb = inode->i_sb;
> @@ -153,15 +162,15 @@ static int erofs_init_inode_xattrs(struct inode *inode)
>    *                            and need to be handled
>    */
>   struct xattr_iter_handlers {
> -	int (*entry)(struct xattr_iter *_it, struct erofs_xattr_entry *entry);
> -	int (*name)(struct xattr_iter *_it, unsigned int processed, char *buf,
> +	int (*entry)(struct erofs_xattr_iter *it, struct erofs_xattr_entry *entry);
> +	int (*name)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
>   		    unsigned int len);
> -	int (*alloc_buffer)(struct xattr_iter *_it, unsigned int value_sz);
> -	void (*value)(struct xattr_iter *_it, unsigned int processed, char *buf,
> +	int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int value_sz);
> +	void (*value)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
>   		      unsigned int len);
>   };
>   
> -static int inline_xattr_iter_begin(struct xattr_iter *it,
> +static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
>   				   struct inode *inode)
>   {
>   	struct erofs_inode *const vi = EROFS_I(inode);
> @@ -189,7 +198,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>    * Regardless of success or failure, `xattr_foreach' will end up with
>    * `ofs' pointing to the next xattr item rather than an arbitrary position.
>    */
> -static int xattr_foreach(struct xattr_iter *it,
> +static int xattr_foreach(struct erofs_xattr_iter *it,
>   			 const struct xattr_iter_handlers *op,
>   			 unsigned int *tlimit)
>   {
> @@ -280,18 +289,10 @@ static int xattr_foreach(struct xattr_iter *it,
>   	return err < 0 ? err : 0;
>   }
>   
> -struct getxattr_iter {
> -	struct xattr_iter it;
> -
> -	char *buffer;
> -	int buffer_size, index, infix_len;
> -	struct qstr name;
> -};
> -
> -static int erofs_xattr_long_entrymatch(struct getxattr_iter *it,
> +static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
>   				       struct erofs_xattr_entry *entry)
>   {
> -	struct erofs_sb_info *sbi = EROFS_SB(it->it.sb);
> +	struct erofs_sb_info *sbi = EROFS_SB(it->sb);
>   	struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
>   		(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
>   
> @@ -309,11 +310,9 @@ static int erofs_xattr_long_entrymatch(struct getxattr_iter *it,
>   	return 0;
>   }
>   
> -static int xattr_entrymatch(struct xattr_iter *_it,
> +static int xattr_entrymatch(struct erofs_xattr_iter *it,
>   			    struct erofs_xattr_entry *entry)
>   {
> -	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
> -
>   	/* should also match the infix for long name prefixes */
>   	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
>   		return erofs_xattr_long_entrymatch(it, entry);
> @@ -325,32 +324,27 @@ static int xattr_entrymatch(struct xattr_iter *_it,
>   	return 0;
>   }
>   
> -static int xattr_namematch(struct xattr_iter *_it,
> +static int xattr_namematch(struct erofs_xattr_iter *it,
>   			   unsigned int processed, char *buf, unsigned int len)
>   {
> -	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
> -
>   	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
>   		return -ENOATTR;
>   	return 0;
>   }
>   
> -static int xattr_checkbuffer(struct xattr_iter *_it,
> +static int xattr_checkbuffer(struct erofs_xattr_iter *it,
>   			     unsigned int value_sz)
>   {
> -	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
>   	int err = it->buffer_size < value_sz ? -ERANGE : 0;
>   
>   	it->buffer_size = value_sz;
>   	return !it->buffer ? 1 : err;
>   }
>   
> -static void xattr_copyvalue(struct xattr_iter *_it,
> +static void xattr_copyvalue(struct erofs_xattr_iter *it,
>   			    unsigned int processed,
>   			    char *buf, unsigned int len)
>   {
> -	struct getxattr_iter *it = container_of(_it, struct getxattr_iter, it);
> -
>   	memcpy(it->buffer + processed, buf, len);
>   }
>   
> @@ -361,41 +355,40 @@ static const struct xattr_iter_handlers find_xattr_handlers = {
>   	.value = xattr_copyvalue
>   };
>   
> -static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
> +static int inline_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
>   {
>   	int ret;
>   	unsigned int remaining;
>   
> -	ret = inline_xattr_iter_begin(&it->it, inode);
> +	ret = inline_xattr_iter_begin(it, inode);
>   	if (ret < 0)
>   		return ret;
>   
>   	remaining = ret;
>   	while (remaining) {
> -		ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
> +		ret = xattr_foreach(it, &find_xattr_handlers, &remaining);
>   		if (ret != -ENOATTR)
>   			break;
>   	}
>   	return ret ? ret : it->buffer_size;
>   }
>   
> -static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
> +static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
>   {
>   	struct erofs_inode *const vi = EROFS_I(inode);
> -	struct super_block *const sb = it->it.sb;
> +	struct super_block *const sb = it->sb;
>   	unsigned int i, xsid;
>   	int ret = -ENOATTR;
>   
>   	for (i = 0; i < vi->xattr_shared_count; ++i) {
>   		xsid = vi->xattr_shared_xattrs[i];
> -		it->it.blkaddr = erofs_xattr_blkaddr(sb, xsid);
> -		it->it.ofs = erofs_xattr_blkoff(sb, xsid);
> -		it->it.kaddr = erofs_read_metabuf(&it->it.buf, sb,
> -						  it->it.blkaddr, EROFS_KMAP);
> -		if (IS_ERR(it->it.kaddr))
> -			return PTR_ERR(it->it.kaddr);
> -
> -		ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
> +		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
> +		it->ofs = erofs_xattr_blkoff(sb, xsid);
> +		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);

could we use erofs_bread() here?

> +		if (IS_ERR(it->kaddr))
> +			return PTR_ERR(it->kaddr);
> +
> +		ret = xattr_foreach(it, &find_xattr_handlers, NULL);
>   		if (ret != -ENOATTR)
>   			break;
>   	}
> @@ -417,7 +410,7 @@ int erofs_getxattr(struct inode *inode, int index,
>   		   void *buffer, size_t buffer_size)
>   {
>   	int ret;
> -	struct getxattr_iter it;
> +	struct erofs_xattr_iter it;
>   
>   	if (!name)
>   		return -EINVAL;
> @@ -427,22 +420,21 @@ int erofs_getxattr(struct inode *inode, int index,
>   		return ret;
>   
>   	it.index = index;
> -	it.name.len = strlen(name);
> +	it.name = (struct qstr)QSTR_INIT(name, strlen(name));
>   	if (it.name.len > EROFS_NAME_LEN)
>   		return -ERANGE;
>   
> -	it.it.sb = inode->i_sb;
> -	it.it.buf = __EROFS_BUF_INITIALIZER;
> -	erofs_init_metabuf(&it.it.buf, it.it.sb);
> -	it.name.name = name;
> -
> +	it.sb = inode->i_sb;
> +	it.buf = __EROFS_BUF_INITIALIZER;
> +	erofs_init_metabuf(&it.buf, it.sb);
>   	it.buffer = buffer;
>   	it.buffer_size = buffer_size;
> +	it.buffer_ofs = 0;
>   
>   	ret = inline_getxattr(inode, &it);
>   	if (ret == -ENOATTR)
>   		ret = shared_getxattr(inode, &it);
> -	erofs_put_metabuf(&it.it.buf);
> +	erofs_put_metabuf(&it.buf);
>   	return ret;
>   }
>   
> @@ -488,25 +480,15 @@ const struct xattr_handler *erofs_xattr_handlers[] = {
>   	NULL,
>   };
>   
> -struct listxattr_iter {
> -	struct xattr_iter it;
> -
> -	struct dentry *dentry;
> -	char *buffer;
> -	int buffer_size, buffer_ofs;
> -};
> -
> -static int xattr_entrylist(struct xattr_iter *_it,
> +static int xattr_entrylist(struct erofs_xattr_iter *it,
>   			   struct erofs_xattr_entry *entry)
>   {
> -	struct listxattr_iter *it =
> -		container_of(_it, struct listxattr_iter, it);
>   	unsigned int base_index = entry->e_name_index;
>   	unsigned int prefix_len, infix_len = 0;
>   	const char *prefix, *infix = NULL;
>   
>   	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX) {
> -		struct erofs_sb_info *sbi = EROFS_SB(_it->sb);
> +		struct erofs_sb_info *sbi = EROFS_SB(it->sb);
>   		struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
>   			(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
>   
> @@ -538,23 +520,17 @@ static int xattr_entrylist(struct xattr_iter *_it,
>   	return 0;
>   }
>   
> -static int xattr_namelist(struct xattr_iter *_it,
> +static int xattr_namelist(struct erofs_xattr_iter *it,
>   			  unsigned int processed, char *buf, unsigned int len)
>   {
> -	struct listxattr_iter *it =
> -		container_of(_it, struct listxattr_iter, it);
> -
>   	memcpy(it->buffer + it->buffer_ofs, buf, len);
>   	it->buffer_ofs += len;
>   	return 0;
>   }
>   
> -static int xattr_skipvalue(struct xattr_iter *_it,
> +static int xattr_skipvalue(struct erofs_xattr_iter *it,
>   			   unsigned int value_sz)
>   {
> -	struct listxattr_iter *it =
> -		container_of(_it, struct listxattr_iter, it);
> -
>   	it->buffer[it->buffer_ofs++] = '\0';
>   	return 1;
>   }
> @@ -566,42 +542,41 @@ static const struct xattr_iter_handlers list_xattr_handlers = {
>   	.value = NULL
>   };
>   
> -static int inline_listxattr(struct listxattr_iter *it)
> +static int inline_listxattr(struct erofs_xattr_iter *it)
>   {
>   	int ret;
>   	unsigned int remaining;
>   
> -	ret = inline_xattr_iter_begin(&it->it, d_inode(it->dentry));
> +	ret = inline_xattr_iter_begin(it, d_inode(it->dentry));
>   	if (ret < 0)
>   		return ret;
>   
>   	remaining = ret;
>   	while (remaining) {
> -		ret = xattr_foreach(&it->it, &list_xattr_handlers, &remaining);
> +		ret = xattr_foreach(it, &list_xattr_handlers, &remaining);
>   		if (ret)
>   			break;
>   	}
>   	return ret ? ret : it->buffer_ofs;
>   }
>   
> -static int shared_listxattr(struct listxattr_iter *it)
> +static int shared_listxattr(struct erofs_xattr_iter *it)
>   {
>   	struct inode *const inode = d_inode(it->dentry);
>   	struct erofs_inode *const vi = EROFS_I(inode);
> -	struct super_block *const sb = it->it.sb;
> +	struct super_block *const sb = it->sb;
>   	unsigned int i, xsid;
>   	int ret = 0;
>   
>   	for (i = 0; i < vi->xattr_shared_count; ++i) {
>   		xsid = vi->xattr_shared_xattrs[i];
> -		it->it.blkaddr = erofs_xattr_blkaddr(sb, xsid);
> -		it->it.ofs = erofs_xattr_blkoff(sb, xsid);
> -		it->it.kaddr = erofs_read_metabuf(&it->it.buf, sb,
> -						  it->it.blkaddr, EROFS_KMAP);
> -		if (IS_ERR(it->it.kaddr))
> -			return PTR_ERR(it->it.kaddr);
> -
> -		ret = xattr_foreach(&it->it, &list_xattr_handlers, NULL);
> +		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
> +		it->ofs = erofs_xattr_blkoff(sb, xsid);
> +		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);

Same here.

Thanks,
Gao Xiang

Re: [PATCH v4 4/5] erofs: unify inline/share xattr iterators for listxattr/getxattr

[Gao Xiang]

On 2023/5/31 11:13, Jingbo Xu wrote:
> Make inline_getxattr() and inline_listxattr() unified as
> iter_inline_xattr(), shared_getxattr() and shared_listxattr() unified as
> iter_shared_xattr().
> 
> After the unification, both iter_inline_xattr() and iter_shared_xattr()
> return 0 on success, and negative error on failure.
> 
> One thing worth noting is that, the logic of returning it->buffer_ofs
> when there's no shared xattrs in shared_listxattr() is moved to
> erofs_listxattr() to make the unification possible.  The only difference
> is that, semantically the old behavior will return ENOATTR rather than
> it->buffer_ofs if ENOATTR encountered when listxattr is parsing upon a
> specific shared xattr, while now the new behavior will return
> it->buffer_ofs in this case.  This is not an issue, as listxattr upon a
> specific xattr won't return ENOATTR.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>   fs/erofs/xattr.c | 210 ++++++++++++++++++-----------------------------
>   1 file changed, 78 insertions(+), 132 deletions(-)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 7c76b4d31920..074743e2b271 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -7,19 +7,6 @@
>   #include <linux/security.h>
>   #include "xattr.h"
>   
> -static inline erofs_blk_t erofs_xattr_blkaddr(struct super_block *sb,
> -					      unsigned int xattr_id)
> -{
> -	return EROFS_SB(sb)->xattr_blkaddr +
> -	       erofs_blknr(sb, xattr_id * sizeof(__u32));
> -}
> -
> -static inline unsigned int erofs_xattr_blkoff(struct super_block *sb,
> -					      unsigned int xattr_id)
> -{
> -	return erofs_blkoff(sb, xattr_id * sizeof(__u32));
> -}
> -
>   struct erofs_xattr_iter {
>   	struct super_block *sb;
>   	struct erofs_buf buf;
> @@ -170,30 +157,6 @@ struct xattr_iter_handlers {
>   		      unsigned int len);
>   };
>   
> -static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
> -				   struct inode *inode)
> -{
> -	struct erofs_inode *const vi = EROFS_I(inode);
> -	unsigned int xattr_header_sz, inline_xattr_ofs;
> -
> -	xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
> -			  sizeof(u32) * vi->xattr_shared_count;
> -	if (xattr_header_sz >= vi->xattr_isize) {
> -		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
> -		return -ENOATTR;
> -	}
> -
> -	inline_xattr_ofs = vi->inode_isize + xattr_header_sz;
> -
> -	it->blkaddr = erofs_blknr(it->sb, erofs_iloc(inode) + inline_xattr_ofs);
> -	it->ofs = erofs_blkoff(it->sb, erofs_iloc(inode) + inline_xattr_ofs);
> -	it->kaddr = erofs_read_metabuf(&it->buf, inode->i_sb, it->blkaddr,
> -				       EROFS_KMAP);
> -	if (IS_ERR(it->kaddr))
> -		return PTR_ERR(it->kaddr);
> -	return vi->xattr_isize - xattr_header_sz;
> -}
> -
>   /*
>    * Regardless of success or failure, `xattr_foreach' will end up with
>    * `ofs' pointing to the next xattr item rather than an arbitrary position.
> @@ -355,46 +318,6 @@ static const struct xattr_iter_handlers find_xattr_handlers = {
>   	.value = xattr_copyvalue
>   };
>   
> -static int inline_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
> -{
> -	int ret;
> -	unsigned int remaining;
> -
> -	ret = inline_xattr_iter_begin(it, inode);
> -	if (ret < 0)
> -		return ret;
> -
> -	remaining = ret;
> -	while (remaining) {
> -		ret = xattr_foreach(it, &find_xattr_handlers, &remaining);
> -		if (ret != -ENOATTR)
> -			break;
> -	}
> -	return ret ? ret : it->buffer_ofs;
> -}
> -
> -static int shared_getxattr(struct inode *inode, struct erofs_xattr_iter *it)
> -{
> -	struct erofs_inode *const vi = EROFS_I(inode);
> -	struct super_block *const sb = it->sb;
> -	unsigned int i, xsid;
> -	int ret = -ENOATTR;
> -
> -	for (i = 0; i < vi->xattr_shared_count; ++i) {
> -		xsid = vi->xattr_shared_xattrs[i];
> -		it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
> -		it->ofs = erofs_xattr_blkoff(sb, xsid);
> -		it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr, EROFS_KMAP);
> -		if (IS_ERR(it->kaddr))
> -			return PTR_ERR(it->kaddr);
> -
> -		ret = xattr_foreach(it, &find_xattr_handlers, NULL);
> -		if (ret != -ENOATTR)
> -			break;
> -	}
> -	return ret ? ret : it->buffer_ofs;
> -}
> -
>   static bool erofs_xattr_user_list(struct dentry *dentry)
>   {
>   	return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
> @@ -405,39 +328,6 @@ static bool erofs_xattr_trusted_list(struct dentry *dentry)
>   	return capable(CAP_SYS_ADMIN);
>   }
>   
> -int erofs_getxattr(struct inode *inode, int index,
> -		   const char *name,
> -		   void *buffer, size_t buffer_size)
> -{
> -	int ret;
> -	struct erofs_xattr_iter it;
> -
> -	if (!name)
> -		return -EINVAL;
> -
> -	ret = erofs_init_inode_xattrs(inode);
> -	if (ret)
> -		return ret;
> -
> -	it.index = index;
> -	it.name = (struct qstr)QSTR_INIT(name, strlen(name));
> -	if (it.name.len > EROFS_NAME_LEN)
> -		return -ERANGE;
> -
> -	it.sb = inode->i_sb;
> -	it.buf = __EROFS_BUF_INITIALIZER;
> -	erofs_init_metabuf(&it.buf, it.sb);
> -	it.buffer = buffer;
> -	it.buffer_size = buffer_size;
> -	it.buffer_ofs = 0;
> -
> -	ret = inline_getxattr(inode, &it);
> -	if (ret == -ENOATTR)
> -		ret = shared_getxattr(inode, &it);
> -	erofs_put_metabuf(&it.buf);
> -	return ret;
> -}
> -
>   static int erofs_xattr_generic_get(const struct xattr_handler *handler,
>   				   struct dentry *unused, struct inode *inode,
>   				   const char *name, void *buffer, size_t size)
> @@ -542,45 +432,98 @@ static const struct xattr_iter_handlers list_xattr_handlers = {
>   	.value = NULL
>   };
>   
> -static int inline_listxattr(struct erofs_xattr_iter *it)
> +static int erofs_iter_inline_xattr(struct erofs_xattr_iter *it,
> +				   struct inode *inode, bool getxattr)
>   {
> +	struct erofs_inode *const vi = EROFS_I(inode);
> +	const struct xattr_iter_handlers *op;
> +	unsigned int xattr_header_sz, remaining;
> +	erofs_off_t pos;
>   	int ret;
> -	unsigned int remaining;
>   
> -	ret = inline_xattr_iter_begin(it, d_inode(it->dentry));

In the past, "ret" here is "an int", and
	vi->xattr_isize - xattr_header_sz < 0 will return
negative value (although I think that value is problematic).

see below.


> -	if (ret < 0)
> -		return ret;
> +	xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
> +			  sizeof(u32) * vi->xattr_shared_count;
> +	if (xattr_header_sz >= vi->xattr_isize) {
> +		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
> +		return -ENOATTR;
> +	}
> +
> +	pos = erofs_iloc(inode) + vi->inode_isize + xattr_header_sz;
> +	it->blkaddr = erofs_blknr(it->sb, pos);
> +	it->ofs = erofs_blkoff(it->sb, pos);
> +	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr, EROFS_KMAP);
> +	if (IS_ERR(it->kaddr))
> +		return PTR_ERR(it->kaddr);
> +
> +	remaining = vi->xattr_isize - xattr_header_sz;

so in this patch, `vi->xattr_isize - xattr_header_sz` here could be
negative that should be avoided as well.

Thanks,
Gao Xiang

Re: [PATCH v4 5/5] erofs: use separate xattr parsers for listxattr/getxattr

[Gao Xiang]

On 2023/5/31 11:13, Jingbo Xu wrote:
> There's a callback styled xattr parser, i.e. xattr_foreach(), which is
> shared among listxattr and getxattr.  Convert it to two separate xattr
> parsers for listxattr and getxattr.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>   fs/erofs/xattr.c | 372 ++++++++++++++++++++---------------------------
>   1 file changed, 155 insertions(+), 217 deletions(-)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 074743e2b271..438fcf22b230 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -12,7 +12,7 @@ struct erofs_xattr_iter {
>   	struct erofs_buf buf;
>   	void *kaddr;
>   	erofs_blk_t blkaddr;
> -	unsigned int ofs;
> +	unsigned int ofs, next_ofs;
>   
>   	char *buffer;
>   	int buffer_size, buffer_ofs;
> @@ -141,183 +141,6 @@ static int erofs_init_inode_xattrs(struct inode *inode)
>   	return ret;
>   }
>   
> -/*
> - * the general idea for these return values is
> - * if    0 is returned, go on processing the current xattr;
> - *       1 (> 0) is returned, skip this round to process the next xattr;
> - *    -err (< 0) is returned, an error (maybe ENOXATTR) occurred
> - *                            and need to be handled
> - */
> -struct xattr_iter_handlers {
> -	int (*entry)(struct erofs_xattr_iter *it, struct erofs_xattr_entry *entry);
> -	int (*name)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
> -		    unsigned int len);
> -	int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int value_sz);
> -	void (*value)(struct erofs_xattr_iter *it, unsigned int processed, char *buf,
> -		      unsigned int len);
> -};
> -
> -/*
> - * Regardless of success or failure, `xattr_foreach' will end up with
> - * `ofs' pointing to the next xattr item rather than an arbitrary position.
> - */
> -static int xattr_foreach(struct erofs_xattr_iter *it,
> -			 const struct xattr_iter_handlers *op,
> -			 unsigned int *tlimit)
> -{
> -	struct erofs_xattr_entry entry;
> -	unsigned int value_sz, processed, slice;
> -	int err;
> -
> -	/* 0. fixup blkaddr, ofs, ipage */
> -	err = erofs_xattr_iter_fixup(it, false);
> -	if (err)
> -		return err;
> -
> -	/*
> -	 * 1. read xattr entry to the memory,
> -	 *    since we do EROFS_XATTR_ALIGN
> -	 *    therefore entry should be in the page
> -	 */
> -	entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
> -	if (tlimit) {
> -		unsigned int entry_sz = erofs_xattr_entry_size(&entry);
> -
> -		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
> -		if (*tlimit < entry_sz) {
> -			DBG_BUGON(1);
> -			return -EFSCORRUPTED;
> -		}
> -		*tlimit -= entry_sz;
> -	}
> -
> -	it->ofs += sizeof(struct erofs_xattr_entry);
> -	value_sz = le16_to_cpu(entry.e_value_size);
> -
> -	/* handle entry */
> -	err = op->entry(it, &entry);
> -	if (err) {
> -		it->ofs += entry.e_name_len + value_sz;
> -		goto out;
> -	}
> -
> -	/* 2. handle xattr name (ofs will finally be at the end of name) */
> -	processed = 0;
> -
> -	while (processed < entry.e_name_len) {
> -		err = erofs_xattr_iter_fixup(it, true);
> -		if (err)
> -			goto out;
> -
> -		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
> -			      entry.e_name_len - processed);
> -
> -		/* handle name */
> -		err = op->name(it, processed, it->kaddr + it->ofs, slice);
> -		if (err) {
> -			it->ofs += entry.e_name_len - processed + value_sz;
> -			goto out;
> -		}
> -
> -		it->ofs += slice;
> -		processed += slice;
> -	}
> -
> -	/* 3. handle xattr value */
> -	processed = 0;
> -
> -	if (op->alloc_buffer) {
> -		err = op->alloc_buffer(it, value_sz);
> -		if (err) {
> -			it->ofs += value_sz;
> -			goto out;
> -		}
> -	}
> -
> -	while (processed < value_sz) {
> -		err = erofs_xattr_iter_fixup(it, true);
> -		if (err)
> -			goto out;
> -
> -		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
> -			      value_sz - processed);
> -		op->value(it, processed, it->kaddr + it->ofs, slice);
> -		it->ofs += slice;
> -		processed += slice;
> -	}
> -
> -out:
> -	/* xattrs should be 4-byte aligned (on-disk constraint) */
> -	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
> -	return err < 0 ? err : 0;
> -}
> -
> -static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
> -				       struct erofs_xattr_entry *entry)
> -{
> -	struct erofs_sb_info *sbi = EROFS_SB(it->sb);
> -	struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
> -		(entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
> -
> -	if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
> -		return -ENOATTR;
> -
> -	if (it->index != pf->prefix->base_index ||
> -	    it->name.len != entry->e_name_len + pf->infix_len)
> -		return -ENOATTR;
> -
> -	if (memcmp(it->name.name, pf->prefix->infix, pf->infix_len))
> -		return -ENOATTR;
> -
> -	it->infix_len = pf->infix_len;
> -	return 0;
> -}
> -
> -static int xattr_entrymatch(struct erofs_xattr_iter *it,
> -			    struct erofs_xattr_entry *entry)
> -{
> -	/* should also match the infix for long name prefixes */
> -	if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
> -		return erofs_xattr_long_entrymatch(it, entry);
> -
> -	if (it->index != entry->e_name_index ||
> -	    it->name.len != entry->e_name_len)
> -		return -ENOATTR;
> -	it->infix_len = 0;
> -	return 0;
> -}
> -
> -static int xattr_namematch(struct erofs_xattr_iter *it,
> -			   unsigned int processed, char *buf, unsigned int len)
> -{
> -	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
> -		return -ENOATTR;
> -	return 0;
> -}
> -
> -static int xattr_checkbuffer(struct erofs_xattr_iter *it,
> -			     unsigned int value_sz)
> -{
> -	int err = it->buffer_size < value_sz ? -ERANGE : 0;
> -
> -	it->buffer_ofs = value_sz;
> -	return !it->buffer ? 1 : err;
> -}
> -
> -static void xattr_copyvalue(struct erofs_xattr_iter *it,
> -			    unsigned int processed,
> -			    char *buf, unsigned int len)
> -{
> -	memcpy(it->buffer + processed, buf, len);
> -}
> -
> -static const struct xattr_iter_handlers find_xattr_handlers = {
> -	.entry = xattr_entrymatch,
> -	.name = xattr_namematch,
> -	.alloc_buffer = xattr_checkbuffer,
> -	.value = xattr_copyvalue
> -};
> -
>   static bool erofs_xattr_user_list(struct dentry *dentry)
>   {
>   	return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
> @@ -370,20 +193,70 @@ const struct xattr_handler *erofs_xattr_handlers[] = {
>   	NULL,
>   };
>   
> -static int xattr_entrylist(struct erofs_xattr_iter *it,
> -			   struct erofs_xattr_entry *entry)
> +typedef int (*erofs_xattr_body_handler)(struct erofs_xattr_iter *it,
> +		unsigned int processed, char *buf, unsigned int len);
> +
> +static int erofs_xattr_namematch(struct erofs_xattr_iter *it,
> +		unsigned int processed, char *buf, unsigned int len)
> +{
> +	if (memcmp(buf, it->name.name + it->infix_len + processed, len))
> +		return -ENOATTR;
> +	return 0;
> +}
> +
> +static int erofs_xattr_copy(struct erofs_xattr_iter *it,
> +		unsigned int unused, char *buf, unsigned int len)
> +{
> +	memcpy(it->buffer + it->buffer_ofs, buf, len);
> +	it->buffer_ofs += len;
> +	return 0;
> +}
> +
> +static int erofs_xattr_body(struct erofs_xattr_iter *it, unsigned int len,
> +			    erofs_xattr_body_handler handler)


could we change erofs_xattr_body_handler into a bool (e.g.
bool copy_or_match)?

> +{
> +	unsigned int slice, processed = 0;
> +
> +	while (processed < len) {
> +		int err = erofs_xattr_iter_fixup(it, true);
> +		if (err)
> +			return err;
> +
> +		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
> +			      len - processed);
> +		err = handler(it, processed, it->kaddr + it->ofs, slice);

So that an indirect call could be avoided, as:

		if (copy_or_match) {
			memcpy(it->buffer + it->buffer_ofs, buf, len);
			it->buffer_ofs += len;
		} else if (memcmp(buf,
				it->name.name + it->infix_len + processed, len)) {
			return -ENOATTR;
		}

?

Thanks,
Gao Xiang

Re: [PATCH v4 4/5] erofs: unify inline/share xattr iterators for listxattr/getxattr

[Jingbo Xu]

On 5/31/23 2:57 PM, Gao Xiang wrote:
> 
> 
> On 2023/5/31 11:13, Jingbo Xu wrote:
>>   -static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
>> -                   struct inode *inode)
>> -{
>> -    struct erofs_inode *const vi = EROFS_I(inode);
>> -    unsigned int xattr_header_sz, inline_xattr_ofs;
>> -
>> -    xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
>> -              sizeof(u32) * vi->xattr_shared_count;
>> -    if (xattr_header_sz >= vi->xattr_isize) {
>> -        DBG_BUGON(xattr_header_sz > vi->xattr_isize);
>> -        return -ENOATTR;
>> -    }

In the original implementation, here when xattr_header_sz >=
vi->xattr_isize, inline_xattr_iter_begin() will return -ENOATTR rather
than a negative integer (i.e. vi->xattr_isize - xattr_header_sz).


>>   static int erofs_xattr_generic_get(const struct xattr_handler *handler,
>>                      struct dentry *unused, struct inode *inode,
>>                      const char *name, void *buffer, size_t size)
>> @@ -542,45 +432,98 @@ static const struct xattr_iter_handlers
>> list_xattr_handlers = {
>>       .value = NULL
>>   };
>>   -static int inline_listxattr(struct erofs_xattr_iter *it)
>> +static int erofs_iter_inline_xattr(struct erofs_xattr_iter *it,
>> +                   struct inode *inode, bool getxattr)
>>   {
>> +    struct erofs_inode *const vi = EROFS_I(inode);
>> +    const struct xattr_iter_handlers *op;
>> +    unsigned int xattr_header_sz, remaining;
>> +    erofs_off_t pos;
>>       int ret;
>> -    unsigned int remaining;
>>   -    ret = inline_xattr_iter_begin(it, d_inode(it->dentry));
> 
> In the past, "ret" here is "an int", and
>     vi->xattr_isize - xattr_header_sz < 0 will return
> negative value (although I think that value is problematic).
> 
> see below.

See comment above.

> 
> 
>> -    if (ret < 0)
>> -        return ret;
>> +    xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
>> +              sizeof(u32) * vi->xattr_shared_count;
>> +    if (xattr_header_sz >= vi->xattr_isize) {
>> +        DBG_BUGON(xattr_header_sz > vi->xattr_isize);
>> +        return -ENOATTR;
>> +    }

This checking for "xattr_header_sz >= vi->xattr_isize" is also included
in this patch.


-- 
Thanks,
Jingbo

Re: [PATCH v4 5/5] erofs: use separate xattr parsers for listxattr/getxattr

[Jingbo Xu]

On 5/31/23 3:18 PM, Gao Xiang wrote:
> 
> 
> On 2023/5/31 11:13, Jingbo Xu wrote:
>> There's a callback styled xattr parser, i.e. xattr_foreach(), which is
>> shared among listxattr and getxattr.  Convert it to two separate xattr
>> parsers for listxattr and getxattr.
>>
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>>   fs/erofs/xattr.c | 372 ++++++++++++++++++++---------------------------
>>   1 file changed, 155 insertions(+), 217 deletions(-)
>>
>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>> index 074743e2b271..438fcf22b230 100644
>> --- a/fs/erofs/xattr.c
>> +++ b/fs/erofs/xattr.c
>> @@ -12,7 +12,7 @@ struct erofs_xattr_iter {
>>       struct erofs_buf buf;
>>       void *kaddr;
>>       erofs_blk_t blkaddr;
>> -    unsigned int ofs;
>> +    unsigned int ofs, next_ofs;
>>         char *buffer;
>>       int buffer_size, buffer_ofs;
>> @@ -141,183 +141,6 @@ static int erofs_init_inode_xattrs(struct inode
>> *inode)
>>       return ret;
>>   }
>>   -/*
>> - * the general idea for these return values is
>> - * if    0 is returned, go on processing the current xattr;
>> - *       1 (> 0) is returned, skip this round to process the next xattr;
>> - *    -err (< 0) is returned, an error (maybe ENOXATTR) occurred
>> - *                            and need to be handled
>> - */
>> -struct xattr_iter_handlers {
>> -    int (*entry)(struct erofs_xattr_iter *it, struct
>> erofs_xattr_entry *entry);
>> -    int (*name)(struct erofs_xattr_iter *it, unsigned int processed,
>> char *buf,
>> -            unsigned int len);
>> -    int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int
>> value_sz);
>> -    void (*value)(struct erofs_xattr_iter *it, unsigned int
>> processed, char *buf,
>> -              unsigned int len);
>> -};
>> -
>> -/*
>> - * Regardless of success or failure, `xattr_foreach' will end up with
>> - * `ofs' pointing to the next xattr item rather than an arbitrary
>> position.
>> - */
>> -static int xattr_foreach(struct erofs_xattr_iter *it,
>> -             const struct xattr_iter_handlers *op,
>> -             unsigned int *tlimit)
>> -{
>> -    struct erofs_xattr_entry entry;
>> -    unsigned int value_sz, processed, slice;
>> -    int err;
>> -
>> -    /* 0. fixup blkaddr, ofs, ipage */
>> -    err = erofs_xattr_iter_fixup(it, false);
>> -    if (err)
>> -        return err;
>> -
>> -    /*
>> -     * 1. read xattr entry to the memory,
>> -     *    since we do EROFS_XATTR_ALIGN
>> -     *    therefore entry should be in the page
>> -     */
>> -    entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
>> -    if (tlimit) {
>> -        unsigned int entry_sz = erofs_xattr_entry_size(&entry);
>> -
>> -        /* xattr on-disk corruption: xattr entry beyond xattr_isize */
>> -        if (*tlimit < entry_sz) {
>> -            DBG_BUGON(1);
>> -            return -EFSCORRUPTED;
>> -        }
>> -        *tlimit -= entry_sz;
>> -    }
>> -
>> -    it->ofs += sizeof(struct erofs_xattr_entry);
>> -    value_sz = le16_to_cpu(entry.e_value_size);
>> -
>> -    /* handle entry */
>> -    err = op->entry(it, &entry);
>> -    if (err) {
>> -        it->ofs += entry.e_name_len + value_sz;
>> -        goto out;
>> -    }
>> -
>> -    /* 2. handle xattr name (ofs will finally be at the end of name) */
>> -    processed = 0;
>> -
>> -    while (processed < entry.e_name_len) {
>> -        err = erofs_xattr_iter_fixup(it, true);
>> -        if (err)
>> -            goto out;
>> -
>> -        slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
>> -                  entry.e_name_len - processed);
>> -
>> -        /* handle name */
>> -        err = op->name(it, processed, it->kaddr + it->ofs, slice);
>> -        if (err) {
>> -            it->ofs += entry.e_name_len - processed + value_sz;
>> -            goto out;
>> -        }
>> -
>> -        it->ofs += slice;
>> -        processed += slice;
>> -    }
>> -
>> -    /* 3. handle xattr value */
>> -    processed = 0;
>> -
>> -    if (op->alloc_buffer) {
>> -        err = op->alloc_buffer(it, value_sz);
>> -        if (err) {
>> -            it->ofs += value_sz;
>> -            goto out;
>> -        }
>> -    }
>> -
>> -    while (processed < value_sz) {
>> -        err = erofs_xattr_iter_fixup(it, true);
>> -        if (err)
>> -            goto out;
>> -
>> -        slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
>> -                  value_sz - processed);
>> -        op->value(it, processed, it->kaddr + it->ofs, slice);
>> -        it->ofs += slice;
>> -        processed += slice;
>> -    }
>> -
>> -out:
>> -    /* xattrs should be 4-byte aligned (on-disk constraint) */
>> -    it->ofs = EROFS_XATTR_ALIGN(it->ofs);
>> -    return err < 0 ? err : 0;
>> -}
>> -
>> -static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
>> -                       struct erofs_xattr_entry *entry)
>> -{
>> -    struct erofs_sb_info *sbi = EROFS_SB(it->sb);
>> -    struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
>> -        (entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
>> -
>> -    if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
>> -        return -ENOATTR;
>> -
>> -    if (it->index != pf->prefix->base_index ||
>> -        it->name.len != entry->e_name_len + pf->infix_len)
>> -        return -ENOATTR;
>> -
>> -    if (memcmp(it->name.name, pf->prefix->infix, pf->infix_len))
>> -        return -ENOATTR;
>> -
>> -    it->infix_len = pf->infix_len;
>> -    return 0;
>> -}
>> -
>> -static int xattr_entrymatch(struct erofs_xattr_iter *it,
>> -                struct erofs_xattr_entry *entry)
>> -{
>> -    /* should also match the infix for long name prefixes */
>> -    if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
>> -        return erofs_xattr_long_entrymatch(it, entry);
>> -
>> -    if (it->index != entry->e_name_index ||
>> -        it->name.len != entry->e_name_len)
>> -        return -ENOATTR;
>> -    it->infix_len = 0;
>> -    return 0;
>> -}
>> -
>> -static int xattr_namematch(struct erofs_xattr_iter *it,
>> -               unsigned int processed, char *buf, unsigned int len)
>> -{
>> -    if (memcmp(buf, it->name.name + it->infix_len + processed, len))
>> -        return -ENOATTR;
>> -    return 0;
>> -}
>> -
>> -static int xattr_checkbuffer(struct erofs_xattr_iter *it,
>> -                 unsigned int value_sz)
>> -{
>> -    int err = it->buffer_size < value_sz ? -ERANGE : 0;
>> -
>> -    it->buffer_ofs = value_sz;
>> -    return !it->buffer ? 1 : err;
>> -}
>> -
>> -static void xattr_copyvalue(struct erofs_xattr_iter *it,
>> -                unsigned int processed,
>> -                char *buf, unsigned int len)
>> -{
>> -    memcpy(it->buffer + processed, buf, len);
>> -}
>> -
>> -static const struct xattr_iter_handlers find_xattr_handlers = {
>> -    .entry = xattr_entrymatch,
>> -    .name = xattr_namematch,
>> -    .alloc_buffer = xattr_checkbuffer,
>> -    .value = xattr_copyvalue
>> -};
>> -
>>   static bool erofs_xattr_user_list(struct dentry *dentry)
>>   {
>>       return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
>> @@ -370,20 +193,70 @@ const struct xattr_handler
>> *erofs_xattr_handlers[] = {
>>       NULL,
>>   };
>>   -static int xattr_entrylist(struct erofs_xattr_iter *it,
>> -               struct erofs_xattr_entry *entry)
>> +typedef int (*erofs_xattr_body_handler)(struct erofs_xattr_iter *it,
>> +        unsigned int processed, char *buf, unsigned int len);
>> +
>> +static int erofs_xattr_namematch(struct erofs_xattr_iter *it,
>> +        unsigned int processed, char *buf, unsigned int len)
>> +{
>> +    if (memcmp(buf, it->name.name + it->infix_len + processed, len))
>> +        return -ENOATTR;
>> +    return 0;
>> +}
>> +
>> +static int erofs_xattr_copy(struct erofs_xattr_iter *it,
>> +        unsigned int unused, char *buf, unsigned int len)
>> +{
>> +    memcpy(it->buffer + it->buffer_ofs, buf, len);
>> +    it->buffer_ofs += len;
>> +    return 0;
>> +}
>> +
>> +static int erofs_xattr_body(struct erofs_xattr_iter *it, unsigned int
>> len,
>> +                erofs_xattr_body_handler handler)
> 
> 
> could we change erofs_xattr_body_handler into a bool (e.g.
> bool copy_or_match)?
> 
>> +{
>> +    unsigned int slice, processed = 0;
>> +
>> +    while (processed < len) {
>> +        int err = erofs_xattr_iter_fixup(it, true);
>> +        if (err)
>> +            return err;
>> +
>> +        slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
>> +                  len - processed);
>> +        err = handler(it, processed, it->kaddr + it->ofs, slice);
> 
> So that an indirect call could be avoided, as:
> 
>         if (copy_or_match) {
>             memcpy(it->buffer + it->buffer_ofs, buf, len);
>             it->buffer_ofs += len;
>         } else if (memcmp(buf,
>                 it->name.name + it->infix_len + processed, len)) {
>             return -ENOATTR;
>         }
> 
> ?

Okay. Will be fixed in the next version.


-- 
Thanks,
Jingbo

Re: [PATCH v4 4/5] erofs: unify inline/share xattr iterators for listxattr/getxattr

[Gao Xiang]

On 2023/5/31 16:16, Jingbo Xu wrote:
> 
> 
> On 5/31/23 2:57 PM, Gao Xiang wrote:
>>
>>
>> On 2023/5/31 11:13, Jingbo Xu wrote:
>>>    -static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
>>> -                   struct inode *inode)
>>> -{
>>> -    struct erofs_inode *const vi = EROFS_I(inode);
>>> -    unsigned int xattr_header_sz, inline_xattr_ofs;
>>> -
>>> -    xattr_header_sz = sizeof(struct erofs_xattr_ibody_header) +
>>> -              sizeof(u32) * vi->xattr_shared_count;
>>> -    if (xattr_header_sz >= vi->xattr_isize) {
>>> -        DBG_BUGON(xattr_header_sz > vi->xattr_isize);
>>> -        return -ENOATTR;
>>> -    }
> 
> In the original implementation, here when xattr_header_sz >=
> vi->xattr_isize, inline_xattr_iter_begin() will return -ENOATTR rather
> than a negative integer (i.e. vi->xattr_isize - xattr_header_sz).

Ah, sorry I misreaded that.

Thanks,
Gao Xiang

Re: [PATCH v4 2/5] erofs: unify xattr_iter structures

[Jingbo Xu]

On 5/31/23 2:53 PM, Gao Xiang wrote:
> 
> 
> On 2023/5/31 11:13, Jingbo Xu wrote:
>> Unify xattr_iter/listxattr_iter/getxattr_iter structures into
>> erofs_xattr_iter structure.
>>
>> This is in preparation for the following further cleanup.
>>
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>>   fs/erofs/xattr.c | 155 ++++++++++++++++++++---------------------------
>>   1 file changed, 65 insertions(+), 90 deletions(-)
>>
>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>> index df6c4e6f1f4e..dffca38a46fd 100644
>> --- a/fs/erofs/xattr.c
>> +++ b/fs/erofs/xattr.c
>> @@ -20,16 +20,25 @@ static inline unsigned int
>> erofs_xattr_blkoff(struct super_block *sb,
>>       return erofs_blkoff(sb, xattr_id * sizeof(__u32));
>>   }
>>   -struct xattr_iter {
>> +struct erofs_xattr_iter {
>>       struct super_block *sb;
>>       struct erofs_buf buf;
>>       void *kaddr;
>> -
>>       erofs_blk_t blkaddr;
>>       unsigned int ofs;
>> +
>> +    char *buffer;
>> +    int buffer_size, buffer_ofs;
>> +
>> +    /* getxattr */
>> +    int index, infix_len;
>> +    struct qstr name;
>> +
>> +    /* listxattr */
>> +    struct dentry *dentry;
>>   };
>>   -static inline int erofs_xattr_iter_fixup(struct xattr_iter *it,
>> bool nospan)
>> +static inline int erofs_xattr_iter_fixup(struct erofs_xattr_iter *it,
>> bool nospan)
>>   {
>>       if (it->ofs < it->sb->s_blocksize)
>>           return 0;
>> @@ -50,7 +59,7 @@ static inline int erofs_xattr_iter_fixup(struct
>> xattr_iter *it, bool nospan)
>>   static int erofs_init_inode_xattrs(struct inode *inode)
>>   {
>>       struct erofs_inode *const vi = EROFS_I(inode);
>> -    struct xattr_iter it;
>> +    struct erofs_xattr_iter it;
>>       unsigned int i;
>>       struct erofs_xattr_ibody_header *ih;
>>       struct super_block *sb = inode->i_sb;
>> @@ -153,15 +162,15 @@ static int erofs_init_inode_xattrs(struct inode
>> *inode)
>>    *                            and need to be handled
>>    */
>>   struct xattr_iter_handlers {
>> -    int (*entry)(struct xattr_iter *_it, struct erofs_xattr_entry
>> *entry);
>> -    int (*name)(struct xattr_iter *_it, unsigned int processed, char
>> *buf,
>> +    int (*entry)(struct erofs_xattr_iter *it, struct
>> erofs_xattr_entry *entry);
>> +    int (*name)(struct erofs_xattr_iter *it, unsigned int processed,
>> char *buf,
>>               unsigned int len);
>> -    int (*alloc_buffer)(struct xattr_iter *_it, unsigned int value_sz);
>> -    void (*value)(struct xattr_iter *_it, unsigned int processed,
>> char *buf,
>> +    int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int
>> value_sz);
>> +    void (*value)(struct erofs_xattr_iter *it, unsigned int
>> processed, char *buf,
>>                 unsigned int len);
>>   };
>>   -static int inline_xattr_iter_begin(struct xattr_iter *it,
>> +static int inline_xattr_iter_begin(struct erofs_xattr_iter *it,
>>                      struct inode *inode)
>>   {
>>       struct erofs_inode *const vi = EROFS_I(inode);
>> @@ -189,7 +198,7 @@ static int inline_xattr_iter_begin(struct
>> xattr_iter *it,
>>    * Regardless of success or failure, `xattr_foreach' will end up with
>>    * `ofs' pointing to the next xattr item rather than an arbitrary
>> position.
>>    */
>> -static int xattr_foreach(struct xattr_iter *it,
>> +static int xattr_foreach(struct erofs_xattr_iter *it,
>>                const struct xattr_iter_handlers *op,
>>                unsigned int *tlimit)
>>   {
>> @@ -280,18 +289,10 @@ static int xattr_foreach(struct xattr_iter *it,
>>       return err < 0 ? err : 0;
>>   }
>>   -struct getxattr_iter {
>> -    struct xattr_iter it;
>> -
>> -    char *buffer;
>> -    int buffer_size, index, infix_len;
>> -    struct qstr name;
>> -};
>> -
>> -static int erofs_xattr_long_entrymatch(struct getxattr_iter *it,
>> +static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
>>                          struct erofs_xattr_entry *entry)
>>   {
>> -    struct erofs_sb_info *sbi = EROFS_SB(it->it.sb);
>> +    struct erofs_sb_info *sbi = EROFS_SB(it->sb);
>>       struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
>>           (entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
>>   @@ -309,11 +310,9 @@ static int erofs_xattr_long_entrymatch(struct
>> getxattr_iter *it,
>>       return 0;
>>   }
>>   -static int xattr_entrymatch(struct xattr_iter *_it,
>> +static int xattr_entrymatch(struct erofs_xattr_iter *it,
>>                   struct erofs_xattr_entry *entry)
>>   {
>> -    struct getxattr_iter *it = container_of(_it, struct
>> getxattr_iter, it);
>> -
>>       /* should also match the infix for long name prefixes */
>>       if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
>>           return erofs_xattr_long_entrymatch(it, entry);
>> @@ -325,32 +324,27 @@ static int xattr_entrymatch(struct xattr_iter *_it,
>>       return 0;
>>   }
>>   -static int xattr_namematch(struct xattr_iter *_it,
>> +static int xattr_namematch(struct erofs_xattr_iter *it,
>>                  unsigned int processed, char *buf, unsigned int len)
>>   {
>> -    struct getxattr_iter *it = container_of(_it, struct
>> getxattr_iter, it);
>> -
>>       if (memcmp(buf, it->name.name + it->infix_len + processed, len))
>>           return -ENOATTR;
>>       return 0;
>>   }
>>   -static int xattr_checkbuffer(struct xattr_iter *_it,
>> +static int xattr_checkbuffer(struct erofs_xattr_iter *it,
>>                    unsigned int value_sz)
>>   {
>> -    struct getxattr_iter *it = container_of(_it, struct
>> getxattr_iter, it);
>>       int err = it->buffer_size < value_sz ? -ERANGE : 0;
>>         it->buffer_size = value_sz;
>>       return !it->buffer ? 1 : err;
>>   }
>>   -static void xattr_copyvalue(struct xattr_iter *_it,
>> +static void xattr_copyvalue(struct erofs_xattr_iter *it,
>>                   unsigned int processed,
>>                   char *buf, unsigned int len)
>>   {
>> -    struct getxattr_iter *it = container_of(_it, struct
>> getxattr_iter, it);
>> -
>>       memcpy(it->buffer + processed, buf, len);
>>   }
>>   @@ -361,41 +355,40 @@ static const struct xattr_iter_handlers
>> find_xattr_handlers = {
>>       .value = xattr_copyvalue
>>   };
>>   -static int inline_getxattr(struct inode *inode, struct
>> getxattr_iter *it)
>> +static int inline_getxattr(struct inode *inode, struct
>> erofs_xattr_iter *it)
>>   {
>>       int ret;
>>       unsigned int remaining;
>>   -    ret = inline_xattr_iter_begin(&it->it, inode);
>> +    ret = inline_xattr_iter_begin(it, inode);
>>       if (ret < 0)
>>           return ret;
>>         remaining = ret;
>>       while (remaining) {
>> -        ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
>> +        ret = xattr_foreach(it, &find_xattr_handlers, &remaining);
>>           if (ret != -ENOATTR)
>>               break;
>>       }
>>       return ret ? ret : it->buffer_size;
>>   }
>>   -static int shared_getxattr(struct inode *inode, struct
>> getxattr_iter *it)
>> +static int shared_getxattr(struct inode *inode, struct
>> erofs_xattr_iter *it)
>>   {
>>       struct erofs_inode *const vi = EROFS_I(inode);
>> -    struct super_block *const sb = it->it.sb;
>> +    struct super_block *const sb = it->sb;
>>       unsigned int i, xsid;
>>       int ret = -ENOATTR;
>>         for (i = 0; i < vi->xattr_shared_count; ++i) {
>>           xsid = vi->xattr_shared_xattrs[i];
>> -        it->it.blkaddr = erofs_xattr_blkaddr(sb, xsid);
>> -        it->it.ofs = erofs_xattr_blkoff(sb, xsid);
>> -        it->it.kaddr = erofs_read_metabuf(&it->it.buf, sb,
>> -                          it->it.blkaddr, EROFS_KMAP);
>> -        if (IS_ERR(it->it.kaddr))
>> -            return PTR_ERR(it->it.kaddr);
>> -
>> -        ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
>> +        it->blkaddr = erofs_xattr_blkaddr(sb, xsid);
>> +        it->ofs = erofs_xattr_blkoff(sb, xsid);
>> +        it->kaddr = erofs_read_metabuf(&it->buf, sb, it->blkaddr,
>> EROFS_KMAP);
> 
> could we use erofs_bread() here?

Okay I will make all the conversions to erofs_bread() a separate patch.



-- 
Thanks,
Jingbo