Update sa_lengths properly when replacing variable-size SA.

[dweeezil]

When updating a SA, the sa_lengths array is used to track the sizes
of all variable-sized SAs. It had not been properly updated when
copying an already-existing variable-sized SA. This didn't come up too
often because variable-sized SAs are relatively rare.

Fixes #1978.

[dweeezil]

I'm going to rework the comment on this commit. I would also like to include a small reproducer.

[nedbass]

Nice catch @dweeezil . I think all you really need to do though is increment length_idx, since the length will be stored in sa_build_layouts.

Also, I'm not sure I agree with removing the ASSERT. In the absence of comments, I'm guessing it is there to document/enforce that only variable-length SAs should be updated with sa_modify_attrs(), whereas SA_COPY_DATA() should be used to update fixed-length SAs. e.g. see sa_attr_op().

[dweeezil]

Oops, I didn't mean to remove the ASSERT. I'll rework it a bit when I redo my comments. Guessing this is an issue for other OpenZFS implementations.

[nedbass]

@chrisrd I noticed that too, I'm guessing whoever wrote that intended to pass length instead of buflen to SA_ADD_BULK_ATTR(). I'd have to walk the code, but I'm guessing it works out the same in practice.

[nedbass]

I agree the gap wording is a bit awkward. sa_modify_attrs() doesn't store anything in sa_lengths[], that array is only used in this context to fetch the lengths of existing var-sized SAs that need to be rewritten. Since you asked for suggestions, here's how I would word the comment and commit message:

diff --git a/module/zfs/sa.c b/module/zfs/sa.c
index 9dc6756..41e3fd4 100644
--- a/module/zfs/sa.c
+++ b/module/zfs/sa.c
@@ -1730,7 +1730,9 @@ 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;

@@ -1744,6 +1746,7 @@ sa_modify_attrs(sa_handle_t *hdl, sa_attr_type_t newattr,
                                ASSERT(action == SA_REPLACE);
                                SA_ADD_BULK_ATTR(attr_desc, j, attr,
                                    locator, datastart, buflen);
+                               ++length_idx;
                        } else {
                                length = SA_REGISTERED_LEN(sa, attr);
                                if (length == 0) {

Properly handle updates of variably-sized SA entries.

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 */

Fixes #1978

[nedbass]

Technically speaking it looks like the SA_REMOVE case is still buggy. In practice I think that is dead code, since SA_REMOVE seems to be unused. But in the interest of clarity and preventing future bugs, perhaps the fix should be along these lines:

diff --git a/module/zfs/sa.c b/module/zfs/sa.c
index 9dc6756..8607d90 100644
--- a/module/zfs/sa.c
+++ b/module/zfs/sa.c
@@ -1730,22 +1730,26 @@ 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) {
                                        length = hdr->sa_lengths[length_idx++];
                                }

[dweeezil]

@nedbass Looks good to me. Re-pushed with one other minor formatting change and your updated comment that makes it clear that sa_lengths is used as a reference rather than written to in sa_modify_attrs.

[behlendorf]

@dweeezil This looks good to me as well, I've queued it up for the automated testing.

[behlendorf]

No problems in the testing, so unless someone has a last minute objection I'm going to merge this.

[dweeezil]

@behlendorf Sounds good to me.

[behlendorf]

Merged as:

5d862cb Properly handle updates of variably-sized SA entries.