SA size calculation bug + patch #1890

chrisrd · 2013-11-21T05:29:26Z

Opening a ticket so it's tracked for ZoL...

Possibly a solution to #1648, 6a7c0cc ?

See: http://article.gmane.org/gmane.comp.file-systems.openzfs.devel/245

Date: Wed, 20 Nov 2013 04:15:21 -0800 (PST)
From: James Pan <jiaming.pan@yahoo.com>
To: "developer@open-zfs.org" <developer@open-zfs.org>, "zfs-devel@freebsd.org" <zfs-devel@freebsd.org>
Subject: [OpenZFS Developer] a ZFS SA bug and my patch

Hi,
I hit a ZFS SA problem on FreeBSD 9.2, but I believe the issue exists on other platform too. Here is the description of the bug.

PROBLEM:
run the attached script on a ZFS, after a few seconds, run zdb -vvv on the ZFS, zdb will crash at the following assertion:

Assertion failed: (IS_SA_BONUSTYPE(bonustype) && SA_HDR_SIZE_MATCH_LAYOUT(hdr, tb) || !IS_SA_BONUSTYPE(bonustype) || (IS_SA_BONUSTYPE(bonustype) && hdr->sa_layout_info == 0)), file
/usr/src/cddl/lib/libzpool/../../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sa.c, line 1509.
Abort (core dumped)

the reason is the SA's header size does not match its layout.

ROOT CAUSE:
The issue will be hit when a file has more than 2 variable-length SA and the total SA size is larger than the bonus buffer's length -  sizeof (blkptr_t), but less the bonus buffer's length.

in sa_find_sizes(), done is set to TRUE if the SA size + header > the bonus buffer's length - sizeof (blkptr_t), then hdrsize += sizeof (uint16_t) will be skipped for the second variable-length SA. If finally all SA can
fit in the bonus buffer and no spill block is needed, we will get a wrong hdrsize.

MY FIX:
I've also attached my simple fix for this issue, anyone who might have interest could you please take a look? Thanks a lot!

The text was updated successfully, but these errors were encountered:

behlendorf · 2013-11-21T19:58:33Z

@chrisrd Yes, I saw that discussion and I know @nedbass did as well. We'll want to review those changes and verify they fix the issue, they look like they should. Then we can revert the workaround you linked to.

Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links (Issue openzfs#1648). Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 Commit 6a7c0cc worked around this bug by forcing xattr's on symlinks to be stored in directory format. This change implements a proper fix, so the workaround can now be reverted. The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html Ported-by: Ned Bass <bass6@llnl.gov> Closes openzfs#1890

This reverts commit 6a7c0cc. A proper fix for Issue openzfs#1648 was landed under Issue openzfs#1890, so this is no longer needed. Signed-off-by: Ned Bass <bass6@llnl.gov>

Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links (Issue openzfs#1648). Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 Commit 6a7c0cc worked around this bug by forcing xattr's on symlinks to be stored in directory format. This change implements a proper fix, so the workaround can now be reverted. The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html Ported-by: Ned Bass <bass6@llnl.gov> Closes openzfs#1890

This reverts commit 6a7c0cc. A proper fix for Issue openzfs#1648 was landed under Issue openzfs#1890, so this is no longer needed. Signed-off-by: Ned Bass <bass6@llnl.gov>

This reverts commit 6a7c0cc. A proper fix for Issue #1648 was landed under Issue #1890, so this is no longer needed. Signed-off-by: Ned Bass <bass6@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #1648

Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links (Issue openzfs#1648). Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 Commit 6a7c0cc worked around this bug by forcing xattr's on symlinks to be stored in directory format. This change implements a proper fix, so the workaround can now be reverted. The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html Ported-by: Ned Bass <bass6@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#1890

This reverts commit 6a7c0cc. A proper fix for Issue openzfs#1648 was landed under Issue openzfs#1890, so this is no longer needed. Signed-off-by: Ned Bass <bass6@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#1648

Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links (Issue openzfs#1648). Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 Commit 6a7c0cc worked around this bug by forcing xattr's on symlinks to be stored in directory format. This change implements a proper fix, so the workaround can now be reverted. The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html Ported-by: Ned Bass <bass6@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#1890

Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links. Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html openzfs/zfs#1890 Sifned-off-by: Ned Bass <bass6@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

Reviewed-by: Ned Bass <bass6@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Andriy Gapon <avg@freebsd.org> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Under the right conditions sa_find_sizes() will compute an incorrect size of the system attribute (SA) header. This causes a failed assertion when the SA_HDR_SIZE_MATCH_LAYOUT() test returns false, and may lead to corruption of SA data. The bug presents itself when there are more than two variable-length SAs of just the right size to fit in the bonus buffer of a dnode. The existing logic fails to account for the SA header space needed to store the sizes of all the variable-length SAs. A reproducer was possible on Linux by setting the xattr=sa dataset property and storing xattrs on symbolic links. Note the corrupt link target name: $ zfs set xattr=sa tank/fish $ cd /tank/fish $ ln -fs 12345678901234567 link $ setfattr -n trusted.0000000000000000000 -v 0x000000000000000000000000 -h link $ setfattr -n trusted.1111111111111111111 -v 0x000000000000000000000000 -h link $ ls -l link lrwxrwxrwx 1 root root 17 Dec 6 15:40 link -> 90123456701234567 The reference link below contains a reproducer for FreeBSD. References: http://lists.open-zfs.org/pipermail/developer/2013-November/000306.html openzfs/zfs#1890 Closes #14

This was referenced Dec 5, 2013

SPLError: 864:0:(zfs_vfsops.c:347:zfs_space_delta_cb()) SPL PANIC #1303

Closed

list a zfs directory hang #1930

Closed

dweeezil mentioned this issue Dec 5, 2013

Corrupted dn_spill forcing ASSERTION in zio.c:285 #1912

Closed

behlendorf closed this as completed in 472e7c6 Dec 10, 2013

chrisrd mentioned this issue Dec 16, 2013

xattr=sa: listxattr EFAULT #1978

Closed

behlendorf mentioned this issue Oct 21, 2015

6434 sa_find_sizes() may compute wrong SA header size openzfs/openzfs#14

Closed

This was referenced Dec 23, 2015

[incomplete] October | 10/2015; List of Upstream Illumos changes #3982

Closed

[OpenZFS] Illumos 6434 - sa_find_sizes() may compute wrong SA header size #4141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SA size calculation bug + patch #1890

SA size calculation bug + patch #1890

chrisrd commented Nov 21, 2013

behlendorf commented Nov 21, 2013

SA size calculation bug + patch #1890

SA size calculation bug + patch #1890

Comments

chrisrd commented Nov 21, 2013

behlendorf commented Nov 21, 2013