lkml.org 
[lkml]   [2021]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] module: add in-kernel support for decompressing
On Fri, Dec 10, 2021 at 04:11:21PM -0800, Luis Chamberlain wrote:
> On Thu, Dec 09, 2021 at 10:09:17PM -0800, Dmitry Torokhov wrote:
> > diff --git a/init/Kconfig b/init/Kconfig
> > index cd23faa163d1..d90774ff7610 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2305,6 +2305,19 @@ config MODULE_COMPRESS_ZSTD
> >
> > endchoice
> >
> > +config MODULE_DECOMPRESS
> > + bool "Support in-kernel module decompression"
> > + depends on MODULE_COMPRESS_GZIP || MODULE_COMPRESS_XZ
> > + select ZLIB_INFLATE if MODULE_COMPRESS_GZIP
> > + select XZ_DEC if MODULE_COMPRESS_XZ
>
> What if MODULE_COMPRESS_GZIP and MODULE_COMPRESS_XZ are enabled?
> These are not mutually exclusive.

They are mutually exclusive, the kernel uses the same (one) compression
method for all kernel modules that it generates (i.e we do not compress
drivers/usb/... with gzip while drivers/net/... with xz).

The idea here is to allow the kernel consume the same format that was
used when generating modules. Supporting multiple formats at once is
overkill IMO.

>
> > + help
> > +
> > + Support for decompressing kernel modules by the kernel itself
> > + instead of relying on userspace to perform this task. Useful when
> > + load pinning security policy is enabled.
>
> Shouldn't kernel decompression be faster too? If so, what's the
> point of doing it in userspace?

Make the kernel smaller? Have more flexibility with exotic compression
formats?

>
> > diff --git a/kernel/module_decompress.c b/kernel/module_decompress.c
> > new file mode 100644
> > index 000000000000..590ca00aa098
> > --- /dev/null
> > +++ b/kernel/module_decompress.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2021 Google LLC.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/highmem.h>
> > +#include <linux/kobject.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/vmalloc.h>
> > +
> > +#include "module-internal.h"
> > +
> > +static int module_extend_max_pages(struct load_info *info, unsigned int extent)
> > +{
> > + struct page **new_pages;
> > +
> > + new_pages = kvmalloc_array(info->max_pages + extent,
> > + sizeof(info->pages), GFP_KERNEL);
> > + if (!new_pages)
> > + return -ENOMEM;
> > +
> > + memcpy(new_pages, info->pages, info->max_pages * sizeof(info->pages));
> > + kvfree(info->pages);
> > + info->pages = new_pages;
> > + info->max_pages += extent;
> > +
> > + return 0;
> > +}
> > +
> > +static struct page *module_get_next_page(struct load_info *info)
> > +{
> > + struct page *page;
> > + int error;
> > +
> > + if (info->max_pages == info->used_pages) {
> > + error = module_extend_max_pages(info, info->used_pages);
> > + if (error)
> > + return ERR_PTR(error);
> > + }
> > +
> > + page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
> > + if (!page)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + info->pages[info->used_pages++] = page;
> > + return page;
> > +}
> > +
> > +#ifdef CONFIG_MODULE_COMPRESS_GZIP
> > +#include <linux/zlib.h>
> > +#define MODULE_COMPRESSION gzip
> > +#define MODULE_DECOMPRESS_FN module_gzip_decompress
>
> So gzip is assumed if your kernel has both gzip and xz. That seems odd.

No, gzuo is used when CONFIG_MODULE_COMPRESS_GZIP is enabled. That means
that CONFIG_MODULE_COMPRESS_XZ is not enabled.

>
> <-- snip -->
>
> > +#elif CONFIG_MODULE_COMPRESS_XZ
> > +#include <linux/xz.h>
> > +#define MODULE_COMPRESSION xz
> > +#define MODULE_DECOMPRESS_FN module_xz_decompress
> > +#else
>
> <-- snip -->
>
> > +#error "Unexpected configuration for CONFIG_MODULE_DECOMPRESS"
>
> Using "depends on" logic on the kconfig symbol would resolve this and
> make this not needed.
>
> Why can't we just inspect the module and determine? Or, why not just add
> a new kconfig symbol under MODULE_DECOMPRESS which lets you specify the
> MODULE_COMPRESSION_TYPE. This way this is explicit.

It is a possibility, I just wanted to decompression scheme match
compression one so there is less potential for misconfiguration.

>
> > +module_init(module_decompress_sysfs_init);
>
> This seems odd, altough it works, can you use late_initcall instead()?

Yes, I can change this.

Thanks.

--
Dmitry

\
 
 \ /
  Last update: 2021-12-11 02:10    [W:0.097 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site