Skip to content

Commit 67629d0

Browse files
nedbassbehlendorf
authored andcommitted
Fix rounding discrepancy in sa_find_sizes()
A rounding discrepancy exists between how sa_build_layouts() and sa_find_sizes() calculate when the spill block needs to be kicked in. This results in a narrow size range where sa_build_layouts() believes there must be a spill block allocated but due to the discrepancy there isn't. A panic then occurs when the hdl->sa_spill NULL pointer is dereferenced. The following reproducer for this bug was isolated: truncate -s 128m /tmp/tank zpool create tank /tmp/tank zfs create -o xattr=sa tank/fish ln -s `perl -e 'print "z" x 41'` /tank/fish/z setfattr -hn trusted.foo -v`perl -e 'print "z"x45'` /tank/fish/z This test results in roughly the following system attribute (SA) layout: 176 bytes - "standard" SA's 41 bytes - name of symbolic link target 100 bytes - XDR encoded nvlist for xattr --- 317 bytes - total Because 317 is less than DN_MAX_BONUSLEN (320), sa_find_sizes() decides no spill block is needed. But sa_build_layouts() rounds 41 up to 48 when computing the space requirements so it tries to switch to the spill block. Note that we were only able to reproduce this bug using a combination of symbolic links and the Linux-specific xattr=sa dataset property. So while this issue is not technically Linux-specific, it may be difficult or impossible to hit the narrow size range needed to reproduce it on other platforms. To fix the discrepancy, round the running total in sa_find_sizes() up to an 8-byte boundary before accounting for each SA, since this is how they will be stored in the bonus and (possibly) spill buffers. To make the intent of the code more clear, explicitly assert key assumptions about expected alignment of data and whether spill-over will occur. Signed-off-by: Matthew Ahrens <mahrens@delphix.com Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #1240
1 parent 89103a2 commit 67629d0

File tree

1 file changed

+5
-0
lines changed

1 file changed

+5
-0
lines changed

module/zfs/sa.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,10 +593,12 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
593593
sizeof (sa_hdr_phys_t);
594594

595595
full_space = (buftype == SA_BONUS) ? DN_MAX_BONUSLEN : db->db_size;
596+
ASSERT(IS_P2ALIGNED(full_space, 8));
596597

597598
for (i = 0; i != attr_count; i++) {
598599
boolean_t is_var_sz;
599600

601+
*total = P2ROUNDUP(*total, 8);
600602
*total += attr_desc[i].sa_length;
601603
if (done)
602604
goto next;
@@ -728,12 +730,15 @@ sa_build_layouts(sa_handle_t *hdl, sa_bulk_attr_t *attr_desc, int attr_count,
728730
for (i = 0, len_idx = 0, hash = -1ULL; i != attr_count; i++) {
729731
uint16_t length;
730732

733+
ASSERT(IS_P2ALIGNED(data_start, 8));
734+
ASSERT(IS_P2ALIGNED(buf_space, 8));
731735
attrs[i] = attr_desc[i].sa_attr;
732736
length = SA_REGISTERED_LEN(sa, attrs[i]);
733737
if (length == 0)
734738
length = attr_desc[i].sa_length;
735739

736740
if (buf_space < length) { /* switch to spill buffer */
741+
VERIFY(spilling);
737742
VERIFY(bonustype == DMU_OT_SA);
738743
if (buftype == SA_BONUS && !sa->sa_force_spill) {
739744
sa_find_layout(hdl->sa_os, hash, attrs_start,

0 commit comments

Comments
 (0)