I compiled with debug information by adding the following to local.conf:

PREFERRED_VERSION_pseudo-native = "1.7.4+gitAUTOINC+735ac1b9ff"
SRCREV_pn-pseudo-native = "735ac1b9ff7471b100f6758f9072bfd45e0e3f40"
PREFERRED_VERSION_pseudo = "1.7.4+gitAUTOINC+735ac1b9ff"
SRCREV_pn-pseudo = "735ac1b9ff7471b100f6758f9072bfd45e0e3f40"
BUILD_OPTIMIZATION_pn-e2fsprogs-native = "-g"
INHIBIT_SYSROOT_STRIP = "1"

I'm still walking through a comparison of mkfs.ext4 inside and outside of pseudo. Some observations:
- llistxattr() for "foobar" returns xattrs in a different order: security.ima comes first under pseudo and second outside
- When I override the order when running under pseudo (see below), the resulting image is correct.

So it seems that setting xattrs with ext2fs_xattr_set() is somehow sensitive to the order in which the two xattrs are getting set.

To override llistxattr() under gdb:
b llistxattr
Breakpoint 2, llistxattr (path=0x63b27b "foobar", list=0x0, size=0) at pseudo_wrapfuncs.c:7534
7534    llistxattr(const char *path, char *list, size_t size) {
(gdb) 
Continuing.

Breakpoint 2, llistxattr (path=0x63b27b "foobar", list=0x621350 "\230\326\354\366\377\177", size=30) at pseudo_wrapfuncs.c:7534
7534    llistxattr(const char *path, char *list, size_t size) {
(gdb) fin
Run till exit from #0  llistxattr (path=0x63b27b "foobar", list=0x621350 "\230\326\354\366\377\177", size=30) at pseudo_wrapfuncs.c:7534
0x000000000040e3be in set_inode_xattr (fs=0x62a400, ino=12, filename=0x63b27b "foobar") at ../../git/misc/create_inode.c:160
160             size = llistxattr(filename, list, size);
Value returned is $14 = 30
(gdb) p list
$15 = 0x621350 "security.ima"
(gdb) call strcpy(list, "security.SMACK64")
$16 = 6427472
(gdb) call strcpy(list+strlen(list)+1, "security.ima")
$18 = 6427489
(gdb) p list
$19 = 0x621350 "security.SMACK64"
(gdb) p list+strlen(list)+1
$20 = 0x621361 "security.ima"

(In reply to comment #0)
> Created attachment 2949 [details]
> rootfs with xattrs
> 
> Attached a test case with a single file inside a directory. The file has two
> xattrs (see bug 8954 for a description how the archive was created):
> 
> $ getfattr -d -m . testrootfs/foobar 
> # file: testrootfs/foobar
> security.SMACK64="_"

How can you set security.SMACK64, please ? I only can set user.foo on my host Ubuntu 12.04 (the "user." is a must).

> security.ima=0sAwIC2C1O/
> QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGv
> ncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/
> SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

And also I got "bad input encoding" when use such a value.

It seems that pseudo related, add Peter and Mark in the loop.

Since I can't set the value you provided, so that I can't reproduce the problem.

> 
> Converting this directory tree into an ext4 partition leads to an incomplete
> security.ima attribute, but only *when running the command under pseudo*. It
> works fine when unpacking the archive as root on the build host (Debian
> Stable) and invoking mkfs.ext4 there.
> 
> Steps to reproduce:
> 1. Build pseudo with bug 8954 fixed. For that add to local.conf:
> 
> PREFERRED_VERSION_pseudo-native = "1.7.4+gitAUTOINC+735ac1b9ff"
> SRCREV_pn-pseudo-native = "735ac1b9ff7471b100f6758f9072bfd45e0e3f40"
> PREFERRED_VERSION_pseudo = "1.7.4+gitAUTOINC+735ac1b9ff"
> SRCREV_pn-pseudo = "735ac1b9ff7471b100f6758f9072bfd45e0e3f40"
> 
> bitbake pseudo pseudo-native
> 
> 2. enter pseudo: bitbake -c devshell core-image-minimal
> 
> 3. Under pseudo:
>    tar --xattrs --xattrs-include=* -xf testrootfs.tar
>    getfattr -d -m . testrootfs/foobar
>    dd if=/dev/zero of=rootfs.ext4 bs=512 count=100000
>    mkfs.ext4 -d testrootfs  rootfs.ext4
> 
> 4. On the host:
>    sudo losetup /dev/loop0 rootfs.ext4
>    mkdir mnt
>    sudo mount /dev/loop0 mnt/; getfattr -d -m . mnt/*; sudo  umount mnt
> 
> Expected result: foobar should have the full xattrs as in the tar archive
> (shown above)
> Actual result:
> 
> # file: mnt/foobar
> security.SMACK64="_"
> mnt/foobar: security.ima: No such attribute
> 
> "No such attribute" = ENODATA = ENOATTR in getfattr(), despite security.ima
> being reported by listxattr().

(In reply to comment #2)
> (In reply to comment #0)
> > Created attachment 2949 [details]
> > rootfs with xattrs
> > 
> > Attached a test case with a single file inside a directory. The file has two
> > xattrs (see bug 8954 for a description how the archive was created):
> > 
> > $ getfattr -d -m . testrootfs/foobar 
> > # file: testrootfs/foobar
> > security.SMACK64="_"
> 
> How can you set security.SMACK64, please ? I only can set user.foo on my
> host Ubuntu 12.04 (the "user." is a must).

To do it on the host, you must be root. But beware that the problem did not occur on the host because the ordering of xattrs was different than under pseudo. I could only reproduce it under pseudo.

Also note that you do not have to re-create a test case with setfattr unless you want to - just unpack the attachement with "tar --xattrs --xattrs-include -xf testrootfs.tar".


> > security.ima=0sAwIC2C1O/
> > QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGv
> > ncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/
> > SrFa1+PDlu9Cdr6L3uOzkkVjoEs=
> 
> And also I got "bad input encoding" when use such a value.

Worked fine for me:

$ sudo setfattr -n security.ima -v security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs= foo

$ getfattr -d -m . foo
# file: foo
security.capability=0sAAAAAgEAAAAAAAAAAAAAAAAAAAA=
security.ima="security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs="

> Since I can't set the value you provided, so that I can't reproduce the
> problem.

Please try with the attached test case. However, to unpack it you have to use the modified pseudo, see step 1 in the initial bug description.

Huh, that's interesting. I saw similar behavior, but at the time I didn't think anything of it. But I just tried it now, and it appeared to work:

# tar --xattrs --xattrs-include=* -xf foo.tar
# getfattr -d -m . testrootfs/foobar
# file: testrootfs/foobar
security.SMACK64="_"
security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

But if I run *outside* of pseudo:

root@seebs-worktop:~/pseudo# tar --xattrs --xattrs-include=* -xf foo.tar
root@seebs-worktop:~/pseudo# getfattr -d -m . testrootfs/foobar
# file: testrootfs/foobar
security.SMACK64="_"

And I don't remotely know why that would be the case. But it may be related to why the generated image is showing different attributes than expected?

(In reply to comment #4)
> But if I run *outside* of pseudo:
> 
> root@seebs-worktop:~/pseudo# tar --xattrs --xattrs-include=* -xf foo.tar
> root@seebs-worktop:~/pseudo# getfattr -d -m . testrootfs/foobar
> # file: testrootfs/foobar
> security.SMACK64="_"
> 
> And I don't remotely know why that would be the case.

My guess is that the host's kernel has IMA support and thus interferes with setting security.ima. I believe Fedora has it in its kernels. strace on tar might show more.

> But it may be related
> to why the generated image is showing different attributes than expected?

No, the problem occurs when running inside pseudo, and there setting the xattrs always works when done "normally". It was just unpacking with GNU tar which led to odd results.

Regarding priority of this bug: I thought initially that it would only occur rarely that we need to have two xattrs on one file.

But I just noticed that I have to label the entire /var hierarchy with a Smack label, and then multiple files are affect. So this is fairly critical for us now.

Huh. At that point, I guess the next question is why it seems to be working for me, but not for you. Although I'm still using the old rootfs, let me try with the one from this case... Looks to me like it's working?

# tar --xattrs --xattrs-include=* -xf testrootfs.tar
# getfattr -d -m . testrootfs/*
# file: testrootfs/foobar
security.SMACK64="_"
security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

So it seems to me that the tar unpack in pseudo is working. Maybe this is an ext4fs thing?

It's quite possible that the issue is that, when you mount the ext4fs, it runs into whatever's making the .ima value disappear when you run tar as root instead of under pseudo, which would be maybe some kind of kernel support. The kernel could be choosing not to show you those values, maybe?

(In reply to comment #7)
> So it seems to me that the tar unpack in pseudo is working. Maybe this is an
> ext4fs thing?

Yes, absolutely. Sorry if that hasn't been clear. As I said in comment #2, the order of xattrs as returned by listxattr() is relevant for mkfs.ext4, but it shouldn't be. It works when returned as on the host, but fails when returned as under pseudo.

Fixing bug 8954 was necessary only to make it simpler to reproduce the problem, because otherwise unpacking the test case under pseudo would have failed.

(In reply to comment #8)
> (In reply to comment #7)
> > So it seems to me that the tar unpack in pseudo is working. Maybe this is an
> > ext4fs thing?
> 
> Yes, absolutely. Sorry if that hasn't been clear. As I said in comment #2,
> the order of xattrs as returned by listxattr() is relevant for mkfs.ext4,
> but it shouldn't be. It works when returned as on the host, but fails when
> returned as under pseudo.
> 
> Fixing bug 8954 was necessary only to make it simpler to reproduce the
> problem, because otherwise unpacking the test case under pseudo would have
> failed.

Maybe it is tar version related ? Works for me when use tar-native:

$ bitbake -c devshell core-image-minimal
$ /buildarea/lyang1/test_u/tmp/sysroots/x86_64-linux/usr/bin/tar-native/tar --xattrs --xattrs-include=* -xf testrootfs.tar

# getfattr -d -m . testrootfs/*
# file: testrootfs/foobar
security.SMACK64="_"
security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=
QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

(In reply to comment #9)
> Maybe it is tar version related ? Works for me when use tar-native:
> 
> $ bitbake -c devshell core-image-minimal
> $ /buildarea/lyang1/test_u/tmp/sysroots/x86_64-linux/usr/bin/tar-native/tar
> --xattrs --xattrs-include=* -xf testrootfs.tar
> 
> # getfattr -d -m . testrootfs/*
> # file: testrootfs/foobar
> security.SMACK64="_"
> security.ima=0sAwIC2C1O/
> QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGv
> ncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/
> SrFa1+PDlu9Cdr6L3uOzkkVjoEs=
> QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

You are confusing this bug here with bug #8954. #8954 was about unpacking the test case archive under pseudo. This one here, bug #8992, is about converting the content of that archive after unpacking into an ext4 partition. 

What you show above is that unpacking works. Now please try creating an ext4 partition from it. If it works, check in which order listxattr() returns xattrs. It fails only when returned in a certain order (see comment #1).

Other combinations of xattrs besides the one mentioned in comment #1 have similar problems.

What seems to avoid the problem is sorting the xattrs by the length of their value.

I'm guessing that the length-of-value is not the actual determinant, but it may be that order in which they were written matters, and we ought to be sorting them, maybe?

I don't know whether there's any existing guarantees about the order in which attributes are stored or written. If I write two attributes, am I guaranteed that list will return them in the order they were written?

(In reply to comment #12)
> I'm guessing that the length-of-value is not the actual determinant, but it
> may be that order in which they were written matters, and we ought to be
> sorting them, maybe?

The order should not matter. Conceptually, it is a key/value store with no guarantees about the ordering in listxattr().

> I don't know whether there's any existing guarantees about the order in
> which attributes are stored or written. If I write two attributes, am I
> guaranteed that list will return them in the order they were written?

No; at least I haven't see anything like that, and it also would not make sense.

Created attachment 2969 [details]
sort xattrs by size before adding to image

This is the workaround that I am using now until someone can fix the image creation code so that it works regardless of the order.

I think I understand what might be the problem here. http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBUTES contains the small snippet that " The entry descriptors are sorted by attribute name, so that two extended attribute blocks can be compared efficiently. ".

Looking at the kernel code for reading ext file systems, it will iterate the attributes and stop short if the comparison becomes higher than the searched value (ext4_xattr_find_entry()).

This is consistent with strace showing an ENODATA, yet the index showing the listing. It could also explain why Patrick's 'order by size' happened to work.

[pid 10171] mmap(NULL, 26258, PROT_READ, MAP_SHARED, 3, 0) = 0x7fdb36a8c000
[pid 10171] close(3)                    = 0
[pid 10171] getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=64*1024}) = 0
[pid 10171] lstat("mnt/foobar", {st_mode=S_IFREG|0755, st_size=1, ...}) = 0
[pid 10171] listxattr("mnt/foobar", NULL, 0) = 30
[pid 10171] listxattr("mnt/foobar", "security.SMACK64\0security.ima\0", 256) = 30
[pid 10171] getxattr("mnt/foobar", "security.SMACK64", 0x0, 0) = 1
[pid 10171] getxattr("mnt/foobar", "security.SMACK64", "_", 256) = 1
[pid 10171] fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 13), ...}) = 0
[pid 10171] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fdb36a8b000
[pid 10171] write(1, "# file: mnt/foobar\n", 19# file: mnt/foobar
) = 19
[pid 10171] write(1, "security.SMACK64=\"_\"\n", 21security.SMACK64="_"
) = 21
[pid 10171] getxattr("mnt/foobar", "security.ima", 0x0, 0) = -1 ENODATA (No data available)
[pid 10171] write(2, "mnt/foobar: ", 12mnt/foobar: ) = 12
[pid 10171] write(2, "security.ima: No such attribute\n", 32security.ima: No such attribute
) = 32

I suspect the libext2fs code in e2fsprogs needs to be taught about this minor sorting detail as I can't see any sorting code in it but I'll need to double check with fresh eyes.

(In reply to comment #15)
> I think I understand what might be the problem here.
> http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBUTES
> contains the small snippet that " The entry descriptors are sorted by
> attribute name, so that two extended attribute blocks can be compared
> efficiently. ".
> 
> Looking at the kernel code for reading ext file systems, it will iterate the
> attributes and stop short if the comparison becomes higher than the searched
> value (ext4_xattr_find_entry()).
> 
> This is consistent with strace showing an ENODATA, yet the index showing the
> listing. It could also explain why Patrick's 'order by size' happened to
> work.

But I was sorting on the wrong value and it just happened to hit the right order, right? Fixing that will be easy and also simplify the code, because it can be done between listxattr() and the following getxattr() calls.

> I suspect the libext2fs code in e2fsprogs needs to be taught about this
> minor sorting detail as I can't see any sorting code in it but I'll need to
> double check with fresh eyes.

Doing the sorting where I placed then might be the right location.

Oh.

4c56d54

-       char *sql = "SELECT name FROM xattrs WHERE file_id = ?;";
+       char *sql = "SELECT name FROM xattrs WHERE file_id = ? ORDER BY name;";

Created attachment 2973 [details]
Patch which fixes the issue

(In reply to comment #16)
> (In reply to comment #15)
> > I think I understand what might be the problem here.
> > http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBUTES
> > contains the small snippet that " The entry descriptors are sorted by
> > attribute name, so that two extended attribute blocks can be compared
> > efficiently. ".
> > 
> > Looking at the kernel code for reading ext file systems, it will iterate the
> > attributes and stop short if the comparison becomes higher than the searched
> > value (ext4_xattr_find_entry()).
> > 
> > This is consistent with strace showing an ENODATA, yet the index showing the
> > listing. It could also explain why Patrick's 'order by size' happened to
> > work.
> 
> But I was sorting on the wrong value and it just happened to hit the right
> order, right?

Yes, that was just luck.

> Fixing that will be easy and also simplify the code, because
> it can be done between listxattr() and the following getxattr() calls.
> 
> > I suspect the libext2fs code in e2fsprogs needs to be taught about this
> > minor sorting detail as I can't see any sorting code in it but I'll need to
> > double check with fresh eyes.
> 
> Doing the sorting where I placed then might be the right location.

I suspect that this needs to be fixed in the ext2fs code, so close but not quite. You should be able to add the attributes in any order and it will sort them into the correct order when the change is committed.

The nice thing is there is already in fact a sort function there, it just needs to handle them in the correct order. I've attached a patch. I'm basing the "right" order on the code in the kernel since I can't find the order defined anywhere. That patch works with the cases I've tested it with. Note that its not just a sort on the value of the attribute names but also their length.

(In reply to comment #17)
> Oh.
> 
> 4c56d54
> 
> -       char *sql = "SELECT name FROM xattrs WHERE file_id = ?;";
> +       char *sql = "SELECT name FROM xattrs WHERE file_id = ? ORDER BY
> name;";

Neat, however this is a bug in the ext* filesystem generation code. Makes a nice workaround though :)

Yeah. But it's easy enough to do so I've put it in pseudo-master because why not, it's cheap.

The workaround is now in pseudo 1.7.5 which has been submitted for oe-core, which should make this go away for now.

I can confirm that RP's patch works. See https://github.com/01org/meta-intel-iot-security/pull/100/commits and the TravisCI test results (also passed with older OE-core where pseudo is not changed) https://travis-ci.org/01org/meta-intel-iot-security/builds/108498911

Robert, will you take care of upstreaming this fix, both in e2fsprogs and in OE-core master?

http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=d1496b4cc48d2c4c081052d04e1ede51a7503f25

Discussions are ongoing upstream about this.

Verified: not reproducible on 36b43b233c841b3c09fc17e04686b4b758780963

sudo mount /dev/loop0 mnt/; getfattr -d -m . mnt/*; sudo  umount mnt
# file: mnt/foobar
security.SMACK64="_"
security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=

getfattr -d -m . testrootfs/foobar
# file: testrootfs/foobar
security.SMACK64="_"
security.ima=0sAwIC2C1O/QCAKE3nkQA9MB2DFaZOveb+E6kTTq4FQrmCugFE5vnX4vGJQ3NK7jNJkcJXuxhQnG1rf68Z31mkGvncxKz93RS9RjjZML7X8I0H5j3+QsU3Y8ZXsxhTg+UOIWiZV0XeVgKIvwaTlvrEXcvNjHH/SrFa1+PDlu9Cdr6L3uOzkkVjoEs=