Skip to content

Commit

Permalink
Properly handle updates of variably-sized SA entries.
Browse files Browse the repository at this point in the history 
During the update process in sa_modify_attrs(), the sizes of existing
variably-sized SA entries are obtained from sa_lengths[]. The case where
a variably-sized SA was being replaced neglected to increment the index
into sa_lengths[], so subsequent variable-length SAs would be rewritten
with the wrong length. This patch adds the missing increment operation
so all variably-sized SA entries are stored with their correct lengths.

Previously, a size-changing update of a variably-sized SA that occurred
when there were other variably-sized SAs in the bonus buffer would cause
the subsequent SAs to be corrupted.  The most common case in which this
would occur is when a mode change caused the ZPL_DACL_ACES entry to
change size when a ZPL_DXATTR (SA xattr) entry already existed.

The following sequence would have caused a failure when xattr=sa was in
force and would corrupt the bonus buffer:

	open(filename, O_WRONLY | O_CREAT, 0600);
	...
	lsetxattr(filename, ...);	/* create xattr SA */
	chmod(filename, 0650);		/* enlarges the ACL */
  • Loading branch information
@dweeezil
dweeezil committed Dec 19, 2013
1 parent c2d439d commit 6eed38e
Showing 1 changed file with 9 additions and 6 deletions.
15 changes: 9 additions & 6 deletions module/zfs/sa.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1730,25 +1730,28 @@ sa_modify_attrs(sa_handle_t *hdl, sa_attr_type_t newattr,
hdr = SA_GET_HDR(hdl, SA_BONUS);
idx_tab = SA_IDX_TAB_GET(hdl, SA_BONUS);
for (; k != 2; k++) {
/* iterate over each attribute in layout */
/* Iterate over each attribute in layout. Fetch the
* size of variable-length attributes needing rewrite
* from sa_lengths[]. */
for (i = 0, length_idx = 0; i != count; i++) {
sa_attr_type_t attr;

attr = idx_tab->sa_layout->lot_attrs[i];
length = SA_REGISTERED_LEN(sa, attr);
if (attr == newattr) {
if (length == 0)
++length_idx;
if (action == SA_REMOVE) {
j++;
continue;
}
ASSERT(SA_REGISTERED_LEN(sa, attr) == 0);
ASSERT(length == 0);
ASSERT(action == SA_REPLACE);
SA_ADD_BULK_ATTR(attr_desc, j, attr,
locator, datastart, buflen);
} else {
length = SA_REGISTERED_LEN(sa, attr);
if (length == 0) {
if (length == 0)
length = hdr->sa_lengths[length_idx++];
}

SA_ADD_BULK_ATTR(attr_desc, j, attr,
NULL, (void *)
Expand All @@ -1770,7 +1773,7 @@ sa_modify_attrs(sa_handle_t *hdl, sa_attr_type_t newattr,
length = buflen;
}
SA_ADD_BULK_ATTR(attr_desc, j, newattr, locator,
datastart, buflen);
datastart, length);
}

error = sa_build_layouts(hdl, attr_desc, attr_count, tx);
Expand Down

2 comments on commit 6eed38e

@chrisrd
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, after much brow furrowing in getting an understanding of the wider context, this code looks good to me.

The only thing I don't understand, and it's not related to these changes, is why does it ASSERT(length == 0) (which I understand is the equivalent of the original ASSERT) in the action == SA_REPLACE case? I.e. why aren't we expecting to be replacing any of the fixed length attrs? (I'm hoping the answer doesn't tell me I haven't understood anything at all!)

...oh, is it because the only place this is called with SA_REPLACE is from sa_attr_op(), and that already has a special case for updating equal length attrs (as fixed length attrs must be)? I.e.:

      case SA_UPDATE:
            /* existing rewrite of attr */
            if (bulk[i].sa_addr &&
                  bulk[i].sa_size == bulk[i].sa_length) {       ### equal (including fixed) length attrs
                     SA_COPY_DATA(bulk[i].sa_data_func,
                        bulk[i].sa_data, bulk[i].sa_addr,
                        bulk[i].sa_length);
                     continue;
            } else if (bulk[i].sa_addr) { /* attr size change */
                     error = sa_modify_attrs(hdl, bulk[i].sa_attr,
                        SA_REPLACE, bulk[i].sa_data_func,
                        bulk[i].sa_data, bulk[i].sa_length, tx);

If that's the case, then the ASSERT in sa_modify_attrs() seems little "spooky action at a distance" - it's not confirming something required for the correct operation of the function, but simply a (very) obscure way of saying "there's a better way of dealing with fixed length attrs", and it doesn't even catch the case of variable length attrs with the same length . Perhaps replace it with something like:

   #
   # There's a much faster way of replacing 
   # same-length attrs (which naturally includes 
   # all fixed-length attrs) than this function,
   # use that instead. See sa_attr_op().
   #
   ASSERT(length == 0 && length != buflen);

Either way:

Reviewed by: Chris Dunlop chris@onthe.net.au

@chrisrd
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth... I've run this patch, including the expanded ASSERT (after changing from perl to C comment markers - doh!), on the filesystems where I was previously seeing the problem per openzfs#1978, and saw no corruptions (and, obviously, no ASSERTs).

Please sign in to comment.