Merge ~paelzer/ubuntu/+source/qemu:lp-1913421-noexec-mount-upgrade-modules-HIRSUTE into ubuntu/+source/qemu:ubuntu/hirsute-devel
- Git
- lp:~paelzer/ubuntu/+source/qemu
- lp-1913421-noexec-mount-upgrade-modules-HIRSUTE
- Merge into ubuntu/hirsute-devel
Status: | Rejected | ||||
---|---|---|---|---|---|
Rejected by: | Christian Ehrhardt | ||||
Proposed branch: | ~paelzer/ubuntu/+source/qemu:lp-1913421-noexec-mount-upgrade-modules-HIRSUTE | ||||
Merge into: | ubuntu/+source/qemu:ubuntu/hirsute-devel | ||||
Diff against target: |
110 lines (+55/-2) 4 files modified
debian/changelog (+14/-0) debian/patches/series (+1/-0) debian/patches/ubuntu/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch (+31/-0) debian/rules (+9/-2) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Dan Streetman (community) | Disapprove | ||
Victor Tapia (community) | Needs Fixing | ||
Sergio Durigan Junior (community) | Needs Fixing | ||
Canonical Server | Pending | ||
git-ubuntu developers | Pending | ||
Review via email: mp+397008@code.launchpad.net |
Commit message
Description of the change
Christian Ehrhardt (paelzer) wrote : | # |
Christian Ehrhardt (paelzer) wrote : | # |
Back to WIP, as I need a way to start the mount first
Christian Ehrhardt (paelzer) wrote : | # |
To be clear, my plan is to submit this to Debian to agree on one way to do it (just as I did with the initial save-the-modules code). But before doing so having a team sanity check would be great.
- 5af3a83... by Mauricio Faria de Oliveira
-
1:5.2+dfsg-3ubuntu2 (patches unapplied)
Imported using git-ubuntu import.
Sergio Durigan Junior (sergiodj) wrote : | # |
Thanks for the MP, Christian.
I read the whole discussion in the bug; I always like how you try to explain your thought process in detail, it helps a lot.
I haven't experimented with the possible solutions proposed in the bug, but I tend to agree that your approach is sensible and makes sense. It'd be cleaner to use tmpfiles, but I don't see how that could handle the "non-noexec" requirement, indeed.
I found a few nits that need fixing in the MP, and I'm interested in knowing the Debian Maintainer's opinion about it. I also think that it might be worth asking ddstreet to take a look, since he expressed his reservations about the approach.
Christian Ehrhardt (paelzer) wrote : | # |
Thanks for the review Sergio!
I leave these notes on passed stages and thoughts mostly for myself if I come back later and don't remember :-) But yeah often enough they help people to trace the thoughts that went into a proposed solution and avoids bringing up the same counters over and over again :-)
Sometimes I happen to be a bit wordy (if a task goes on for a while), so I'm glad you appreciate it.
I fixed all your Nits and pushed, thanks.
Christian Ehrhardt (paelzer) wrote : | # |
I also agree that ddstreet (and also Victor) should take a look before I reach out to Debian.
Adding review slots for them.
Victor Tapia (vtapia) wrote : | # |
I'm facing an issue while reinstalling/
Removing qemu-block-extra (1:5.2+
/var/lib/
dpkg: error processing package qemu-block-extra (--remove):
installed qemu-block-extra package post-removal script subprocess returned error exit status 2
Errors were encountered while processing:
qemu-block-extra
# cat /var/lib/
#!/bin/sh
set -e
case $1 in (purge|remove) rm -f /run/qemu/
Victor Tapia (vtapia) wrote : | # |
Also, I just noticed that the qemu-block-extra default file is not installed in /etc/default after upgrading from current. I'm testing again to confirm
Victor Tapia (vtapia) wrote : | # |
Confirmed, the file is not installed and downgrading triggers the error:
dpkg: warning: downgrading qemu-block-extra from 1:5.2+dfsg-
(Reading database ... 90691 files and directories currently installed.)
Preparing to unpack .../qemu-
/var/lib/
dpkg: error processing archive /var/cache/
installed qemu-block-extra package pre-removal script subprocess returned error exit status 127
Errors were encountered while processing:
/var/cache/
E: Sub-process /usr/bin/dpkg returned an error code (1)
Where the error comes while trying to source the default file:
#!/bin/sh
set -e
case $1 in (upgrade|
- 506a15f... by Christian Ehrhardt
-
changelog: provide run-qemu.mount to be able to load modules after upgrade (LP: #1913421)
Signed-off-by: Christian Ehrhardt <email address hidden>
Christian Ehrhardt (paelzer) wrote : | # |
Hi Victor,
yeah - the initial issue you found came from a stray "fi" left from former code.
I've already seen that and resolved it yesterday.
But that isn't yet in 1:5.2+dfsg-
That fail on downgrades due to the missing config file is a good catch. I've pulled it forward and now ignore errors.
The default file was installed in between, but indeed int he current PPA version is not provided.
I see that is because I moved from qemu-system-common to just qemu-block-extra (to keep all changes local to one PKG). The name needs to match in the dh_installinit call.
Also rebased and rebuilt in the PPA, new version there is:
1:5.2+
Christian Ehrhardt (paelzer) wrote : | # |
@Sergio/@Victor - can I have your re-review of the updated MP please?
@Dan - I know time is scarce, but do you think you will be able to have a look as well?
Victor Tapia (vtapia) wrote : | # |
Reinstalling triggers a prerm issue:
root@sprite:
' ' ')
Reading package lists... Done
Building dependency tree
Reading state information... Done
0 upgraded, 0 newly installed, 5 reinstalled, 0 to remove and 0 not upgraded.
Need to get 3280 kB of archives.
After this operation, 0 B of additional disk space will be used.
Get:1 http://
Get:2 http://
Get:3 http://
Get:4 http://
Get:5 http://
Fetched 3280 kB in 5s (715 kB/s)
(Reading database ... 89356 files and directories currently installed.)
Preparing to unpack .../qemu_
Unpacking qemu (1:5.2+
Preparing to unpack .../qemu-
/var/lib/
/var/lib/
Unpacking qemu-block-extra (1:5.2+
Preparing to unpack .../qemu-
Unpacking qemu-system-common (1:5.2+
Preparing to unpack .../qemu-
Unpacking qemu-system-data (1:5.2+
Preparing to unpack .../qemu-
Unpacking qemu-system-
Setting up qemu (1:5.2+
Setting up qemu-system-
Setting up qemu-system-data (1:5.2+
Setting up qemu-block-extra (1:5.2+
Setting up qemu-system-common (1:5.2+
Processing triggers for man-db (2.9.3-2) ...
Processing triggers for mailcap (3.68ubuntu1) ...
Processing triggers for hicolor-icon-theme (0.17-2) ...
root@sprite:
#!/bin/sh
set -e
source /etc/default/
The issue happens because source is a bash builtin but dash doesn't know about it (same with ...
Victor Tapia (vtapia) wrote : | # |
FWIW:
root@sprite:
lrwxrwxrwx 1 root root 4 Jan 19 12:01 /usr/bin/sh -> dash*
Dan Streetman (ddstreet) wrote : | # |
I have 2 objections: 1) the mount approach is unnecessarily and excessively complex, and 2) the opt-in is a non-starter for me.
I think this can be fairly trivial with tmpfiles, let me see if i can get a MR ready.
Christian Ehrhardt (paelzer) wrote : | # |
I'm happy to see/consider/
Let us know when you have something or if you had to give up for some reason.
IMHO after the discussion I had mentioned I agree that making it an opt-in is a strict requirement. But that part can be discussed separately to the mount/tmpfiles details.
Until then I'll still fix up the bits in my proposal.
@Victor - I didn't have much time for this before and need to beg your pardon for being my alpha-tester. I've now run that a few times and sorted out the dash-in-
Working re-install
root@h-
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following package was automatically installed and is no longer required:
libfreetype6
Use 'apt autoremove' to remove it.
0 upgraded, 0 newly installed, 1 reinstalled, 0 to remove and 22 not upgraded.
Need to get 142 kB of archives.
After this operation, 0 B of additional disk space will be used.
Get:1 http://
Fetched 142 kB in 0s (673 kB/s)
(Reading database ... 32746 files and directories currently installed.)
Preparing to unpack .../qemu-
Unpacking qemu-block-extra (1:5.2+
Setting up qemu-block-extra (1:5.2+
No /var/run/qemu, and if I set the config value to YES then the next re-install saves the modules.
Dan Streetman (ddstreet) wrote : | # |
> Let us know when you have something or if you had to give up for some reason.
i threw this together quickly, it correctly handles saving the modules in prerm and removing them on boot, but i didn't test if qemu could load them correctly.
https:/
Christian Ehrhardt (paelzer) wrote : | # |
Dan finally made me see that tmpfiles could - instead of providing a dir which they can't in the way we need - be used to clean afterwards.
I've combined what I like of both our approaches into this branch here.
Force-pushed and ready for re-review.
The PPA has the new (yet untested) version.
For testing with libvirt use:
cat >> /etc/apparmor.
# let qemu load old shared objects after upgrades (LP: #1847361)
/usr/
# but explicitly deny writing to these files
audit deny /usr/lib/
EOF
FYI:
tag: lp-1913421-
Dan Streetman (ddstreet) wrote : | # |
comments inline below; I'm still 100% a nak on this being opt-in.
Christian Ehrhardt (paelzer) wrote : | # |
@Dan - I'll bring the opt-in discussion to the Server Team to probe for more opinions once the carnival mass PTO is over. I need to make it an opt-in for Debian to not be RC removed by some people :-)
But we could get the code there and set our default differently if we decide to be default-on with opt-out.
Looking at the inline comments now ...
Christian Ehrhardt (paelzer) wrote : | # |
@Dan - I have fixed-and-
Thank you!
@Victor / @Sergio - this reaches a much more polished state now - I guess it would be time to re-review once you are around.
Christian Ehrhardt (paelzer) wrote : | # |
I was before torn between the extreme positions of some DDs which made me feel unwell about the feature. And at the same time I was always understanding ddstreet and the user/customer who most likely just wants things to work.
So I didn't need anyone to yes/no to the configuration feature, what I needed was some hard fact-based (and thereby defensible) discussion why we need/not-need to make it configurable (and as a follow on opt in/out).
Thanks to a great discussion with racb I feel much better now on this.
We have taken a step back and checked how a Debian system and the Debian policy works in geenral in that regard.
And actually, we don't violate anything.
If this would be just a binary (no .so files) after an upgrade it would still be running.
Nothing in Debian or the policy forces it to restart.
If you want there are add-on tools like need-restart which help with that, but that is separate.
Without our feature upgraded processes are broken as some of their former hot-add features now fail - and all we do is fixing that. After all that is why I implemented all of this.
So all we do is ensure the running binary does not break (good), we do not encourage/help to keep broken things. Everyone knows and everywhere we state that on an upgrade things should be restarted.
We then discussed the need or use-case of default-on opt-out but even that serves no usual policy use case. There also is no config that says "after upgrade your running `less` process can't page down anymore" (which is like the hot-add of qemu) and we don't have to opt-in/out of that.
So yeah, I feel re-assured in our initial stance of this being a good feature and that default-enabled makes sense.
I know that ddstreet is +1 on that and from my old discussions Victor would have wanted it to be configurable (he didn't).
So I'm dropping the configurability now (but keep the working code if I ever need it)...
Christian Ehrhardt (paelzer) wrote : | # |
Changes force pushed (which are much smaller now) and PPA updated.
To me this now feels polished enough to start to get the opinion of the Debian maintainer as well.
Unless there is harsh feedback until tomorrow I'll submit an MR for discussion there as well.
Christian Ehrhardt (paelzer) wrote : | # |
FYI - I've also opened up the Debian discussion at: https:/
To some extend (now that we seem to somewhat settle on an approach) those can be had concurrently.
Furthermore I've pinged mdeslaur to think if the path would have any security implications that come to his mind.
Christian Ehrhardt (paelzer) wrote : | # |
FYI
[13:26] <mdeslaur> that path looks ok to me...last discussions I nacked putting it somewhere in /tmp (re @paelzer: mdeslaur: to be sure I wanted to ask if you'd see any security problems from that path change?)
Christian Ehrhardt (paelzer) wrote : | # |
part of the 6.0 merge now and the recent activity in Debian.
Abandoning this MR
Unmerged commits
- 506a15f... by Christian Ehrhardt
-
changelog: provide run-qemu.mount to be able to load modules after upgrade (LP: #1913421)
Signed-off-by: Christian Ehrhardt <email address hidden>
- 2144c45... by Christian Ehrhardt
-
* Improve saving modules on upgrades (LP: #1913421)
- d/p/u/lp-1913421- keep-old- package- block-modules- in-usr- lib-dir. patch:
change the fallback-load path from /var/run/qemu to
/usr/lib/ @{multiarch} /qemu/uninstall ed/
- d/rules: provide and install qemu-block-extra.tmpfiles to clear
old modules on reboot
- d/rules: change backup path to /usr/lib/@{multiarch} /qemu/uninstall ed/
- d/rules: also clear directories on remove
- d/rules: clear all modules on purgeSigned-off-by: Christian Ehrhardt <email address hidden>
- 5af3a83... by Mauricio Faria de Oliveira
-
1:5.2+dfsg-3ubuntu2 (patches unapplied)
Imported using git-ubuntu import.
- c16535e... by Christian Ehrhardt
-
changelog: lp-1907789-
build-no- pie-is- no-functional- liker-flag. patch is still needed until 6.0 is released Signed-off-by: Christian Ehrhardt <email address hidden>
- c8eafcb... by Christian Ehrhardt
-
d/p/ubuntu/
lp-1907789- build-no- pie-is- no-functional- liker-flag. patch: fix ld usage of -no-pie (LP: #1907789) Signed-off-by: Christian Ehrhardt <email address hidden>
- c4c9987... by Christian Ehrhardt
-
d/control: regenerated from d/control-in
Signed-off-by: Christian Ehrhardt <email address hidden>
- 7b8ba01... by Christian Ehrhardt
-
changelog: 1:5.2+dfsg-3ubuntu1
Signed-off-by: Christian Ehrhardt <email address hidden>
- 1976759... by Christian Ehrhardt
-
changelog: 1:5.2+dfsg-2ubuntu1
Signed-off-by: Christian Ehrhardt <email address hidden>
- b879110... by Christian Ehrhardt
-
Update qemu machine types to match 5.2
Signed-off-by: Christian Ehrhardt <email address hidden>
- be18b53... by Christian Ehrhardt
-
merge-changelogs
Signed-off-by: Christian Ehrhardt <email address hidden>
Preview Diff
1 | diff --git a/debian/changelog b/debian/changelog |
2 | index ef8158a..b3e2408 100644 |
3 | --- a/debian/changelog |
4 | +++ b/debian/changelog |
5 | @@ -1,3 +1,17 @@ |
6 | +qemu (1:5.2+dfsg-3ubuntu3) hirsute; urgency=medium |
7 | + |
8 | + * Improve saving modules on upgrades (LP: #1913421) |
9 | + - d/p/u/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch: |
10 | + change the fallback-load path from /var/run/qemu to |
11 | + /usr/lib/@{multiarch}/qemu/uninstalled/ |
12 | + - d/rules: provide and install qemu-block-extra.tmpfiles to clear |
13 | + old modules on reboot |
14 | + - d/rules: change backup path to /usr/lib/@{multiarch}/qemu/uninstalled/ |
15 | + - d/rules: also clear directories on remove |
16 | + - d/rules: clear all modules on purge |
17 | + |
18 | + -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Wed, 27 Jan 2021 13:45:55 +0100 |
19 | + |
20 | qemu (1:5.2+dfsg-3ubuntu2) hirsute; urgency=medium |
21 | |
22 | * No change rebuild to pick up liburing. (LP: #1914145) |
23 | diff --git a/debian/patches/series b/debian/patches/series |
24 | index 7826812..5af0590 100644 |
25 | --- a/debian/patches/series |
26 | +++ b/debian/patches/series |
27 | @@ -16,3 +16,4 @@ ubuntu/enable-svm-by-default.patch |
28 | ubuntu/define-ubuntu-machine-types.patch |
29 | ubuntu/pre-bionic-256k-ipxe-efi-roms.patch |
30 | ubuntu/lp-1907789-build-no-pie-is-no-functional-liker-flag.patch |
31 | +ubuntu/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch |
32 | diff --git a/debian/patches/ubuntu/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch b/debian/patches/ubuntu/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch |
33 | new file mode 100644 |
34 | index 0000000..2224336 |
35 | --- /dev/null |
36 | +++ b/debian/patches/ubuntu/lp-1913421-keep-old-package-block-modules-in-usr-lib-dir.patch |
37 | @@ -0,0 +1,31 @@ |
38 | +From 2fee97487b47b9d36af9f220fdb76d10c032a6a4 Mon Sep 17 00:00:00 2001 |
39 | +From: Dan Streetman <ddstreet@canonical.com> |
40 | +Date: Wed, 10 Feb 2021 21:00:53 -0500 |
41 | +Subject: [PATCH] keep old package block modules in /usr/lib dir and use |
42 | + tmpfiles.d to remove them on boot |
43 | +Bug-Ubuntu: https://launchpad.net/bugs/1913421 |
44 | + |
45 | +The originally chosen path in /var/run/qemu is in many systems mounted |
46 | +as noexec and thereby not usable for the original purpose of a fallback |
47 | +load of shared objects. |
48 | + |
49 | +Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> |
50 | + |
51 | +--- |
52 | + |
53 | +diff --git a/util/module.c b/util/module.c |
54 | +index c65060c167d..c89c1e51106 100644 |
55 | +--- a/util/module.c |
56 | ++++ b/util/module.c |
57 | +@@ -246,7 +246,7 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail) |
58 | + version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION), |
59 | + G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~", |
60 | + '_'); |
61 | +- dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir); |
62 | ++ dirs[n_dirs++] = g_strdup_printf("%s/uninstalled/%s", CONFIG_QEMU_MODDIR, version_dir); |
63 | + #endif |
64 | + |
65 | + assert(n_dirs <= ARRAY_SIZE(dirs)); |
66 | +-- |
67 | +2.25.1 |
68 | + |
69 | diff --git a/debian/rules b/debian/rules |
70 | index 956015a..ae6149e 100755 |
71 | --- a/debian/rules |
72 | +++ b/debian/rules |
73 | @@ -38,7 +38,7 @@ enable_linux_user = $(if $(filter qemu-user,${BUILD_PACKAGES}),enable,disable) |
74 | |
75 | FIRMWAREPATH = /usr/share/qemu:/usr/share/seabios:/usr/lib/ipxe/qemu |
76 | PKGVERSION = Debian ${DEB_VERSION} |
77 | -SAVEMODDIR = /run/qemu/$(shell echo -n "${PKGVERSION}" | tr --complement '[:alnum:]+-.~' '_') |
78 | +SAVEMODDIR = ${libdir}/qemu/uninstalled/$(shell echo -n "${PKGVERSION}" | tr --complement '[:alnum:]+-.~' '_') |
79 | sysdatadir = debian/qemu-system-data/usr/share/qemu |
80 | |
81 | ALPHAEV67_CROSSPFX = alpha-linux-gnu- |
82 | @@ -269,7 +269,9 @@ binary-arch: |
83 | # other module types for now (5.0) can't be loaded at runtime, only at startup |
84 | echo 'case $$1 in (upgrade|deconfigure) mkdir -p ${SAVEMODDIR}; cp -p ${libdir}/qemu/block-*.so ${SAVEMODDIR}/;; esac' \ |
85 | >> debian/qemu-block-extra.prerm.debhelper |
86 | - echo 'case $$1 in (purge|remove) rm -f ${SAVEMODDIR}/block-*.so;; esac' \ |
87 | + echo 'case $$1 in (remove) rm -f ${SAVEMODDIR}/block-*.so; rmdir --ignore-fail-on-non-empty ${SAVEMODDIR};; esac' \ |
88 | + >> debian/qemu-block-extra.postrm.debhelper |
89 | + echo 'case $$1 in (purge) rm -rf ${libdir}/qemu/uninstalled;; esac' \ |
90 | >> debian/qemu-block-extra.postrm.debhelper |
91 | |
92 | ifeq (${enable_system},enable) |
93 | @@ -414,6 +416,10 @@ endif |
94 | dh_installsystemd -a -pqemu-system-common --no-restart-on-upgrade --name=qemu-kvm |
95 | dh_installinit -a -pqemu-guest-agent |
96 | dh_installsystemd -a -pqemu-guest-agent --no-start --no-enable |
97 | + echo "# loadable modules from uninstalled/upgraded qemu package versions are stored" > debian/qemu-block-extra.tmpfiles |
98 | + echo "# here, for still-running instances; this removes them on boot" >> debian/qemu-block-extra.tmpfiles |
99 | + echo "D /usr/lib/${DEB_HOST_MULTIARCH}/qemu/uninstalled 0755 root root -" >> debian/qemu-block-extra.tmpfiles |
100 | + dh_installtmpfiles -a |
101 | dh_link -a |
102 | dh_lintian -a |
103 | dh_strip -a |
104 | @@ -607,6 +613,7 @@ clean: debian/control |
105 | dh_testdir |
106 | rm -rf b |
107 | find scripts/ -name '*.pyc' -delete || : |
108 | + rm -f debian/qemu-block-extra.tmpfiles |
109 | rm -f debian/qemu-user.1 |
110 | dh_clean |
111 |
PPA: https:/ /launchpad. net/~paelzer/ +archive/ ubuntu/ lp-1913421- qemu-module- noexec