Merge ~mfo/grub:lp1987567 into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

Proposed by Mauricio Faria de Oliveira
Status: Merged
Merged at revision: 47a3d1d5e93e54de7f184a4b3cf43465b42959dc
Proposed branch: ~mfo/grub:lp1987567
Merge into: ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu
Diff against target: 224 lines (+204/-0)
3 files modified
debian/patches/linux_xen-Properly-load-multiple-initrd-files.patch (+123/-0)
debian/patches/linux_xen-Properly-order-multiple-initrd-files.patch (+79/-0)
debian/patches/series (+2/-0)
Reviewer Review Type Date Requested Status
Julian Andres Klode Needs Fixing
Review via email: mp+428882@code.launchpad.net

Description of the change

Fixes for grub's xen template to properly handle multiple initrd files
(e.g., /boot/microcode.cpio if present, and /boot/initrd.img-VERSION).

To post a comment you must log in.
Revision history for this message
Julian Andres Klode (juliank) wrote :

Please remove the changelog entry and make sure that the patch series is exported cleanly used gbp pq and does not have hand edited patches.

review: Needs Fixing
Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

Hi Julian! Thanks for reviewing.

Changelog entry removed (should regenerate nicely with `gbp dch`).

Hmm, I did export the patch series with `gbp pq`
(`gbp pq apply` + `git commit --amend` for DEP3/LP: #/Gbp-Pq: Name
for both patches, then `gbp pq export`.)

Could you please clarify what exactly is off / I added incorrectly?

