* [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item
@ 2016-02-22 17:52 fdmanana
2016-02-23 23:29 ` Satoru Takeuchi
2016-02-24 15:53 ` [PATCH v2] " fdmanana
0 siblings, 2 replies; 5+ messages in thread
From: fdmanana @ 2016-02-22 17:52 UTC (permalink / raw)
To: linux-btrfs
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>
---
fs/btrfs/xattr.c | 62 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 39 insertions(+), 23 deletions(-)
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index f2a20d5..caf643d 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];
@@ -319,28 +322,41 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
goto next;
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);
+ }
path->slots[0]++;
}
ret = total_size;
--
2.7.0.rc3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item
2016-02-22 17:52 [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item fdmanana
@ 2016-02-23 23:29 ` Satoru Takeuchi
2016-02-24 11:42 ` Holger Hoffstätte
2016-02-24 15:53 ` [PATCH v2] " fdmanana
1 sibling, 1 reply; 5+ messages in thread
From: Satoru Takeuchi @ 2016-02-23 23:29 UTC (permalink / raw)
To: fdmanana, linux-btrfs
On 2016/02/23 2:52, 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.
>
> 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
Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
* chris/integration-4.6(HEAD is 790dd8b)
=======================================
# ./check generic/337
FSTYP -- btrfs
PLATFORM -- Linux/x86_64 fedora23 4.2.7-300.fc23.x86_64
MKFS_OPTIONS -- /dev/vdc
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/vdc /scratch_mnt
generic/337 - output mismatch (see /root/xfstests/results//generic/337.out.bad)
--- tests/generic/337.out 2016-02-24 07:26:33.000000000 +0900
+++ /root/xfstests/results//generic/337.out.bad 2016-02-24 07:43:02.471000000 +0900
@@ -1,7 +1,5 @@
QA output created by 337
# file: SCRATCH_MNT/testfile
-user.J3__T_Km3dVsW_="hello"
-user.WvG1c1Td="qwerty"
user.foobar="123"
user.ping="pong"
user.something="pizza"
...
(Run 'diff -u tests/generic/337.out /root/xfstests/results//generic/337.out.bad' to see the entire diff)
Ran: generic/337
Failures: generic/337
Failed 1 of 1 tests
================================================================
* the above source + your patch
============================================================
# ./check generic/337
FSTYP -- btrfs
PLATFORM -- Linux/x86_64 fedora23 4.5.0-rc3-ktest+
MKFS_OPTIONS -- /dev/vdc
MOUNT_OPTIONS -- /dev/vdc /scratch_mnt
generic/337 0s
Ran: generic/337
Passed all 1 tests
============================================================
Thanks,
Satoru
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> fs/btrfs/xattr.c | 62 +++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index f2a20d5..caf643d 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];
>
> @@ -319,28 +322,41 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
> goto next;
>
> 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);
> + }
> path->slots[0]++;
> }
> ret = total_size;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item
2016-02-23 23:29 ` Satoru Takeuchi
@ 2016-02-24 11:42 ` Holger Hoffstätte
0 siblings, 0 replies; 5+ messages in thread
From: Holger Hoffstätte @ 2016-02-24 11:42 UTC (permalink / raw)
To: Satoru Takeuchi; +Cc: fdmanana, linux-btrfs
On Wed, Feb 24, 2016 at 12:29 AM, Satoru Takeuchi
<takeuchi_satoru@jp.fujitsu.com> wrote:
> On 2016/02/23 2:52, 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.
(snip)
This gives me (with gcc 5.3.0):
fs/btrfs/xattr.c: In function 'btrfs_listxattr':
fs/btrfs/xattr.c:357:8: warning: 'this_len' may be used uninitialized
in this function [-Wmaybe-uninitialized]
cur += this_len;
^
fs/btrfs/xattr.c:357:8: warning: 'cur' may be used uninitialized in
this function [-Wmaybe-uninitialized]
fs/btrfs/xattr.c:327:9: warning: 'item_size' may be used uninitialized
in this function [-Wmaybe-uninitialized]
while (cur < item_size) {
^
fs/btrfs/xattr.c:358:7: warning: 'di' may be used uninitialized in
this function [-Wmaybe-uninitialized]
di = (struct btrfs_dir_item *)((char *)di + this_len);
^
And at least from a compiler POV these seem to have merit, caused by a possibly
premature "goto next" above the "while (cur < item_size)" loop.
cheers
Holger
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] Btrfs: fix listxattrs not listing all xattrs packed in the same item
2016-02-22 17:52 [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item fdmanana
2016-02-23 23:29 ` Satoru Takeuchi
@ 2016-02-24 15:53 ` fdmanana
2016-02-24 16:11 ` Holger Hoffstätte
1 sibling, 1 reply; 5+ messages in thread
From: fdmanana @ 2016-02-24 15:53 UTC (permalink / raw)
To: linux-btrfs
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(-)
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;
--
2.7.0.rc3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Btrfs: fix listxattrs not listing all xattrs packed in the same item
2016-02-24 15:53 ` [PATCH v2] " fdmanana
@ 2016-02-24 16:11 ` Holger Hoffstätte
0 siblings, 0 replies; 5+ messages in thread
From: Holger Hoffstätte @ 2016-02-24 16:11 UTC (permalink / raw)
To: fdmanana, linux-btrfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-24 16:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-22 17:52 [PATCH] Btrfs: fix listxattrs not listing all xattrs packed in the same item fdmanana
2016-02-23 23:29 ` Satoru Takeuchi
2016-02-24 11:42 ` Holger Hoffstätte
2016-02-24 15:53 ` [PATCH v2] " fdmanana
2016-02-24 16:11 ` Holger Hoffstätte
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.