| Message ID | 1456163525-1098-1-git-send-email-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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; > -- 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
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 -- 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 --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;