Bug 1470397 - Add 'F' flag to binfmt .conf files, drop qemu-user-static, merge qemu-user-binfmt into qemu-user
Add 'F' flag to binfmt .conf files, drop qemu-user-static, merge qemu-user-bi...
Status: NEW
Product: Fedora
Classification: Fedora
Component: qemu (Show other bugs)
27
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Fedora Virtualization Maintainers
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-12 18:24 EDT by Nathaniel McCallum
Modified: 2018-03-29 08:41 EDT (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch implementing the proposed updates (22.65 KB, patch)
2017-07-13 23:09 EDT, Nathaniel McCallum
no flags Details | Diff
Patch implementing the proposed updates (22.55 KB, patch)
2017-07-18 11:50 EDT, Nathaniel McCallum
no flags Details | Diff

  None (edit)
Description Nathaniel McCallum 2017-07-12 18:24:55 EDT
This PR for DNF enables the ability to install cross-architecture roots:
https://github.com/rpm-software-management/dnf/pull/866

However, actually doing this is cumbersome because qemu-user-static has dependencies. Instead, we should create a package that contains just the static binaries and has no dependencies. If we had this we could do:

$ dnf install --installroot=/tmp/myroot qemu-user-static
$ dnf install --installroot=/tmp/myroot --setopt=arch=armv7hl systemd passwd dnf fedora-release vim-minimal
Comment 1 Daniel Berrange 2017-07-13 05:18:06 EDT
What is the real world problem you're seeing. Dnf can happily resolve the few deps that are needed, so I'm not really finding the idea of dropping them appealing
Comment 2 Nathaniel McCallum 2017-07-13 08:37:36 EDT
In the example I gave you, we are building a root that has only one x86 package (qemu-user-static) and all other packages are armv7hl.

The real world problem is conflicts with cross-architecture roots. qemu-user-static doesn't *really* have any dependencies. But the ones it has, pull in a bunch of packages. But these packages conflict with the same packages from the cross-architecture.

Here is more backdrop:
https://npmccallum.gitlab.io/post/cross-architecture-roots-with-dnf/

We also have a related problem, which is that qemu-user-binfmt and qemu-user-static both install *different* versions of the binfmt conf files. So the example I gave in the blog doesn't actually work (I just discovered) because the host and container have different binaries. We should split apart the binfmt conf files into a separate package and have qemu-user and qemu-user-static have the same binary names and conflict. This way the host can install qemu-user-binfmt and qemu-user while the container can install just qemu-user-static.
Comment 3 Nathaniel McCallum 2017-07-13 12:37:28 EDT
Specifically, the problem is this (derived from the previous example):

$ sudo rpm -ivh --root=/tmp/F25ARM qemu-user-static-*.x86_64.rpm
warning: qemu-user-static-2.7.1-6.fc25.x86_64.rpm: Header V3 RSA/SHA256 Signature, key ID fdb19c98: NOKEY
error: Failed dependencies:
	qemu-common = 2:2.7.1-6.fc25 is needed by qemu-user-static-2:2.7.1-6.fc25.x86_64
	systemd-units is needed by qemu-user-static-2:2.7.1-6.fc25.x86_64


First, we should have a qemu-user-binfmt package (as we currently do). This should depend on either qemu-user or qemu-user-static (it currently depends only on qemu-user). It would be best if we could figure out a way to make dnf prefer qemu-user.

Second, qemu-user-static should have its binaries renamed to remove the -static suffix. It should conflict with qemu-user.

Third, qemu-user-static should drop all its bundled binfmt .conf files.

Fourth, qemu-user-static should drop its qemu-common dependency. I don't believe qemu-common is needed at all.

Users who do "dnf upgrade" and have qemu-user installed should gain qemu-user-binfmt.
Comment 4 Nathaniel McCallum 2017-07-13 18:31:03 EDT
After more research, I think I have a simpler solution.

qemu-user-static uses the 'F' flag in its binfmt, but qemu-user-binfmt doesn't. We need to add the 'F' binfmt flag to qemu-user-binfmt and then we can remove qemu-user-static altogether. It isn't needed in the chroot at all. This will dramatically simplify qemu.spec.

I also don't see any reason to keep the split between qemu-user and qemu-user-binfmt. So (I think) these can be consolidated.
Comment 5 Nathaniel McCallum 2017-07-13 18:54:17 EDT
Here is my first stab at this simplification:

https://koji.fedoraproject.org/koji/taskinfo?taskID=20507784

(I'm posting it here for public record. It is building now...)
Comment 6 Nathaniel McCallum 2017-07-13 22:42:33 EDT
Here's a successful build:

https://koji.fedoraproject.org/koji/taskinfo?taskID=20511080

If I've done everything correctly, upgrades to this package should replace both qemu-user-static and qemu-user-binfmt with qemu-user. Cross-architecture roots should work fine without any modifications (qemu-user does NOT need to be installed).
Comment 7 Nathaniel McCallum 2017-07-13 22:51:23 EDT
BTW, qemu builds are broken on rawhide. But that isn't my fault.
Comment 8 Nathaniel McCallum 2017-07-13 23:09 EDT
Created attachment 1298074 [details]
Patch implementing the proposed updates
Comment 9 Nathaniel McCallum 2017-07-13 23:11:36 EDT
The attached patch implements my proposed updates.

However, qemu-common could still be cleaned up. The qemu-user subpackage depends on it but doesn't actually require anything from it. Nevertheless, this dependency bloat no longer blocks me. So I have not done anything about it.
Comment 10 Nathaniel McCallum 2017-07-17 14:21:33 EDT
Daniel, you should have everything needed to review these packages changes. Do you know when you'll be able to review them?
Comment 11 Nathaniel McCallum 2017-07-18 11:50 EDT
Created attachment 1300545 [details]
Patch implementing the proposed updates
Comment 12 Nathaniel McCallum 2017-07-18 11:52:54 EDT
I've rebased the patch given Daniel's new commit to rawhide today.
Comment 13 Daniel Berrange 2017-07-19 08:09:38 EDT
I built a new QEMU version for Fedora 26 with this patch applied, but AFAICT, it does not work.

I start off with  qemu-user-static installed on my x86_64 host, so i have that setup for binfmt:

# cd /proc/sys/fs/binfmt_misc
# cat qemu-arm
enabled
interpreter /usr/bin/qemu-arm-static
flags: F
offset 0
magic 7f454c4601010100000000000000000002002800
mask ffffffffffffff000000000000000000feffffff


With this, I can chroot into an ARM tree and run commands:

# chroot  /home/berrange/arm-test/vroot /bin/busybox ls
bin   etc   init  lib   root  sbin  tmp   usr


Now, I unregister the binfmt handlers and install the QEMU build with this patch applied

# for i in qemu-* ; do echo -1 > $i ; done

# rpm -U /home/berrange/src/fedora/qemu/x86_64/qemu*-2.9.0-7.fc26.x86_64.rpm


Check that qemu-user-static got replaced by qemu-user:

# rpm -q qemu-user-static
package qemu-user-static is not installed
# rpm -q qemu-user
qemu-user-2.9.0-7.fc26.x86_64



See the binfmt now points to 'qemu-arm' with the "F" flag present:

# cat qemu-arm
enabled
interpreter /usr/bin/qemu-arm
flags: F
offset 0
magic 7f454c4601010100000000000000000002002800
mask ffffffffffffff000000000000000000feffffff


Attempting to run commands in the ARM chroot now fails:

# chroot  /home/berrange/arm-test/vroot /bin/busybox ls
chroot: failed to run command ‘/bin/busybox’: No such file or directory
Comment 14 Nathaniel McCallum 2017-07-19 10:53:45 EDT
I suspect this is because of lazy linking (I haven't been able to confirm this due to network issues this morning). We can do LDFLAGS="-z now", though I'm not sure we want that for the entirety of qemu. Do we want to pursue retiring qemu-user-static in favor of non-lazy linking? Or should we just retain qemu-user-static?
Comment 15 Daniel Berrange 2017-07-19 11:01:02 EDT
QEMU is already linking using  -z,now becaue that's passed in from the RPM specs

eg RPM is calling configure with

 --extra-ldflags="-Wl,--build-id -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -pie -Wl,-z,relro -Wl,-z,now"

and i see that passed to the qemu-arm binary

c++ ....snip... -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g  -Wl,--build-id -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -pie -Wl,-z,relro -Wl,-z,now  -o qemu-arm ...snip...
Comment 16 Nathaniel McCallum 2017-07-19 15:29:12 EDT
From what I can see the original kernel patch[0] only works with static binaries. This is because while the executable file is loaded during configuration time, the loading of libraries is delayed until execution time - which obviously can't work.

I'm looking into our options. I'm hoping this is as easy as moving search_binary_handler() into bm_register_write().

In the meantime, I think we can merge qemu-user-binfmt back into qemu-user. The initial package split was never done correctly (qemu-user still depends on systemd and restarts the binfmt configuration even though it doesn't have binfmt config). Further, there are no packages which depend on qemu-user-binfmt. So, AFAICS, we should just ship the binfmts as part of qemu-user.

Longer term, we can retire qemu-user-static once the kernel supports dynamic binaries with the F flag.

[0]: https://github.com/torvalds/linux/commit/948b701a607f123df92ed29084413e5dd8cda2ed
Comment 17 Daniel Berrange 2017-07-20 04:45:06 EDT
The point of having qemu-user-binfmt separately is that it allows installation of both qemu-user and qemu-user-static at the same time. This is useful because you may have apps / scripts written to assume existance of the long term existing qemu-user package, but none the less need to have qemu-user-static for chroots. This mirrors the way Debian has split their qemu user packages.

The problem you mention with %post scripts is simply a bug we can easily fix.
Comment 18 Nathaniel McCallum 2017-07-20 10:08:16 EDT
Another option would be consolidating all the packages and simply building the qemu userspace emulation as static until we have a kernel fix. Once the kernel is fixed, then we can internally switch to dynamic linking. While this has the disadvantage of having static binaries, it has the advantage of exposing the package name changes earlier rather than later.

I'm happy with either option as a first step. Please let me know which you prefer.
Comment 19 Neal Gompa 2017-07-20 10:11:22 EDT
I would just keep both variants, and once there's no more need for the static one, then we retire that variant.
Comment 20 Nathaniel McCallum 2017-07-20 10:13:33 EDT
I lean this direction as well. Daniel, do you mind if I use  my proven packager abilities to fix the aforementioned bug with the %post scripts and dependencies?

Then we can leave this bug open for a rework later.
Comment 21 Daniel Berrange 2017-07-20 10:27:03 EDT
I don't want to see us get rid of the dynamic linked binaries - users who have no need for the static linked version are better served by having the dynamic linked one installed, as they pick up security updates to any linked libraries immediately.

Feel free to fix the %post script mistake though.
Comment 22 Nathaniel McCallum 2017-07-20 10:54:45 EDT
I have fixed the %post scripts and dependencies in rawhide. It is building now. I spoke with crobinso about backporting to F26, which he'll do next week.

https://koji.fedoraproject.org/koji/buildinfo?buildID=920701

Once all this lands, our main blocker is kernel support for non-static binaries. If we can make that happen, we can consolidate the no-longer-necessary subpackages.
Comment 23 Jan Kurik 2017-08-15 02:24:53 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 27 development cycle.
Changing version to '27'.
Comment 24 Cole Robinson 2018-03-29 08:41:23 EDT
Sounds like there isn't much to do here any more until there's kernel support for dynamic + binfmt 'F' ? Is there a bug for that?

Note, in rawhide I switched binfmt generation to use qemu's qemu-binfmt-conf.sh script which basically does all the same generation we were doing in the spec file. the mask values it uses are more restrictive but I think the net result is the same. Please be on the lookout for regressions

Note You need to log in before you can comment on or make changes to this bug.