Thanks again,
Mauricio

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/patches/linux_xen-Properly-load-multiple-initrd-files.patch b/debian/patches/linux_xen-Properly-load-multiple-initrd-files.patch
2new file mode 100644
3index 0000000..b0bb155
4--- /dev/null
5+++ b/debian/patches/linux_xen-Properly-load-multiple-initrd-files.patch
6@@ -0,0 +1,123 @@
7+From: Mauricio Faria de Oliveira <mfo@canonical.com>
8+Date: Sat, 6 Aug 2022 20:46:48 -0300
9+Subject: templates/linux_xen: Properly load multiple initrd files
10+MIME-Version: 1.0
11+Content-Type: text/plain; charset="utf-8"
12+Content-Transfer-Encoding: 8bit
13+
14+The linux_xen template can put multiple initrd files in the
15+same multiboot[2] module[2] command, which is against specs.
16+
17+This causes ONLY the _first_ initrd file to be loaded; other
18+files just have filenames in a "cmdline" string of the first
19+initrd file and are NOT loaded.
20+
21+Fix this by inserting a module[2] command per initrd file.
22+
23+Before:
24+
25+ # touch /boot/xen /boot/microcode.cpio
26+ # grub-mkconfig 2>/dev/null | grep -P '^\t(multiboot|module)'
27+ multiboot /boot/xen ...
28+ module /boot/vmlinuz-5.4.0-122-generic ...
29+ module --nounzip /boot/microcode.cpio /boot/initrd.img-5.4.0-122-generic
30+
31+After:
32+
33+ # touch /boot/xen /boot/microcode.cpio
34+ # grub-mkconfig 2>/dev/null | grep -P '^\t(multiboot|module)'
35+ multiboot /boot/xen ...
36+ module /boot/vmlinuz-5.4.0-122-generic ...
37+ module --nounzip /boot/microcode.cpio
38+ module --nounzip /boot/initrd.img-5.4.0-122-generic
39+
40+Cause:
41+
42+The code was copied from the linux template, which is *apparently*
43+equivalent.. but its backing command grub_cmd_initrd() *supports*
44+multiple files (see grub_initrd_init()), while grub_cmd_module()
45+*does not* (see grub_multiboot[2]_add_module()).
46+
47+See commit e86f6aafb8de ("grub-mkconfig/20_linux_xen: Support multiple early initrd images"):
48+ 'This is basically a copy of a698240d "grub-mkconfig/10_linux:
49+ Support multiple early initrd images" ...'
50+
51+Specs:
52+
53+Both multiboot and multiboot2 specifications mention support for
54+'multiple boot modules' (struct/tag used for kernel/initrd files):
55+
56+ "Boot loaders don’t have to support multiple boot modules,
57+ but they are strongly encouraged to" [1,2]
58+
59+However, there is a 1:1 relationship between boot modules and files,
60+more or less clearly; note the usage of singular/plural "module(s)".
61+(Multiboot2, clearly: "One tag appears per module".)
62+
63+ Multiboot [1]:
64+
65+ "the ‘mods’ fields indicate ... what boot modules
66+ were loaded ..., and where they can be found.
67+ ‘mods_count’ contains the number of modules loaded"
68+
69+ "The first two fields contain the start and end addresses
70+ of the boot module itself."
71+
72+ Multiboot2 [2]:
73+
74+ "This tag indicates ... what boot module was loaded ...,
75+ and where it can be found."
76+
77+ "The ‘mod_start’ and ‘mod_end’ contain the start and end
78+ physical addresses of the boot module itself."
79+
80+ "One tag appears per module.
81+ This tag type may appear multiple times."
82+
83+And both clearly mention the 'string' field of a boot module,
84+which is to be used by the operating system, not boot loader:
85+
86+ "The ‘string’ field provides an arbitrary string to be
87+ associated with that particular boot module ...
88+ its exact use is specific to the operating system."
89+
90+Links:
91+
92+[1] https://www.gnu.org/software/grub/manual/multiboot/multiboot.html
93+ 3.3 Boot information format
94+
95+[2] https://www.gnu.org/software/grub/manual/multiboot2/multiboot.html
96+ 3.6.6 Modules
97+
98+Fixes: e86f6aafb8de ("grub-mkconfig/20_linux_xen: Support multiple early initrd images")
99+
100+Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
101+
102+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1987567
103+Origin: backport, https://git.savannah.gnu.org/cgit/grub.git/commit/?id=b4b4acaf4ec7af1a78d122c10baed4e85187e2a5
104+[mfo: backport: refresh lower context lines.]
105+LP: #1987567
106+---
107+ util/grub.d/20_linux_xen.in | 6 +++---
108+ 1 file changed, 3 insertions(+), 3 deletions(-)
109+
110+diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
111+index a12780e..6f38c5d 100644
112+--- a/util/grub.d/20_linux_xen.in
113++++ b/util/grub.d/20_linux_xen.in
114+@@ -166,12 +166,12 @@ EOF
115+ message="$(gettext_printf "Loading initial ramdisk ...")"
116+ initrd_path=
117+ for i in ${initrd}; do
118+- initrd_path="${initrd_path} ${rel_dirname}/${i}"
119+- done
120+- sed "s/^/$submenu_indentation/" << EOF
121++ initrd_path="${rel_dirname}/${i}"
122++ sed "s/^/$submenu_indentation/" << EOF
123+ echo '$(echo "$message" | grub_quote)'
124+ ${module_loader} --nounzip $(echo $initrd_path)
125+ EOF
126++ done
127+ fi
128+ if ${xsm} && test -n "${xenpolicy}" ; then
129+ message="$(gettext_printf "Loading XSM policy ...")"
130diff --git a/debian/patches/linux_xen-Properly-order-multiple-initrd-files.patch b/debian/patches/linux_xen-Properly-order-multiple-initrd-files.patch
131new file mode 100644
132index 0000000..2e38f3b
133--- /dev/null
134+++ b/debian/patches/linux_xen-Properly-order-multiple-initrd-files.patch
135@@ -0,0 +1,79 @@
136+From: Mauricio Faria de Oliveira <mfo@canonical.com>
137+Date: Sat, 6 Aug 2022 22:07:58 -0300
138+Subject: templates/linux_xen: Properly order the multiple initrd files
139+
140+The linux_xen template orders the "early" initrd file(s) _first_
141+(i.e., before the "real" initrd files) and that seems reasonable,
142+as microcode updates usually come first.
143+
144+However, this usually breaks Linux boot with initrd under Xen
145+because Xen assumes the real initrd is the first multiboot[2]
146+module after the kernel, passing its address over to Linux.
147+
148+So, if a microcode-only initrd (i.e., without init/userspace)
149+is found by grub-mkconfig, it ends up considered as a normal
150+initrd by the Linux kernel, which cannot do anything with it
151+(as it has no other files) and panic()s unable to mount root
152+if it depends on a initrd to do that (e.g., root=UUID=...).
153+
154+...
155+
156+Well, since Xen doesn't actually use the provided microcode
157+by default / unless the 'ucode=<module number|scan>' option
158+is enabled, this isn't used in the general case (and breaks).
159+
160+Additionally, if an user enables the 'ucode=' option, that
161+either specifies which module is to be used for microcode,
162+or scans all modules (regardless of being first) for that.
163+
164+Thus, for Xen:
165+- it is *not required* to have microcode first,
166+- but it is *required* to have real initrd first
167+
168+So, fix it by ordering the real initrd before early initrd(s).
169+
170+...
171+
172+Corner case specific to Xen implementation details:
173+
174+It is actually _possible_ to have a microcode initrd first,
175+but that requires a non-default option (so can't rely on it),
176+and it turns out to be inconsistent with its counterpart
177+(really shouldn't rely on it, as it may get confusing; below).
178+
179+'ucode=1' does manually specify the first module is microcode
180+_AND_ clears its bit in the module bitmap. The next module is
181+now the 'new first', and gets passed to Linux as initrd. Good.
182+
183+'ucode=scan' checks all modules for microcode, but does _NOT_
184+clear a bit if it finds one (reasonable, as it can find that
185+prepended in a "real" initrd anyway, which needs to be used).
186+The first module still gets passed to Linux as initrd. Bad.
187+
188+Fixes: e86f6aafb8de ("grub-mkconfig/20_linux_xen: Support multiple early initrd images")
189+
190+Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
191+
192+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1987567
193+Origin: upstream, https://git.savannah.gnu.org/cgit/grub.git/commit/?id=18d8eafdea2322dc80c37e826a75e4d62094fecc
194+LP: #1987567
195+---
196+ util/grub.d/20_linux_xen.in | 5 ++++-
197+ 1 file changed, 4 insertions(+), 1 deletion(-)
198+
199+diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
200+index 6f38c5d..0629890 100644
201+--- a/util/grub.d/20_linux_xen.in
202++++ b/util/grub.d/20_linux_xen.in
203+@@ -304,7 +304,10 @@ while [ "x${xen_list}" != "x" ] ; do
204+
205+ initrd=
206+ if test -n "${initrd_early}" || test -n "${initrd_real}"; then
207+- initrd="${initrd_early} ${initrd_real}"
208++ # Xen assumes the real initrd is the first module after the kernel.
209++ # Additional (later) initrds can also be used for microcode update,
210++ # with Xen option 'ucode=<scan|module number> (non-default anyway).
211++ initrd="${initrd_real} ${initrd_early}"
212+
213+ initrd_display=
214+ for i in ${initrd}; do
215diff --git a/debian/patches/series b/debian/patches/series
216index 091eccf..39d91e4 100644
217--- a/debian/patches/series
218+++ b/debian/patches/series
219@@ -159,3 +159,5 @@ ubuntu-disable-LOAD-FILE2-protocol-for-initrd-on-ARM.patch
220 0159-fs-btrfs-Fix-several-fuzz-issues-with-invalid-dir-it.patch
221 0160-fs-btrfs-Fix-more-ASAN-and-SEGV-issues-found-with-fu.patch
222 0161-fs-btrfs-Fix-more-fuzz-issues-related-to-chunks.patch
223+linux_xen-Properly-load-multiple-initrd-files.patch
224+linux_xen-Properly-order-multiple-initrd-files.patch

Subscribers

People subscribed via source and target branches