During a ufs filesystem restore we found that extended attributes weren't correctly installed on # uname -aKU FreeBSD hathor 11.0-STABLE FreeBSD 11.0-STABLE #0 r311660M: Sun Jan 8 21:30:51 AEDT 2017 root@hathor:/110002/D/K8/hqdev-amd64-smp-vga amd64 1100509 1100509 /sbin/restore is our primary backup strategy as neither of cp, tar nor rsync store extended attributes. # Simplest test - for a successful test of extended attributes sh truncate -s 64m /tmp/testf M=`mdconfig -a -t vnode -f /tmp/testf` newfs /dev/$M mount /dev/$M /mnt/A cd /mnt/A echo hello>file-ext setextattr system hi hello file-ext setextattr user sz 6 file-ext lsextattr system file-ext && getextattr system hi file-ext dump -0Lar -C 32 -r -f /tmp/d.dmp /mnt/A rm -v file-ext cd /mnt/A && restore -rvf /tmp/d.dmp lsextattr system file-ext; getextattr system hi file-ext; getextattr user sz file-ext exit umount /mnt/A && mdconfig -du 0 # Test for failure. sh truncate -s 64m /tmp/testf M=`mdconfig -a -t vnode -f /tmp/testf` newfs /dev/$M mount /dev/$M /mnt/A cd /mnt/A echo hello>file-ext setextattr system m "`md5 -q file-ext`" file-ext setextattr user sz `stat -f %z file-ext` file-ext lsextattr system file-ext && getextattr system m file-ext dump -0Lar -C 32 -r -f /tmp/d.dmp /mnt/A # Added the following cd umount /mnt/A && mdconfig -du 0 && rm /tmp/testf # Could replace the from here to next comment with "rm -v file-ext" truncate -s 64m /tmp/testf M=`mdconfig -a -t vnode -f /tmp/testf` newfs /dev/$M mount /dev/$M /mnt/A cd /mnt/A && restore -rvf /tmp/d.dmp # lsextattr system file-ext; getextattr system m file-ext; getextattr user sz file-ext cd exit umount /mnt/A; mdconfig -du 0 Results: During restore - note that the name and value appear to be merged in the system table. Set attributes for ./file-ext: system, (40 bytes), mb1946ac92492d2347c6235b4d2611184 user, (24 bytes), sz If we change setextattr system m "`md5 -q file-ext`" file-ext to setextattr system m "`md5 -q file-ext|head -c 6`" file-ext We get a different result during restore Set attributes for ./file-ext: system, (16 bytes), mb1946a (unable to set) user, (24 bytes), sz Something is very confused. Happy to test as we tried quite a few combinations of restoring on same/different vnode devices with more complex ext attributes.
It appears that dump just dumps raw extattr data from UFS. The routine in restore responsible for parsing that raw data and turning it into syscalls on the receiving filesystem is set_extattr_fd() in tape.c. The intent of the "system, (16 bytes), mb1946a (unable to set)" print is to print the namespace, extattr data length, and name of the extattr. The problem appears to be that restore is using "%*s" rather than "%.*s" to print the non-null-terminated eap->ea_name. The former is the field width, while the latter is the precision. Only precision controls the number of characters printed by a %s format. The same naive behavior around non-null-terminated ea_name leads to the same bogus string being passed into extattr_set_fd() (or extattr_set_link, etc for the other variants) and used to check for ACL attributes.
Hm. The headers do seem to claim that ea_name is nul-terminated, but it doesn't appear to be the case in your output. Perhaps UFS is producing corrupt extattr data or perhaps the comment is wrong. There are other places that assume the string is nul-terminated, though.
The kernel code (e.g. ffs_findextattr) indicates that EA names are not, in fact, nul-terminated. So the headers restore uses are wrong, as is the restore code. This piece of kernel code hasn't changed substantially since the early 2000s. The misleading header comment and macros that reference ea_name as if it is a nul-terminated string were introduced in 2007 in r167010. It immediately precedes r167011, which introduced extattr support to restore. ea_name will *happen* to be nul-terminated when ea_namelength % 8 == 1, like in your "m" example. This is because `sizeof(uint32_t) + 3 * sizeof(uint8_t) + strlen(ea_name)` is padded to multiples of 8 bytes, and the padding is always zeroed. Well, there's a long time between ~2002 and 2007. Maybe Kirk just forgot that ea_name wasn't nul-terminated, or didn't try any keys with length == 1 (mod 8). I propose the following actions: * Fix the ufs/extattr.h documentation for ea_name, and remove EXTATTR_SET_LENGTHS macro (unused) entirely (it uses strlen(ea_name)) * Fix accompanying documentation in extattr.9, fs.5. * Merge set_extattr_file(), set_extattr_link(), and set_extattr_fd() in restore and fix uses of ea_name that assume nul-termination. * Finally, perform the simplification that was proposed in the r167010 commit log. I'll draw up some patches.
https://reviews.freebsd.org/D9206 fixes the documentation.
(In reply to Conrad Meyer from comment #3) Thank-you very much for nurturing this bug, and hopefully return to the butterfly state. Looking forward to confirming patch function. Yes we did test fairly extensively the storage and recovery of extattrs before moving into production as part of integrity checking. Years pass, and a restore is called for - argh!
(In reply to dewayne from comment #5) Fortunately, your dumps are just fine. An updated restore(8) will fix the behavior. I've proposed a fix[0], which I've tested using a workflow similar to your repro. Thanks! [0]: https://reviews.freebsd.org/D9208
Finally, implement the FFS simplifications originally proposed in 2007: https://reviews.freebsd.org/D9225
A commit references this bug: Author: cem Date: Wed Jan 18 17:55:49 UTC 2017 New revision: 312391 URL: https://svnweb.freebsd.org/changeset/base/312391 Log: ufs/extattr.h: Fix documentation of ea_name termination The ea_name string is not nul-terminated. Correct the documentation. Because the subsequent field is padded to 8 bytes, and the padding is zeroed, the ea_name string will appear to be nul-terminated whenever the length isn't exactly one (mod eight). This was introduced in r167010 (2007). Additionally, mark the length fields as unsigned. This particularly matters for the single byte ea_namelength field, which can represent extended attribute names up to 255 bytes long. No functional change. PR: 216127 Reported by: dewayne at heuristicsystems.com.au Reviewed by: kib@ Sponsored by: Dell EMC Isilon Differential Revision: https://reviews.freebsd.org/D9206 Changes: head/share/man/man5/fs.5 head/sys/ufs/ufs/extattr.h
A commit references this bug: Author: cem Date: Wed Jan 18 18:16:58 UTC 2017 New revision: 312393 URL: https://svnweb.freebsd.org/changeset/base/312393 Log: restore(8): Handle extended attribute names correctly UFS2 extended attribute names are not NUL-terminated. Handle appropriately. Correct the EXTATTR_BASE_LENGTH() macro, which handled ea_namelength == one (mod eight) extended attributes incorrectly. PR: 216127 Reported by: dewayne at heuristicsystems.com.au Reviewed by: kib@ Sponsored by: Dell EMC Isilon Differential Revision: https://reviews.freebsd.org/D9208 Changes: head/sbin/restore/dirs.c head/sbin/restore/extern.h head/sbin/restore/tape.c head/sys/sys/extattr.h head/sys/ufs/ufs/extattr.h
A commit references this bug: Author: cem Date: Thu Jan 19 16:46:05 UTC 2017 New revision: 312416 URL: https://svnweb.freebsd.org/changeset/base/312416 Log: ffs_vnops: Simplify extattr access As suggested in r167010, use the structure type and macros to access and modify UFS2 extended attributes. Add assertions that pointers are aligned in places where we now access the data through a structure pointer, instead of character-by-character. PR: 216127 Reported by: dewayne at heuristicsystems.com.au Reviewed by: kib@ Sponsored by: Dell EMC Isilon Differential Revision: https://reviews.freebsd.org/D9225 Changes: head/sys/ufs/ffs/ffs_vnops.c
(In reply to commit-hook from comment #9) Thank-you for promptly addressing this issue. I've rebuild/installed FreeBSD 11.0S world & kernel with patches D9206 D9208 and the restore operates as expected. :) Though I did perform additional tests. Firstly I restored the original dump file, and the restore operation successfully restored the ext attributes :). I then (fileA has different contents and different extattr in the following): - restored fileA from dump file d.dmp extended attributes m and s # patch test -the filesystem is deleted - fileA (same name different content) with attributes m1 and s1 is stored in dump file d2.dmp -filesystem is deleted -fileA is restored from d.dmp -fileA is restored from d2.dmp (fileA with different contents & different extattr) The extended attributes are those from both dump files. This may be as intended, or it may not? Though, if we restore the user mode, owner and times of a restored, file; I do wonder if only the ext attributes of the latest recovered file should also replace all previous extended attributes. I don't have a use case that assists, as my needs are met by overwriting the values of the stored keys. However the testing did reveal something that probably should be explicit (in the doc?).
(In reply to dewayne from comment #11) > The extended attributes are those from both dump files. This may be as intended, or it may not? I don't know. It seems like they are just accumulating naively in that case. Ordinarily I think restores are expected to happen to a pristine filesystem, or level >0 dumps, from the same source, onto a previous 0-level restore. Conflicting restores are sort of a weird case. > Though, if we restore the user mode, owner and times of a restored, file; I do wonder if only the ext attributes of the latest recovered file should also replace all previous extended attributes. That does seem like a reasonable behavior to me. > I don't have a use case that assists, as my needs are met by overwriting the values of the stored keys. However the testing did reveal something that probably should be explicit (in the doc?). Probably! I am probably not the right person to make that change, though, as I'm pretty unfamiliar with restore(8).
The extended attribute system calls either add or delete attributes. To get the behavior of replacing the extended attributes rather than augmenting them would require explicitly removing the old attributes and then adding the new ones. An easier approach is to unlink the old file (which clears out the old extended attributes) and then create the new file of the same name and add the new extended attributes to it. This effect can be obtained by including the -u flag when running restore. So perhaps what should be done is to simply add text to the restore man page suggesting the the -u flag be used when restoring files with extended attributes to avoid having them accumulate? Or maybe just make the -u flag default to on.
(In reply to Kirk McKusick from comment #13) Restore is used in the context of a full, and then incremental restores. So it makes sense to only restore the attributes of the file being restored and yes the -u option should be used. I think the most elegant approach is to draw the attention of users to the -u option with an additional sentence along the lines of: "This flag is recommended when using extended attributes, to avoid improperly accumulating attributes upon (pre)existing files." Changing the behaviour of restore in the default case, probably isn't warranted. Someone may a script that "requires" restore to complain for some reason!? Thank-you for your consideration.
A commit references this bug: Author: mckusick Date: Sun Jan 22 17:49:14 UTC 2017 New revision: 312638 URL: https://svnweb.freebsd.org/changeset/base/312638 Log: By default, when doing incremental restores the restore program overwrites an existing file rather than removing it and creating a new file. If the old and new version of the file both have extended attributes and the extended attributes of the two versions of the file are different, the result is that the new file ends up with the union of the extended attributes of the old and new files. To get the behavior of replacing the extended attributes rather than augmenting them requires explicitly removing the old attributes and then adding the new ones. To get this behavior, the old file must be unlinked (which clears out the old extended attributes). Then the new file of the same name must be created and the new extended attributes added to it. This behavior can be obtained by specifying the -u flag when running restore. Rather than defaulting the -u option to on and possibly breaking existing scripts using restore, this change simply notes in the restore.8 manual page that the -u flag is recommended when using restore on filesystems that contain extended attributes. PR: 216127 Reported by: dewayne at heuristicsystems.com.au Differential Revision: https://reviews.freebsd.org/D9208 Changes: head/sbin/restore/restore.8