11996 2017-08-29 08:14:48 +0000 pseudo: cannot use unnamed temporary files (O_TMPFILE) 2018-03-05 02:19:54 +0000 1 1 6 Yocto Project Subprojects Pseudo pseudo unspecified x86 Multiple RESOLVED FIXED Medium+ normal 2.5 M3 1 samuli.piippo alex.kanavin alex.kanavin randy.macleod richard.purdie seebs thiago yp.pseudo.watcher yp.watcher --- --- Don't know oldest_to_newest 76224 0 samuli.piippo 2017-08-29 08:14:48 +0000 Latest qmake is now using unnamed temporary files and fails when running with pseudo. Error occcurs when calling linkat with the final name for the file (-1, No such file or directory) char path[PATH_MAX]; fd = open("/path/to/dir", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR); snprintf(path, PATH_MAX, "/proc/self/fd/%d", fd); linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW); 76226 1 seebs 2017-08-29 13:26:12 +0000 Okay that's horrible. linkat() already has AT_EMPTY_PATH for this, which would have been: linkat(fd, "", AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW | AT_EMPTY_PATH) But instead of using that, they're requesting a hard link to a file descriptor file in another filesystem. And then there's a hack where, since 2.6.18, AT_SYMLINK_FOLLOW on /procs/self/fd/N, that is silently turned into a way to create a link to an unlinked file. This will require some special-casing and we should probably handle both cases. But also I would like to state for the record that both of these are insanely bad interfaces. If you want a way to link a file descriptor, make a way to link a file descriptor. Instead, we get one thing that only sometimes under special circumstances changes the meaning of a parameter entirely, and also a case where you're allowed to specify that you want a hard link to another filesystem, but you specified to follow symbolic links, so that's okay then. Ugh. 76253 2 thiago 2017-08-29 16:38:04 +0000 (In reply to comment #1) > But also I would like to state for the record that both of these are > insanely bad interfaces. If you want a way to link a file descriptor, make a > way to link a file descriptor. Instead, we get one thing that only sometimes > under special circumstances changes the meaning of a parameter entirely, and > also a case where you're allowed to specify that you want a hard link to > another filesystem, but you specified to follow symbolic links, so that's > okay then. Ugh. I totally agree with you. linkat(fd, "", ..., AT_EMPTY_PATH) would have been a lot better, but that doesn't work. The man page for open(2) gives this incantation: O_TMPFILE must be specified with one of O_RDWR or O_WRONLY and, optionally, O_EXCL. If O_EXCL is not spec- ified, then linkat(2) can be used to link the temporary file into the filesystem, making it permanent, using code like the following: char path[PATH_MAX]; fd = open("/path/to/dir", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR); /* File I/O on 'fd'... */ snprintf(path, PATH_MAX, "/proc/self/fd/%d", fd); linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW); 76254 3 seebs 2017-08-29 16:45:45 +0000 Huh, I wonder *why* it doesn't work, since it's documented anyway. Or do you mean "doesn't work with pseudo"? Because neither of them will yet. I'm really swamped right now, I have three or four things to fix when I have a free day or two. 76420 4 thiago 2017-08-31 17:48:40 +0000 (In reply to comment #3) > Huh, I wonder *why* it doesn't work, since it's documented anyway. > > Or do you mean "doesn't work with pseudo"? Because neither of them will yet. > > I'm really swamped right now, I have three or four things to fix when I have > a free day or two. As far as we can tell, it doesn't work under pseudo. We found it while trying to compile Qt 5.10 with it, as part of building the Qt layer. We currently don't see a way to apply a workaround that would catch this reliably and would not affect the functionality for everyone else. At this point, the suggestion is for the Qt layer to patch the sources and disable the feature. 76825 5 alex.kanavin 2017-09-14 11:32:17 +0000 It's not very clear to me: is there now a workaround on the Qt side, or is it something that has to be solved on pseudo side? 76830 6 samuli.piippo 2017-09-14 12:57:13 +0000 There is a workaround applied to Qt in meta-qt5 layer, but there won't be anything in Qt itself. 76853 7 thiago 2017-09-14 16:32:50 +0000 The workaround is for open+O_TMPFILE failing, which happens if you run against an old kernel and happened also with the qemu bug. There's no workaround in the sources for linkat() failing. This is where pseudo is failing. The layer disables the O_TMPFILE feature completely until pseudo is fixed, but anyone not using the layer or somehow tinkering with it the wrong way will be affected. 77067 8 sjolley.yp.pm 2017-09-22 15:08:24 +0000 It appears the questions have been answered. If you still need more information, place it back in NEEDINFO and can you restate/clearify the question? 77265 9 4042 alex.kanavin 2017-10-02 14:39:53 +0000 Created attachment 4042 Alex's naive patch Here's a naive patch I tried to see if it helps, and looks like it doesn't. TBH, I have no real idea what I did here, but if anyone wants to comment, please do. 77865 10 alex.kanavin 2017-10-30 14:03:44 +0000 Assigning to Seebs; at this point I would really need guidance on the patch, how this should work and how to move forward. 78707 11 seebs 2017-12-14 22:27:35 +0000 Hi, sorry for the delay, stuff has pretty much just been Staying On Fire for a long time. I'm travelling this week but should be able to look more closely at this when I get back. I think a correct solution for this really weird use case is going to have to be hard-coding some kind of awareness of that special path structure into linkat(), to have it detect that the path it's been requested to link is a magic path that is actually really a file descriptor, not a path at all, so just create a new link using the information we have for that file descriptor. 78712 12 thiago 2017-12-15 07:18:43 +0000 I don't think it's special with linkat(). The /proc/PID/fd path *is* the actual file descriptor, regardless of whether the file the readlink() returns exists or not. The path it shows is the path that was used to open, but that doesn't need to be visible to you. 78713 13 alex.kanavin 2017-12-15 12:34:30 +0000 (In reply to comment #11) > Hi, sorry for the delay, stuff has pretty much just been Staying On Fire for > a long time. I'm travelling this week but should be able to look more > closely at this when I get back. > > I think a correct solution for this really weird use case is going to have > to be hard-coding some kind of awareness of that special path structure into > linkat(), to have it detect that the path it's been requested to link is a > magic path that is actually really a file descriptor, not a path at all, so > just create a new link using the information we have for that file > descriptor. That's what I attempted to do in the patch I attached, but it doesn't seem to help with building qt5. Like I said, I don't really know what I'm doing in the patch, so someone competent needs to look at it :) 78727 14 seebs 2017-12-18 20:14:40 +0000 huh, that's odd. I had a few minutes, so I built a trivial test case so I could verify my understanding of what's wrong. And... it worked? #include <errno.h> #include <fcntl.h> #include <unistd.h> #include <stdio.h> #include <stdlib.h> int main(void) { char buffer[256]; int fd = open("a", O_RDWR); snprintf(buffer, 256, "/proc/self/fd/%d", fd); int rc = linkat(AT_FDCWD, buffer, AT_FDCWD, "b", AT_SYMLINK_FOLLOW); fprintf(stderr, "rc: %d, errno: %d\n", rc, errno); exit(0); } Given this, if I make a file a, and run this, I end up with a hard link to it, b, with the same stats, and linkat succeeds. Is this happening while running in a chroot environment or something, so that /proc/self/fd wouldn't actually exist? 78728 15 thiago 2017-12-18 21:17:44 +0000 For the kernel, the file actually exists, so linkat() will work. However, the /proc/self/fd/N link may be broken if you readlink() it. That's actually the case for O_TMPFILE: it always shows up as deleted. So if you readlink(), you'll get garbage. You need to pass the actual /proc/self/fd/N name to the kernel for the operation to complete. Try with O_TMPFILE or by inserting unlink("a") before the linkat(): #define _GNU_SOURCE 1 #include <errno.h> #include <fcntl.h> #include <unistd.h> #include <stdio.h> #include <stdlib.h> int main(void) { char buffer[256]; int fd = open(".", O_RDWR | O_TMPFILE, 0666); snprintf(buffer, 256, "/proc/self/fd/%d", fd); int rc = linkat(AT_FDCWD, buffer, AT_FDCWD, "b", AT_SYMLINK_FOLLOW); fprintf(stderr, "rc: %d, errno: %d\n", rc, errno); exit(0); } 78824 16 seebs 2017-12-22 23:16:00 +0000 I got this half-fixed, in that this case works, but it breaks things horribly now if you use O_TMPFILE and *don't* link the file, because then we have a stray entry in the database with no path, and a dev/ino pair that could be reused. So this will take a bit longer. I badly underestimated this one. It looks like I have most of the hooks in place for the thing, though; I need a table of file info parallel to the fd_paths table that indicates files which should be deleted on close, probably. So, open with O_TMPFILE will send an initial CREAT message, then a MAY_UNLINK, and then it can send DID_UNLINK if it needs to. ... Still some loose ends here. I'll try to get this fixed up "soon". 78826 17 thiago 2017-12-23 01:17:43 +0000 Workaround: reject O_TMPFILE and return either EOPNOTSUPP or EISDIR. The caller will fall back to a regular file with a temporary name. 78827 18 seebs 2017-12-23 01:34:32 +0000 Okay, so, I pushed a patch that does make this work, but it's *awful*. I do not recommend it. I admit that the ENOTSUPP answer is tempting. But I think I may be able to do a better job by just abandoning any pretense that this is really "linking" a file in any meaningful sense. But also I'm tired and I'm not gonna try to fix it tonight. But the basic idea would be: Revert the top two patches in master, and redo the O_TMPFILE support patch in a completely different way, where we just send an O_CREAT for the "linked" file when linking. (Also, probably, change the behavior with O_TMPFILE to suppress creating any database entry, and definitely not try to use the directory name as the path of the opened file, since that's clearly wrong.) 79098 19 seebs 2018-01-17 01:53:58 +0000 okay, sorry for the delay, this was horrible and i had to retry it a bit to make a patch i didn't hate. now in master, along with several other fixes, as pseudo 1.9.0. (the 1.9 version bump is for epoll.) 79194 20 alex.kanavin 2018-01-23 15:06:41 +0000 (In reply to comment #19) > okay, sorry for the delay, this was horrible and i had to retry it a bit to > make a patch i didn't hate. now in master, along with several other fixes, > as pseudo 1.9.0. (the 1.9 version bump is for epoll.) Can you upload the 1.9.0 tarball here please? http://downloads.yoctoproject.org/releases/pseudo/ 79196 21 seebs 2018-01-23 20:20:31 +0000 I don't think I actually have access to the upload dir. can someone else grab this and put it in the right place? https://www.seebs.net/tmp/pseudo-1.9.0.tar.bz2 md5 7ec8409bccdbd9c958367469b6059071 sha256 a3f150cc55b2fa009c9411335981b55675ead4e5cb4a9c36ed40b32f81d020c1 this includes two more patches I'd missed, one is restoring higher reliability (fixing a subtle bug introduced by FASTOP), the other is a fix for openat flags because of a silly typo. 79315 22 alex.kanavin 2018-02-02 12:18:17 +0000 Unfortunately, I'm seeing the following error with the latest upstream commit of pseudo: | DEBUG: Executing shell function do_install | got *at() syscall for unknown directory, fd 4 | unknown base path for fd 4, path usr | couldn't allocate absolute path for 'usr'. | chown: cannot access '/home/ak/development/poky/build/tmp/work/all-poky-linux/tzdata/2018c-r0/image/usr': Bad address | got *at() syscall for unknown directory, fd 4 | unknown base path for fd 4, path etc | couldn't allocate absolute path for 'etc'. | chown: cannot access '/home/ak/development/poky/build/tmp/work/all-poky-linux/tzdata/2018c-r0/image/etc': Bad address | WARNING: exit code 1 from a shell command. I've bisected between that and PSEUDO_1_8_2, and established the the commit that introduces the failure is indeed this one: http://git.yoctoproject.org/cgit/cgit.cgi/pseudo/commit/?id=bbebd1e85f4b3f30a355e8721052c568e8408cfc (handle O_TMPFILE and linkat()) Also, latest pseudo locks up altogether when epoll is enabled, but let's address issues in order :) 79443 23 seebs 2018-02-14 21:31:52 +0000 Gah. Thanks for the report, sorry for delay in getting back to you on it. Do you have a reproducer smaller than "build yocto" available? I can try to look, but I don't know how to trigger this, and a complete yocto build is Large. 79448 24 alex.kanavin 2018-02-15 07:44:34 +0000 I don't know how to trigger this outside of yocto either. But you don't need a complete yocto build - this happens as soon as the first target recipe is reaching do_install(). I just checked that on my machine with an empty sstate cache this takes about three minutes (when running 'bitbake os-release' for example, which is an arch-independent recipe without dependencies, and so does not require building the toolchain or libc). 79449 25 alex.kanavin 2018-02-15 09:03:33 +0000 Ah, the branch that you can take is here: http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=akanavin/pseudo-1.9.0 79452 26 alex.kanavin 2018-02-15 15:07:56 +0000 I can also enable additional logging, and we can try to find where things diverge in the working and failing cases, but I'd need guidance from you on what to enable. 79466 27 seebs 2018-02-15 22:38:43 +0000 The problem appears to be that O_TMPFILE has bits in it that are also in other modes, so (flags & O_TMPFILE) was the wrong test. Fixed now in upstream master. 79467 28 thiago 2018-02-15 22:59:41 +0000 O_TMPFILE has the bits to O_DIRECTORY 79482 29 alex.kanavin 2018-02-16 10:07:57 +0000 Thanks, that does seem to fix the issue! Also, epoll hanging problem seems to be gone, looks like it was the same problem. I've posted the update patch to oe-core list for review and autobuilder testing: http://lists.openembedded.org/pipermail/openembedded-core/2018-February/147507.html Seebs, I'd like to ask two more things, neither is complicated :) If you could: a) look at remaining patches, and see what can be taken to upstream git? http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/meta/recipes-devtools/pseudo/files?h=akanavin/pseudo-1.9.0 b) then tag the latest upstream commit as, for example, 'pseudo-1.9.1'. I'd really prefer that we start using git tags to map versions to specific commits. Thanks :) 79493 30 alex.kanavin 2018-02-17 11:31:00 +0000 @Samuli @Thiago Can you please try this poky branch and see if a) it generally works; b) it fixes the specific issue that started this bug? http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=akanavin/pseudo-1.9.0 79616 31 alex.kanavin 2018-02-27 14:20:53 +0000 Assigning to Samuli - please do test the fix; we do not want to merge it to oe-core and then find out it does not actually help with the issue that started this bug. 79642 32 samuli.piippo 2018-03-01 11:37:15 +0000 verified that open(O_TMPFILE)+linkat now works with qmake, as far as files are copied. However, there are always random files that fail the host-user-contaminated QA check during packaging. 79643 33 alex.kanavin 2018-03-01 11:43:27 +0000 We're tracking the host user contamination issue elsewhere: bug 12434, bug 11299. I'll send the patch to oe-core list again, so hopefully it makes it in before 2.5 79693 34 alex.kanavin 2018-03-05 02:19:54 +0000 The patch is in oe-core/poky master: http://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=d50b9c511e17c6f582ef39b61eaaa8d4555eabba 4042 2017-10-02 14:39:53 +0000 2017-10-02 14:39:53 +0000 Alex's naive patch 0001-linkat.c-add-special-case-for-linking-a-file-descrip.patch text/plain 1145 alex.kanavin RnJvbSAyM2FiYzIyZDI5YzgwZDQyNzY1YzAxZDRjNmNlZWZiMzVhZmIxNDdiIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBbGV4YW5kZXIgS2FuYXZpbiA8YWxleC5rYW5hdmluQGdtYWls LmNvbT4KRGF0ZTogTW9uLCAyIE9jdCAyMDE3IDE2OjMyOjE5ICswMzAwClN1YmplY3Q6IFtQQVRD SF0gbGlua2F0LmM6IGFkZCBzcGVjaWFsIGNhc2UgZm9yIGxpbmtpbmcgYSBmaWxlIGRlc2NyaXB0 b3IKClNpZ25lZC1vZmYtYnk6IEFsZXhhbmRlciBLYW5hdmluIDxhbGV4LmthbmF2aW5AZ21haWwu Y29tPgotLS0KIHBvcnRzL3VuaXgvZ3V0cy9saW5rYXQuYyB8IDYgKysrKystCiAxIGZpbGUgY2hh bmdlZCwgNSBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvcG9ydHMv dW5peC9ndXRzL2xpbmthdC5jIGIvcG9ydHMvdW5peC9ndXRzL2xpbmthdC5jCmluZGV4IGVjMjdl NDcuLmNkMDE5OWQgMTAwNjQ0Ci0tLSBhL3BvcnRzL3VuaXgvZ3V0cy9saW5rYXQuYworKysgYi9w b3J0cy91bml4L2d1dHMvbGlua2F0LmMKQEAgLTM0LDcgKzM0LDExIEBACiAjZW5kaWYKICAgICAg ICAgb2xkcGF0aCA9IHBzZXVkb19yb290X3BhdGgoX19mdW5jX18sIF9fTElORV9fLCBvbGRkaXJm ZCwgb2xkbmFtZSwgcmZsYWdzKTsKICAgICAgICAgbmV3cGF0aCA9IHBzZXVkb19yb290X3BhdGgo X19mdW5jX18sIF9fTElORV9fLCBuZXdkaXJmZCwgbmV3bmFtZSwgQVRfU1lNTElOS19OT0ZPTExP Vyk7Ci0gICAgICAgIHJjID0gcmVhbF9saW5rKG9sZHBhdGgsIG5ld3BhdGgpOworCisgICAgICAg IGlmICgoZmxhZ3MgJiBBVF9FTVBUWV9QQVRIKSB8fCBzdHJuY21wKG9sZHBhdGgsICIvcHJvYy9z ZWxmL2ZkLyIsIHN0cmxlbigiL3Byb2Mvc2VsZi9mZC8iKSkgPT0gMCkKKyAgICAgICAgICAgICAg ICByYyA9IHJlYWxfbGlua2F0KG9sZGRpcmZkLCBvbGRuYW1lLCBuZXdkaXJmZCwgbmV3bmFtZSwg ZmxhZ3MpOworICAgICAgICBlbHNlCisgICAgICAgICAgICAgICAgcmMgPSByZWFsX2xpbmsob2xk cGF0aCwgbmV3cGF0aCk7CiAgICAgICAgIHNhdmVfZXJybm8gPSBlcnJubzsKICAgICAgICAgaWYg KHJjID09IC0xKSB7CiAgICAgICAgICAgICAgICAgZXJybm8gPSBzYXZlX2Vycm5vOwotLSAKMi4x NC4xCgo=