sa_find_sizes() may compute wrong SA header size

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 #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 6a7c0ccca44ad02c476a111d8f7911fc8b12fff7 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 #1890

diff --git a/module/zfs/sa.c b/module/zfs/sa.c
index 117d3868a251986841fccb7b67ee5fe470d281ac..9dc6756dce8d8c600aec99bec298eb890250b74b 100644 (file)
--- a/module/zfs/sa.c
+++ b/module/zfs/sa.c
@@ -571,10 +571,9 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
 {
        int var_size = 0;
        int i;
-       int j = -1;
        int full_space;
        int hdrsize;
-       boolean_t done = B_FALSE;
+       int extra_hdrsize;
 
        if (buftype == SA_BONUS && sa->sa_force_spill) {
                *total = 0;
@@ -585,10 +584,9 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
 
        *index = -1;
        *total = 0;
+       *will_spill = B_FALSE;
 
-       if (buftype == SA_BONUS)
-               *will_spill = B_FALSE;
-
+       extra_hdrsize = 0;
        hdrsize = (SA_BONUSTYPE_FROM_DB(db) == DMU_OT_ZNODE) ? 0 :
            sizeof (sa_hdr_phys_t);
 
@@ -600,8 +598,8 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
 
                *total = P2ROUNDUP(*total, 8);
                *total += attr_desc[i].sa_length;
-               if (done)
-                       goto next;
+               if (*will_spill)
+                       continue;
 
                is_var_sz = (SA_REGISTERED_LEN(sa, attr_desc[i].sa_attr) == 0);
                if (is_var_sz) {
@@ -609,21 +607,27 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
                }
 
                if (is_var_sz && var_size > 1) {
-                       if (P2ROUNDUP(hdrsize + sizeof (uint16_t), 8) +
+                       /* Don't worry that the spill block might overflow.
+                        * It will be resized if needed in sa_build_layouts().
+                        */
+                       if (buftype == SA_SPILL ||
+                           P2ROUNDUP(hdrsize + sizeof (uint16_t), 8) +
                            *total < full_space) {
                                /*
                                 * Account for header space used by array of
                                 * optional sizes of variable-length attributes.
-                                * Record the index in case this increase needs
-                                * to be reversed due to spill-over.
+                                * Record the extra header size in case this
+                                * increase needs to be reversed due to
+                                * spill-over.
                                 */
                                hdrsize += sizeof (uint16_t);
-                               j = i;
+                               if (*index != -1)
+                                       extra_hdrsize += sizeof (uint16_t);
                        } else {
-                               done = B_TRUE;
-                               *index = i;
-                               if (buftype == SA_BONUS)
-                                       *will_spill = B_TRUE;
+                               ASSERT(buftype == SA_BONUS);
+                               if (*index == -1)
+                                       *index = i;
+                               *will_spill = B_TRUE;
                                continue;
                        }
                }
@@ -638,22 +642,15 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
                    (*total + P2ROUNDUP(hdrsize, 8)) >
                    (full_space - sizeof (blkptr_t))) {
                        *index = i;
-                       done = B_TRUE;
                }
 
-next:
                if ((*total + P2ROUNDUP(hdrsize, 8)) > full_space &&
                    buftype == SA_BONUS)
                        *will_spill = B_TRUE;
        }
 
-       /*
-        * j holds the index of the last variable-sized attribute for
-        * which hdrsize was increased.  Reverse the increase if that
-        * attribute will be relocated to the spill block.
-        */
-       if (*will_spill && j == *index)
-               hdrsize -= sizeof (uint16_t);
+       if (*will_spill)
+               hdrsize -= extra_hdrsize;
 
        hdrsize = P2ROUNDUP(hdrsize, 8);
        return (hdrsize);