| Message ID | 20241218185453.367465-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tools/libxg: Don't gunzip the guests initrd | expand |
On 18.12.2024 19:54, Andrew Cooper wrote: > Decompressing the kernel is necessary to inspect the ELF notes, but the > dombuilder will gunzip() secondary modules too. Specifically gunzip(), no > other decompression algorithms. > > This may have been necessary in the dim and distant past, but it is broken > today. Linux specifically supports concatenating CPIO fragments of differing > compressions, and any attempt to interpret it with a single algorithm may > corrupt later parts. > > This was an unexpected discovery while trying to test Xen's gunzip() > logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0). > > Intepreting secondary modules *should* be left as an exersize to the guest. Nit: "Interpreting" and "exercise" and ... > This reduces work done in dom0. > > This is not expected to cause a practical different to guests these days. ... "difference"? Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 12/18/24 7:54 PM, Andrew Cooper wrote: > Decompressing the kernel is necessary to inspect the ELF notes, but the > dombuilder will gunzip() secondary modules too. Specifically gunzip(), no > other decompression algorithms. > > This may have been necessary in the dim and distant past, but it is broken > today. Linux specifically supports concatenating CPIO fragments of differing > compressions, and any attempt to interpret it with a single algorithm may > corrupt later parts. > > This was an unexpected discovery while trying to test Xen's gunzip() > logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0). > > Intepreting secondary modules *should* be left as an exersize to the guest. > This reduces work done in dom0. > > This is not expected to cause a practical different to guests these days. > > Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com> > --- > CC: Anthony PERARD<anthony.perard@vates.tech> > CC: Juergen Gross<jgross@suse.com> > CC: Anthony PERARD<anthony.perard@vates.tech> > CC: Michal Orzel<michal.orzel@amd.com> > CC: Jan Beulich<jbeulich@suse.com> > CC: Julien Grall<julien@xen.org> > CC: Roger Pau Monné<roger.pau@citrix.com> > CC: Stefano Stabellini<sstabellini@kernel.org> > CC: Oleksii Kurochko<oleksii.kurochko@gmail.com> > > v2: > * Rewrite the commit message. > > If this does cause a problem, we can reintroduce it, but behind a config > option because it needs to not be on by default. > > Linux's habit of prepending an uncompressed CPIO containing microcode is > likely to cause this logic to be skipped anyway. > --- > CHANGELOG.md | 2 ++ Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com> Thanks. ~ Oleksii > tools/libs/guest/xg_dom_core.c | 40 ++++++---------------------------- > 2 files changed, 9 insertions(+), 33 deletions(-) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 15f681459f22..61510e6a11c0 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) > > ### Changed > - Fixed blkif protocol specification for sector sizes different than 512b. > + - The dombuilder in libxenguest no longer un-gzips secondary modules, instead > + leaving this to the guest kernel to do in guest context. > - On x86: > - Prefer ACPI reboot over UEFI ResetSystem() run time service call. > - Switched the xAPIC flat driver to use physical destination mode for external > diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c > index f5521d528be1..595b0a667c03 100644 > --- a/tools/libs/guest/xg_dom_core.c > +++ b/tools/libs/guest/xg_dom_core.c > @@ -980,37 +980,24 @@ int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb) > > static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod) > { > - size_t unziplen, modulelen; > + size_t modulelen; > void *modulemap; > char name[10]; > > - if ( !dom->modules[mod].seg.vstart ) > - unziplen = xc_dom_check_gzip(dom->xch, > - dom->modules[mod].blob, dom->modules[mod].size); > - else > - unziplen = 0; > + modulelen = dom->modules[mod].size; > > - modulelen = max(unziplen, dom->modules[mod].size); > - if ( dom->max_module_size ) > + if ( dom->max_module_size && modulelen > dom->max_module_size ) > { > - if ( unziplen && modulelen > dom->max_module_size ) > - { > - modulelen = min(unziplen, dom->modules[mod].size); > - if ( unziplen > modulelen ) > - unziplen = 0; > - } > - if ( modulelen > dom->max_module_size ) > - { > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, > - "module %u image too large", mod); > - goto err; > - } > + xc_dom_panic(dom->xch, XC_INVALID_KERNEL, > + "module %u image too large", mod); > + goto err; > } > > snprintf(name, sizeof(name), "module%u", mod); > if ( xc_dom_alloc_segment(dom, &dom->modules[mod].seg, name, > dom->modules[mod].seg.vstart, modulelen) != 0 ) > goto err; > + > modulemap = xc_dom_seg_to_ptr(dom, &dom->modules[mod].seg); > if ( modulemap == NULL ) > { > @@ -1018,21 +1005,8 @@ static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod) > __FUNCTION__, mod); > goto err; > } > - if ( unziplen ) > - { > - if ( xc_dom_do_gunzip(dom->xch, dom->modules[mod].blob, dom->modules[mod].size, > - modulemap, unziplen) != -1 ) > - return 0; > - if ( dom->modules[mod].size > modulelen ) > - goto err; > - } > > - /* Fall back to handing over the raw blob. */ > memcpy(modulemap, dom->modules[mod].blob, dom->modules[mod].size); > - /* If an unzip attempt was made, the buffer may no longer be all zero. */ > - if ( unziplen > dom->modules[mod].size ) > - memset(modulemap + dom->modules[mod].size, 0, > - unziplen - dom->modules[mod].size); > > return 0; >
diff --git a/CHANGELOG.md b/CHANGELOG.md index 15f681459f22..61510e6a11c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Changed - Fixed blkif protocol specification for sector sizes different than 512b. + - The dombuilder in libxenguest no longer un-gzips secondary modules, instead + leaving this to the guest kernel to do in guest context. - On x86: - Prefer ACPI reboot over UEFI ResetSystem() run time service call. - Switched the xAPIC flat driver to use physical destination mode for external diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c index f5521d528be1..595b0a667c03 100644 --- a/tools/libs/guest/xg_dom_core.c +++ b/tools/libs/guest/xg_dom_core.c @@ -980,37 +980,24 @@ int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb) static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod) { - size_t unziplen, modulelen; + size_t modulelen; void *modulemap; char name[10]; - if ( !dom->modules[mod].seg.vstart ) - unziplen = xc_dom_check_gzip(dom->xch, - dom->modules[mod].blob, dom->modules[mod].size); - else - unziplen = 0; + modulelen = dom->modules[mod].size; - modulelen = max(unziplen, dom->modules[mod].size); - if ( dom->max_module_size ) + if ( dom->max_module_size && modulelen > dom->max_module_size ) { - if ( unziplen && modulelen > dom->max_module_size ) - { - modulelen = min(unziplen, dom->modules[mod].size); - if ( unziplen > modulelen ) - unziplen = 0; - } - if ( modulelen > dom->max_module_size ) - { - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, - "module %u image too large", mod); - goto err; - } + xc_dom_panic(dom->xch, XC_INVALID_KERNEL, + "module %u image too large", mod); + goto err; } snprintf(name, sizeof(name), "module%u", mod); if ( xc_dom_alloc_segment(dom, &dom->modules[mod].seg, name, dom->modules[mod].seg.vstart, modulelen) != 0 ) goto err; + modulemap = xc_dom_seg_to_ptr(dom, &dom->modules[mod].seg); if ( modulemap == NULL ) { @@ -1018,21 +1005,8 @@ static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod) __FUNCTION__, mod); goto err; } - if ( unziplen ) - { - if ( xc_dom_do_gunzip(dom->xch, dom->modules[mod].blob, dom->modules[mod].size, - modulemap, unziplen) != -1 ) - return 0; - if ( dom->modules[mod].size > modulelen ) - goto err; - } - /* Fall back to handing over the raw blob. */ memcpy(modulemap, dom->modules[mod].blob, dom->modules[mod].size); - /* If an unzip attempt was made, the buffer may no longer be all zero. */ - if ( unziplen > dom->modules[mod].size ) - memset(modulemap + dom->modules[mod].size, 0, - unziplen - dom->modules[mod].size); return 0;
Decompressing the kernel is necessary to inspect the ELF notes, but the dombuilder will gunzip() secondary modules too. Specifically gunzip(), no other decompression algorithms. This may have been necessary in the dim and distant past, but it is broken today. Linux specifically supports concatenating CPIO fragments of differing compressions, and any attempt to interpret it with a single algorithm may corrupt later parts. This was an unexpected discovery while trying to test Xen's gunzip() logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0). Intepreting secondary modules *should* be left as an exersize to the guest. This reduces work done in dom0. This is not expected to cause a practical different to guests these days. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Anthony PERARD <anthony.perard@vates.tech> CC: Juergen Gross <jgross@suse.com> CC: Anthony PERARD <anthony.perard@vates.tech> CC: Michal Orzel <michal.orzel@amd.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> v2: * Rewrite the commit message. If this does cause a problem, we can reintroduce it, but behind a config option because it needs to not be on by default. Linux's habit of prepending an uncompressed CPIO containing microcode is likely to cause this logic to be skipped anyway. --- CHANGELOG.md | 2 ++ tools/libs/guest/xg_dom_core.c | 40 ++++++---------------------------- 2 files changed, 9 insertions(+), 33 deletions(-)