diff mbox

[v2] Btrfs: fix listxattrs not listing all xattrs packed in the same item

Message ID 1456329239-30802-1-git-send-email-fdmanana@kernel.org (mailing list archive)
State Accepted
Headers show

Commit Message

Filipe Manana Feb. 24, 2016, 3:53 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

In the listxattrs handler, we were not listing all the xattrs that are
packed in the same btree item, which happens when multiple xattrs have
a name that when crc32c hashed produce the same checksum value.

Fix this by processing them all.

The following test case for xfstests reproduces the issue:

  seq=`basename $0`
  seqres=$RESULT_DIR/$seq
  echo "QA output created by $seq"
  tmp=/tmp/$$
  status=1	# failure is the default!
  trap "_cleanup; exit \$status" 0 1 2 3 15

  _cleanup()
  {
      cd /
      rm -f $tmp.*
  }

  # get standard environment, filters and checks
  . ./common/rc
  . ./common/filter
  . ./common/attr

  # real QA test starts here
  _supported_fs generic
  _supported_os Linux
  _require_scratch
  _require_attrs

  rm -f $seqres.full

  _scratch_mkfs >>$seqres.full 2>&1
  _scratch_mount

  # Create our test file with a few xattrs. The first 3 xattrs have a name
  # that when given as input to a crc32c function result in the same checksum.
  # This made btrfs list only one of the xattrs through listxattrs system call
  # (because it packs xattrs with the same name checksum into the same btree
  # item).
  touch $SCRATCH_MNT/testfile
  $SETFATTR_PROG -n user.foobar -v 123 $SCRATCH_MNT/testfile
  $SETFATTR_PROG -n user.WvG1c1Td -v qwerty $SCRATCH_MNT/testfile
  $SETFATTR_PROG -n user.J3__T_Km3dVsW_ -v hello $SCRATCH_MNT/testfile
  $SETFATTR_PROG -n user.something -v pizza $SCRATCH_MNT/testfile
  $SETFATTR_PROG -n user.ping -v pong $SCRATCH_MNT/testfile

  # Now call getfattr with --dump, which calls the listxattrs system call.
  # It should list all the xattrs we have set before.
  $GETFATTR_PROG --absolute-names --dump $SCRATCH_MNT/testfile | _filter_scratch

  status=0
  exit

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Fixed logic to jump into next item, which was detected by warnings
    from gcc 5.3.0 (reported by Holger).

 fs/btrfs/xattr.c | 65 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Comments

Holger Hoffstätte Feb. 24, 2016, 4:11 p.m. UTC | #1 
On 02/24/16 16:53, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In the listxattrs handler, we were not listing all the xattrs that are
> packed in the same btree item, which happens when multiple xattrs have
> a name that when crc32c hashed produce the same checksum value.
> 
> Fix this by processing them all.
(snip)
> V2: Fixed logic to jump into next item, which was detected by warnings
>     from gcc 5.3.0 (reported by Holger).

Great! No more warnings and also a lot less confusing. :)

thanks!
Holger

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index f2a20d5..145d2b8 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -260,16 +260,12 @@  out:
 
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	struct btrfs_key key, found_key;
+	struct btrfs_key key;
 	struct inode *inode = d_inode(dentry);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_path *path;
-	struct extent_buffer *leaf;
-	struct btrfs_dir_item *di;
-	int ret = 0, slot;
+	int ret = 0;
 	size_t total_size = 0, size_left = size;
-	unsigned long name_ptr;
-	size_t name_len;
 
 	/*
 	 * ok we want all objects associated with this id.
@@ -291,6 +287,13 @@  ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		goto err;
 
 	while (1) {
+		struct extent_buffer *leaf;
+		int slot;
+		struct btrfs_dir_item *di;
+		struct btrfs_key found_key;
+		u32 item_size;
+		u32 cur;
+
 		leaf = path->nodes[0];
 		slot = path->slots[0];
 
@@ -316,31 +319,45 @@  ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		if (found_key.type > BTRFS_XATTR_ITEM_KEY)
 			break;
 		if (found_key.type < BTRFS_XATTR_ITEM_KEY)
-			goto next;
+			goto next_item;
 
 		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
-		if (verify_dir_item(root, leaf, di))
-			goto next;
-
-		name_len = btrfs_dir_name_len(leaf, di);
-		total_size += name_len + 1;
+		item_size = btrfs_item_size_nr(leaf, slot);
+		cur = 0;
+		while (cur < item_size) {
+			u16 name_len = btrfs_dir_name_len(leaf, di);
+			u16 data_len = btrfs_dir_data_len(leaf, di);
+			u32 this_len = sizeof(*di) + name_len + data_len;
+			unsigned long name_ptr = (unsigned long)(di + 1);
+
+			if (verify_dir_item(root, leaf, di)) {
+				ret = -EIO;
+				goto err;
+			}
 
-		/* we are just looking for how big our buffer needs to be */
-		if (!size)
-			goto next;
+			total_size += name_len + 1;
+			/*
+			 * We are just looking for how big our buffer needs to
+			 * be.
+			 */
+			if (!size)
+				goto next;
 
-		if (!buffer || (name_len + 1) > size_left) {
-			ret = -ERANGE;
-			goto err;
-		}
+			if (!buffer || (name_len + 1) > size_left) {
+				ret = -ERANGE;
+				goto err;
+			}
 
-		name_ptr = (unsigned long)(di + 1);
-		read_extent_buffer(leaf, buffer, name_ptr, name_len);
-		buffer[name_len] = '\0';
+			read_extent_buffer(leaf, buffer, name_ptr, name_len);
+			buffer[name_len] = '\0';
 
-		size_left -= name_len + 1;
-		buffer += name_len + 1;
+			size_left -= name_len + 1;
+			buffer += name_len + 1;
 next:
+			cur += this_len;
+			di = (struct btrfs_dir_item *)((char *)di + this_len);
+		}
+next_item:
 		path->slots[0]++;
 	}
 	ret = total_size;