[PATCH 0/5] libkmod: Use kernel decompression support

[PATCH 0/5] libkmod: Use kernel decompression support

[Lucas De Marchi]

When kernel is built with CONFIG_MODULE_DECOMPRESS=y, it can handle 1
algorithm for module decompression with finit_module(). When that
algorithm matches the one used in the module we are trying to load,
prefer using the in-kernel decompression.  This way the kernel can also
apply any additional security measures based on where the module is
coming from.

In future, if the kernel supports more algorithms at a time, libkmod
could even be compiled without them and just let the kernel handle it.
Since it's likely a distro kernel supports all of them, that would
seem a good thing to do (on the other hand, tools like modinfo and
depmod wouldn't be able read the module information).

For zstd, this needs the following fix on the kernel side:
https://lore.kernel.org/linux-modules/ZHkQNQK5zrzo4Cq2@bombadil.infradead.org/

Lucas De Marchi (5):
  libkmod: Do not inititialize file->memory on open
  libkmod: Extract finit_module vs init_module paths
  libkmod: Keep track of compression type
  libkmod: Keep track of in-kernel compression support
  libkmod: Use kernel decompression when available

 libkmod/libkmod-elf.c      |   5 ++
 libkmod/libkmod-file.c     |  46 +++++++++-----
 libkmod/libkmod-internal.h |  13 +++-
 libkmod/libkmod-module.c   | 127 ++++++++++++++++++++++++-------------
 libkmod/libkmod.c          |  42 ++++++++++++
 5 files changed, 170 insertions(+), 63 deletions(-)

-- 
2.40.1

[PATCH 1/5] libkmod: Do not inititialize file->memory on open

[Lucas De Marchi]

Add a separate function to load the file contents when it's needed.
When it's not needed on the path of loading modules via finit_module(),
there is no need to mmap the file. This will help support loading
modules with the in-kernel compression support.

This is done differently than the lazy initialization for
kmod_file_get_elf() because on the contents case there is also the
file->size to be updated. It would be a weird API to return the pointer
and have the size changed as a side-effect.

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
 libkmod/libkmod-elf.c      |  5 +++++
 libkmod/libkmod-file.c     | 17 ++++++++++++++---
 libkmod/libkmod-internal.h |  3 ++-
 libkmod/libkmod-module.c   |  2 ++
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/libkmod/libkmod-elf.c b/libkmod/libkmod-elf.c
index fb2e3d9..933825b 100644
--- a/libkmod/libkmod-elf.c
+++ b/libkmod/libkmod-elf.c
@@ -281,6 +281,11 @@ struct kmod_elf *kmod_elf_new(const void *memory, off_t size)
 	assert_cc(sizeof(uint32_t) == sizeof(Elf32_Word));
 	assert_cc(sizeof(uint32_t) == sizeof(Elf64_Word));
 
+	if (!memory) {
+		errno = -EINVAL;
+		return NULL;
+	}
+
 	class = elf_identify(memory, size);
 	if (class < 0) {
 		errno = -class;
diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index b6a8cc9..1449c41 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -421,6 +421,7 @@ struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
 	if (file->elf)
 		return file->elf;
 
+	kmod_file_load_contents(file);
 	file->elf = kmod_elf_new(file->memory, file->size);
 	return file->elf;
 }
@@ -431,7 +432,7 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 	struct kmod_file *file = calloc(1, sizeof(struct kmod_file));
 	const struct comp_type *itr;
 	size_t magic_size_max = 0;
-	int err;
+	int err = 0;
 
 	if (file == NULL)
 		return NULL;
@@ -477,8 +478,8 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 	if (file->ops == NULL)
 		file->ops = &reg_ops;
 
-	err = file->ops->load(file);
 	file->ctx = ctx;
+
 error:
 	if (err < 0) {
 		if (file->fd >= 0)
@@ -491,6 +492,14 @@ error:
 	return file;
 }
 
+int kmod_file_load_contents(struct kmod_file *file)
+{
+	if (file->memory)
+		return 0;
+
+	return file->ops->load(file);
+}
+
 void *kmod_file_get_contents(const struct kmod_file *file)
 {
 	return file->memory;
@@ -516,7 +525,9 @@ void kmod_file_unref(struct kmod_file *file)
 	if (file->elf)
 		kmod_elf_unref(file->elf);
 
-	file->ops->unload(file);
+	if (file->memory)
+		file->ops->unload(file);
+
 	if (file->fd >= 0)
 		close(file->fd);
 	free(file);
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 4a4af58..4799ed5 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -152,6 +152,7 @@ bool kmod_module_is_builtin(struct kmod_module *mod) __attribute__((nonnull(1)))
 /* libkmod-file.c */
 struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
 struct kmod_elf *kmod_file_get_elf(struct kmod_file *file) __attribute__((nonnull(1)));
+int kmod_file_load_contents(struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 void *kmod_file_get_contents(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 off_t kmod_file_get_size(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 bool kmod_file_get_direct(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
@@ -166,7 +167,7 @@ struct kmod_modversion {
 	char *symbol;
 };
 
-struct kmod_elf *kmod_elf_new(const void *memory, off_t size) _must_check_ __attribute__((nonnull(1)));
+struct kmod_elf *kmod_elf_new(const void *memory, off_t size) _must_check_;
 void kmod_elf_unref(struct kmod_elf *elf) __attribute__((nonnull(1)));
 const void *kmod_elf_get_memory(const struct kmod_elf *elf) _must_check_ __attribute__((nonnull(1)));
 int kmod_elf_get_strings(const struct kmod_elf *elf, const char *section, char ***array) _must_check_ __attribute__((nonnull(1,2,3)));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 7736b7e..f352fe1 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -917,6 +917,8 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 			goto init_finished;
 	}
 
+	kmod_file_load_contents(mod->file);
+
 	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
 		elf = kmod_file_get_elf(mod->file);
 		if (elf == NULL) {
-- 
2.40.1

Re: [PATCH 1/5] libkmod: Do not inititialize file->memory on open

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:39:57PM -0700, Lucas De Marchi wrote:
> diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> index b6a8cc9..1449c41 100644
> --- a/libkmod/libkmod-file.c
> +++ b/libkmod/libkmod-file.c
> @@ -421,6 +421,7 @@ struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
>  	if (file->elf)
>  		return file->elf;
>  
> +	kmod_file_load_contents(file);
>  	file->elf = kmod_elf_new(file->memory, file->size);
>  	return file->elf;
>  }

<-- snip -->

> @@ -491,6 +492,14 @@ error:
>  	return file;
>  }
>  
> +int kmod_file_load_contents(struct kmod_file *file)
> +{
> +	if (file->memory)
> +		return 0;
> +
> +	return file->ops->load(file);
> +}

Should be void if we are not really going to be using the return
value when we use this routine?

  Luis

[PATCH 2/5] libkmod: Extract finit_module vs init_module paths

[Lucas De Marchi]

Extract 2 functions to handle finit_module vs init_modules differences,
with a fallback from the former to the latter.

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
 libkmod/libkmod-module.c | 120 ++++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 47 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index f352fe1..6ed5ad4 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -861,6 +861,73 @@ KMOD_EXPORT int kmod_module_remove_module(struct kmod_module *mod,
 
 extern long init_module(const void *mem, unsigned long len, const char *args);
 
+static int do_finit_module(struct kmod_module *mod, unsigned int flags,
+			   const char *args)
+{
+	unsigned int kernel_flags = 0;
+	int err;
+
+	/*
+	 * Re-use ENOSYS, returned when there is no such syscall, so the
+	 * fallback to init_module applies
+	 */
+	if (!kmod_file_get_direct(mod->file))
+		return -ENOSYS;
+
+	if (flags & KMOD_INSERT_FORCE_VERMAGIC)
+		kernel_flags |= MODULE_INIT_IGNORE_VERMAGIC;
+	if (flags & KMOD_INSERT_FORCE_MODVERSION)
+		kernel_flags |= MODULE_INIT_IGNORE_MODVERSIONS;
+
+	err = finit_module(kmod_file_get_fd(mod->file), args, kernel_flags);
+	if (err < 0)
+		err = -errno;
+
+	return err;
+}
+
+static int do_init_module(struct kmod_module *mod, unsigned int flags,
+			  const char *args)
+{
+	struct kmod_elf *elf;
+	const void *mem;
+	off_t size;
+	int err;
+
+	kmod_file_load_contents(mod->file);
+
+	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
+		elf = kmod_file_get_elf(mod->file);
+		if (elf == NULL) {
+			err = -errno;
+			return err;
+		}
+
+		if (flags & KMOD_INSERT_FORCE_MODVERSION) {
+			err = kmod_elf_strip_section(elf, "__versions");
+			if (err < 0)
+				INFO(mod->ctx, "Failed to strip modversion: %s\n", strerror(-err));
+		}
+
+		if (flags & KMOD_INSERT_FORCE_VERMAGIC) {
+			err = kmod_elf_strip_vermagic(elf);
+			if (err < 0)
+				INFO(mod->ctx, "Failed to strip vermagic: %s\n", strerror(-err));
+		}
+
+		mem = kmod_elf_get_memory(elf);
+	} else {
+		mem = kmod_file_get_contents(mod->file);
+	}
+	size = kmod_file_get_size(mod->file);
+
+	err = init_module(mem, size, args);
+	if (err < 0)
+		err = -errno;
+
+	return err;
+}
+
 /**
  * kmod_module_insert_module:
  * @mod: kmod module
@@ -881,9 +948,6 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 							const char *options)
 {
 	int err;
-	const void *mem;
-	off_t size;
-	struct kmod_elf *elf;
 	const char *path;
 	const char *args = options ? options : "";
 
@@ -904,52 +968,14 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 		}
 	}
 
-	if (kmod_file_get_direct(mod->file)) {
-		unsigned int kernel_flags = 0;
-
-		if (flags & KMOD_INSERT_FORCE_VERMAGIC)
-			kernel_flags |= MODULE_INIT_IGNORE_VERMAGIC;
-		if (flags & KMOD_INSERT_FORCE_MODVERSION)
-			kernel_flags |= MODULE_INIT_IGNORE_MODVERSIONS;
-
-		err = finit_module(kmod_file_get_fd(mod->file), args, kernel_flags);
-		if (err == 0 || errno != ENOSYS)
-			goto init_finished;
-	}
-
-	kmod_file_load_contents(mod->file);
-
-	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
-		elf = kmod_file_get_elf(mod->file);
-		if (elf == NULL) {
-			err = -errno;
-			return err;
-		}
+	err = do_finit_module(mod, flags, args);
+	if (err == -ENOSYS)
+		err = do_init_module(mod, flags, args);
 
-		if (flags & KMOD_INSERT_FORCE_MODVERSION) {
-			err = kmod_elf_strip_section(elf, "__versions");
-			if (err < 0)
-				INFO(mod->ctx, "Failed to strip modversion: %s\n", strerror(-err));
-		}
-
-		if (flags & KMOD_INSERT_FORCE_VERMAGIC) {
-			err = kmod_elf_strip_vermagic(elf);
-			if (err < 0)
-				INFO(mod->ctx, "Failed to strip vermagic: %s\n", strerror(-err));
-		}
-
-		mem = kmod_elf_get_memory(elf);
-	} else {
-		mem = kmod_file_get_contents(mod->file);
-	}
-	size = kmod_file_get_size(mod->file);
+	if (err < 0)
+		INFO(mod->ctx, "Failed to insert module '%s': %s\n",
+		     path, strerror(-err));
 
-	err = init_module(mem, size, args);
-init_finished:
-	if (err < 0) {
-		err = -errno;
-		INFO(mod->ctx, "Failed to insert module '%s': %m\n", path);
-	}
 	return err;
 }
 
-- 
2.40.1

Re: [PATCH 2/5] libkmod: Extract finit_module vs init_module paths

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:39:58PM -0700, Lucas De Marchi wrote:
> Extract 2 functions to handle finit_module vs init_modules differences,
> with a fallback from the former to the latter.
> 
> Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Other than not checking for the error for kmod_file_load_contents()
looks good to me.

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

[PATCH 3/5] libkmod: Keep track of compression type

[Lucas De Marchi]

Do not only set the type as direct, but also keep track of the
compression being used. This will allow using the in-kernel compression
in future.

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
 libkmod/libkmod-file.c     | 27 +++++++++++++++------------
 libkmod/libkmod-internal.h |  7 +++++++
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 1449c41..705770a 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -58,7 +58,7 @@ struct kmod_file {
 	gzFile gzf;
 #endif
 	int fd;
-	bool direct;
+	enum kmod_file_compression_type compression;
 	off_t size;
 	void *memory;
 	const struct file_ops *ops;
@@ -376,19 +376,20 @@ static const char magic_zlib[] = {0x1f, 0x8b};
 
 static const struct comp_type {
 	size_t magic_size;
+	enum kmod_file_compression_type compression;
 	const char *magic_bytes;
 	const struct file_ops ops;
 } comp_types[] = {
 #ifdef ENABLE_ZSTD
-	{sizeof(magic_zstd), magic_zstd, {load_zstd, unload_zstd}},
+	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, {load_zstd, unload_zstd}},
 #endif
 #ifdef ENABLE_XZ
-	{sizeof(magic_xz), magic_xz, {load_xz, unload_xz}},
+	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, {load_xz, unload_xz}},
 #endif
 #ifdef ENABLE_ZLIB
-	{sizeof(magic_zlib), magic_zlib, {load_zlib, unload_zlib}},
+	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, {load_zlib, unload_zlib}},
 #endif
-	{0, NULL, {NULL, NULL}}
+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, {NULL, NULL}}
 };
 
 static int load_reg(struct kmod_file *file)
@@ -403,7 +404,7 @@ static int load_reg(struct kmod_file *file)
 			    file->fd, 0);
 	if (file->memory == MAP_FAILED)
 		return -errno;
-	file->direct = true;
+
 	return 0;
 }
 
@@ -448,7 +449,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 			magic_size_max = itr->magic_size;
 	}
 
-	file->direct = false;
 	if (magic_size_max > 0) {
 		char *buf = alloca(magic_size_max + 1);
 		ssize_t sz;
@@ -468,15 +468,18 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 		}
 
 		for (itr = comp_types; itr->ops.load != NULL; itr++) {
-			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0)
+			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
+				file->ops = &itr->ops;
+				file->compression = itr->compression;
 				break;
+			}
 		}
-		if (itr->ops.load != NULL)
-			file->ops = &itr->ops;
 	}
 
-	if (file->ops == NULL)
+	if (file->ops == NULL) {
 		file->ops = &reg_ops;
+		file->compression = KMOD_FILE_COMPRESSION_NONE;
+	}
 
 	file->ctx = ctx;
 
@@ -512,7 +515,7 @@ off_t kmod_file_get_size(const struct kmod_file *file)
 
 bool kmod_file_get_direct(const struct kmod_file *file)
 {
-	return file->direct;
+	return file->compression == KMOD_FILE_COMPRESSION_NONE;
 }
 
 int kmod_file_get_fd(const struct kmod_file *file)
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 4799ed5..7b8a158 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -61,6 +61,13 @@ struct kmod_list {
 	void *data;
 };
 
+enum kmod_file_compression_type {
+	KMOD_FILE_COMPRESSION_NONE = 0,
+	KMOD_FILE_COMPRESSION_ZSTD,
+	KMOD_FILE_COMPRESSION_XZ,
+	KMOD_FILE_COMPRESSION_ZLIB,
+};
+
 struct kmod_list *kmod_list_append(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
 struct kmod_list *kmod_list_prepend(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
 struct kmod_list *kmod_list_remove(struct kmod_list *list) _must_check_;
-- 
2.40.1

Re: [PATCH 3/5] libkmod: Keep track of compression type

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:39:59PM -0700, Lucas De Marchi wrote:
> Do not only set the type as direct, but also keep track of the
> compression being used. This will allow using the in-kernel compression
> in future.
> 
> Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

[PATCH 4/5] libkmod: Keep track of in-kernel compression support

[Lucas De Marchi]

When creating the context, read /sys/kernel/compression to check what's
the compression type supported by the kernel. This will later be used
when loading modules to check if the decompression step has to happen in
userspace or if it can be delegated to the kernel.

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
 libkmod/libkmod.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 2670f9a..103469e 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -83,6 +83,7 @@ struct kmod_ctx {
 	void *log_data;
 	const void *userdata;
 	char *dirname;
+	enum kmod_file_compression_type kernel_compression;
 	struct kmod_config *config;
 	struct hash *modules_by_name;
 	struct index_mm *indexes[_KMOD_INDEX_MODULES_SIZE];
@@ -227,6 +228,40 @@ static char *get_kernel_release(const char *dirname)
 	return p;
 }
 
+static enum kmod_file_compression_type get_kernel_compression(struct kmod_ctx *ctx)
+{
+	const char *path = "/sys/module/compression";
+	char buf[16];
+	int fd;
+	int err;
+
+	fd = open(path, O_RDONLY|O_CLOEXEC);
+	if (fd < 0) {
+		/* Not having the file is not an error: kernel may be too old */
+		DBG(ctx, "could not open '%s' for reading: %m\n", path);
+		return KMOD_FILE_COMPRESSION_NONE;
+	}
+
+	err = read_str_safe(fd, buf, sizeof(buf));
+	close(fd);
+	if (err < 0) {
+		ERR(ctx, "could not read from '%s': %s\n",
+		    path, strerror(-err));
+		return KMOD_FILE_COMPRESSION_NONE;
+	}
+
+	if (streq(buf, "zstd\n"))
+		return KMOD_FILE_COMPRESSION_ZSTD;
+	else if (streq(buf, "xz\n"))
+		return KMOD_FILE_COMPRESSION_XZ;
+	else if (streq(buf, "gzip\n"))
+		return KMOD_FILE_COMPRESSION_ZLIB;
+
+	ERR(ctx, "unknown kernel compression %s", buf);
+
+	return KMOD_FILE_COMPRESSION_NONE;
+}
+
 /**
  * kmod_new:
  * @dirname: what to consider as linux module's directory, if NULL
@@ -272,6 +307,8 @@ KMOD_EXPORT struct kmod_ctx *kmod_new(const char *dirname,
 	if (env != NULL)
 		kmod_set_log_priority(ctx, log_priority(env));
 
+	ctx->kernel_compression = get_kernel_compression(ctx);
+
 	if (config_paths == NULL)
 		config_paths = default_config_paths;
 	err = kmod_config_new(ctx, &ctx->config, config_paths);
-- 
2.40.1

Re: [PATCH 4/5] libkmod: Keep track of in-kernel compression support

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:40:00PM -0700, Lucas De Marchi wrote:
> When creating the context, read /sys/kernel/compression to check what's
> the compression type supported by the kernel. This will later be used
> when loading modules to check if the decompression step has to happen in
> userspace or if it can be delegated to the kernel.
> 
> Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 4/5] libkmod: Keep track of in-kernel compression support

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:40:00PM -0700, Lucas De Marchi wrote:
> When creating the context, read /sys/kernel/compression to check what's
> the compression type supported by the kernel. This will later be used
> when loading modules to check if the decompression step has to happen in
> userspace or if it can be delegated to the kernel.
> 
> Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

[PATCH 5/5] libkmod: Use kernel decompression when available

[Lucas De Marchi]

With the recent changes to bypass loading the file it's possible to
reduce the work in userspace and delegating it to the kernel. Without
any compression to illustrate:

Before:
	read(3, "\177ELF\2\1", 6)               = 6
	lseek(3, 0, SEEK_SET)                   = 0
	newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=238592, ...}, AT_EMPTY_PATH) = 0
	mmap(NULL, 238592, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7fd85cbd1000
	finit_module(3, "", 0)                  = 0
	munmap(0x7fd85cbd1000, 238592)          = 0
	close(3)                                = 0

After:
	read(3, "\177ELF\2\1", 6)               = 6
	lseek(3, 0, SEEK_SET)                   = 0
	finit_module(3, "", 0)                  = 0
	close(3)                                = 0

When using kernel compression now it's also possible to direct libkmod
to take the finit_module() path, avoiding the decompression in userspace
and just delegating it to the kernel.

Before:
	read(3, "(\265/\375\244\0", 6)          = 6
	lseek(3, 0, SEEK_SET)                   = 0
	read(3, "(\265/\375\244", 5)            = 5
	mmap(NULL, 135168, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f3fa431e000
	read(3, "\0\244\3\0\\y\6", 7)           = 7
	mmap(NULL, 372736, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f3fa414f000
	brk(0x55944c6a1000)                     = 0x55944c6a1000
	read(3, "\356|\6G\27U\20 \312\260s\211\335\333\263\326\330\336\273O\211\356\306K\360Z\341\374U6\342\221"..., 53038) = 53038
	mremap(0x7f3fa431e000, 135168, 266240, MREMAP_MAYMOVE) = 0x7f3fa410e000
	read(3, ",;\3\nqf\311\362\325\211\7\341\375A\355\221\371L\\\5\7\375 \32\246<(\258=K\304"..., 20851) = 20851
	mremap(0x7f3fa410e000, 266240, 397312, MREMAP_MAYMOVE) = 0x7f3fa40ad000
	read(3, ")\36\250\213", 4)              = 4
	read(3, "", 4)                          = 0
	munmap(0x7f3fa414f000, 372736)          = 0
	init_module(0x7f3fa40ad010, 238592, "") = 0
	munmap(0x7f3fa40ad000, 397312)          = 0
	close(3)                                = 0

After:
	read(3, "(\265/\375\244P", 6)           = 6
	lseek(3, 0, SEEK_SET)                   = 0
	finit_module(3, "", 0x4 /* MODULE_INIT_??? */) = 0
	close(3)                                = 0

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
 libkmod/libkmod-file.c     |  4 ++--
 libkmod/libkmod-internal.h |  3 ++-
 libkmod/libkmod-module.c   | 17 +++++++++++++----
 libkmod/libkmod.c          |  5 +++++
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 705770a..ffdda92 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -513,9 +513,9 @@ off_t kmod_file_get_size(const struct kmod_file *file)
 	return file->size;
 }
 
-bool kmod_file_get_direct(const struct kmod_file *file)
+enum kmod_file_compression_type kmod_file_get_compression(const struct kmod_file *file)
 {
-	return file->compression == KMOD_FILE_COMPRESSION_NONE;
+	return file->compression;
 }
 
 int kmod_file_get_fd(const struct kmod_file *file)
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 7b8a158..edb4eac 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -112,6 +112,7 @@ void kmod_pool_add_module(struct kmod_ctx *ctx, struct kmod_module *mod, const c
 void kmod_pool_del_module(struct kmod_ctx *ctx, struct kmod_module *mod, const char *key) __attribute__((nonnull(1, 2, 3)));
 
 const struct kmod_config *kmod_get_config(const struct kmod_ctx *ctx) __attribute__((nonnull(1)));
+enum kmod_file_compression_type kmod_get_kernel_compression(const struct kmod_ctx *ctx) __attribute__((nonnull(1)));
 
 /* libkmod-config.c */
 struct kmod_config_path {
@@ -162,7 +163,7 @@ struct kmod_elf *kmod_file_get_elf(struct kmod_file *file) __attribute__((nonnul
 int kmod_file_load_contents(struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 void *kmod_file_get_contents(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 off_t kmod_file_get_size(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
-bool kmod_file_get_direct(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
+enum kmod_file_compression_type kmod_file_get_compression(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 int kmod_file_get_fd(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 void kmod_file_unref(struct kmod_file *file) __attribute__((nonnull(1)));
 
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 6ed5ad4..a9e1be8 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
 static int do_finit_module(struct kmod_module *mod, unsigned int flags,
 			   const char *args)
 {
+	enum kmod_file_compression_type compression, kernel_compression;
 	unsigned int kernel_flags = 0;
 	int err;
 
 	/*
-	 * Re-use ENOSYS, returned when there is no such syscall, so the
-	 * fallback to init_module applies
+	 * When module is not compressed or its compression type matches the
+	 * one in use by the kernel, there is no need to read the file
+	 * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
+	 * as when finit_module() is not supported.
 	 */
-	if (!kmod_file_get_direct(mod->file))
-		return -ENOSYS;
+	compression = kmod_file_get_compression(mod->file);
+	kernel_compression = kmod_get_kernel_compression(mod->ctx);
+	if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
+	      compression == kernel_compression))
+		return ENOSYS;
+
+	if (compression != KMOD_FILE_COMPRESSION_NONE)
+		kernel_flags |= MODULE_INIT_COMPRESSED_FILE;
 
 	if (flags & KMOD_INSERT_FORCE_VERMAGIC)
 		kernel_flags |= MODULE_INIT_IGNORE_VERMAGIC;
diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 103469e..1b8773c 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -1016,3 +1016,8 @@ const struct kmod_config *kmod_get_config(const struct kmod_ctx *ctx)
 {
 	return ctx->config;
 }
+
+enum kmod_file_compression_type kmod_get_kernel_compression(const struct kmod_ctx *ctx)
+{
+	return ctx->kernel_compression;
+}
-- 
2.40.1

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Luis Chamberlain]

On Thu, Jun 01, 2023 at 03:40:01PM -0700, Lucas De Marchi wrote:
> With the recent changes to bypass loading the file it's possible to
> reduce the work in userspace and delegating it to the kernel. Without
> any compression to illustrate:
> 
> Before:
> 	read(3, "\177ELF\2\1", 6)               = 6
> 	lseek(3, 0, SEEK_SET)                   = 0
> 	newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=238592, ...}, AT_EMPTY_PATH) = 0
> 	mmap(NULL, 238592, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7fd85cbd1000
> 	finit_module(3, "", 0)                  = 0
> 	munmap(0x7fd85cbd1000, 238592)          = 0
> 	close(3)                                = 0
> 
> After:
> 	read(3, "\177ELF\2\1", 6)               = 6
> 	lseek(3, 0, SEEK_SET)                   = 0
> 	finit_module(3, "", 0)                  = 0
> 	close(3)                                = 0

It's not clear to me how the patches did the above, in particular
avoiding the newfstatat() for the non-decompression use case.

  Luis

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Lucas De Marchi]

On Tue, Jun 06, 2023 at 11:38:35AM -0700, Luis Chamberlain wrote:
>On Thu, Jun 01, 2023 at 03:40:01PM -0700, Lucas De Marchi wrote:
>> With the recent changes to bypass loading the file it's possible to
>> reduce the work in userspace and delegating it to the kernel. Without
>> any compression to illustrate:
>>
>> Before:
>> 	read(3, "\177ELF\2\1", 6)               = 6
>> 	lseek(3, 0, SEEK_SET)                   = 0
>> 	newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=238592, ...}, AT_EMPTY_PATH) = 0
>> 	mmap(NULL, 238592, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7fd85cbd1000
>> 	finit_module(3, "", 0)                  = 0
>> 	munmap(0x7fd85cbd1000, 238592)          = 0
>> 	close(3)                                = 0
>>
>> After:
>> 	read(3, "\177ELF\2\1", 6)               = 6
>> 	lseek(3, 0, SEEK_SET)                   = 0
>> 	finit_module(3, "", 0)                  = 0
>> 	close(3)                                = 0
>
>It's not clear to me how the patches did the above, in particular
>avoiding the newfstatat() for the non-decompression use case.

because now we are not taking the path to mmap the file anymore.
 From load_reg():

	if (fstat(file->fd, &st) < 0)
		return -errno;

	file->size = st.st_size;
	file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
			    file->fd, 0);

from STAT(2):

	The  underlying  system  call  employed  by the glibc fstatat() wrapper
	function is actually called  fstatat64()  or,  on  some  architectures,
	newfstatat().

With load_reg() not being called anymore, these 2 syscalls are gone.

We still read the header (first 6 bytes as per above), to make sure we select
the right handler for the compression method. In the case above it was uncompressed
("\177ELF\2\1"), so we lseek() and give it to the kernel. If it was
a compression algo matching the one in use by the kernel, we would just add
the compression flag and do the same thing.
If it was a different compression type, then we'd fallback to the
previous handling with mmap() + decompression in usersapce +
init_module().


Lucas De Marchi

>
>  Luis

Re: [PATCH 0/5] libkmod: Use kernel decompression support

[Lucas De Marchi]

On Thu, Jun 01, 2023 at 03:39:56PM -0700, Lucas De Marchi wrote:
>When kernel is built with CONFIG_MODULE_DECOMPRESS=y, it can handle 1
>algorithm for module decompression with finit_module(). When that
>algorithm matches the one used in the module we are trying to load,
>prefer using the in-kernel decompression.  This way the kernel can also
>apply any additional security measures based on where the module is
>coming from.
>
>In future, if the kernel supports more algorithms at a time, libkmod
>could even be compiled without them and just let the kernel handle it.
>Since it's likely a distro kernel supports all of them, that would
>seem a good thing to do (on the other hand, tools like modinfo and
>depmod wouldn't be able read the module information).
>
>For zstd, this needs the following fix on the kernel side:
>https://lore.kernel.org/linux-modules/ZHkQNQK5zrzo4Cq2@bombadil.infradead.org/
>
>Lucas De Marchi (5):
>  libkmod: Do not inititialize file->memory on open
>  libkmod: Extract finit_module vs init_module paths
>  libkmod: Keep track of compression type
>  libkmod: Keep track of in-kernel compression support
>  libkmod: Use kernel decompression when available

thank you all for the reviews. I just pushed all the patches here.

Lucas De Marchi

>
> libkmod/libkmod-elf.c      |   5 ++
> libkmod/libkmod-file.c     |  46 +++++++++-----
> libkmod/libkmod-internal.h |  13 +++-
> libkmod/libkmod-module.c   | 127 ++++++++++++++++++++++++-------------
> libkmod/libkmod.c          |  42 ++++++++++++
> 5 files changed, 170 insertions(+), 63 deletions(-)
>
>-- 
>2.40.1
>

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Emil Velikov]

Greetings Lucas, list,

I've pulled the email off lore.kernel.org manually (haven't played
with lei yet), so chances are the following will be "slightly"
malformed.

Above all - hell yeah, thank you for wiring this neat functionality.

Out of curiosity: have you done any measurements - CPU cycles, memory
or other - how well the kernel decompression performs vs the userspace
one?

That said, I may have spotted a small bug, namely:

> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
>  static int do_finit_module(struct kmod_module *mod, unsigned int flags,
>     const char *args)
>  {
> + enum kmod_file_compression_type compression, kernel_compression;
>  unsigned int kernel_flags = 0;
>  int err;
>
>  /*
> - * Re-use ENOSYS, returned when there is no such syscall, so the
> - * fallback to init_module applies
> + * When module is not compressed or its compression type matches the
> + * one in use by the kernel, there is no need to read the file
> + * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
> + * as when finit_module() is not supported.
>  */
> - if (!kmod_file_get_direct(mod->file))
> - return -ENOSYS;
> + compression = kmod_file_get_compression(mod->file);
> + kernel_compression = kmod_get_kernel_compression(mod->ctx);
> + if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
> +       compression == kernel_compression))
> + return ENOSYS;
> +

Old code returns negative -ENOSYS (negative), the new one a positive
ENOSYS. Where the fallback, mentioned in the comment just above,
triggers on the former negative ENOSYS.

Mind you I'm still sipping coffee, so chances are I'm missing something here.

Thanks again and HTH o/
Emil

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Emil Velikov]

On Thu, 15 Jun 2023 at 10:36, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Greetings Lucas, list,
>
> I've pulled the email off lore.kernel.org manually (haven't played
> with lei yet), so chances are the following will be "slightly"
> malformed.
>
> Above all - hell yeah, thank you for wiring this neat functionality.
>
> Out of curiosity: have you done any measurements - CPU cycles, memory
> or other - how well the kernel decompression performs vs the userspace
> one?
>
> That said, I may have spotted a small bug, namely:
>
> > --- a/libkmod/libkmod-module.c
> > +++ b/libkmod/libkmod-module.c
> > @@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
> >  static int do_finit_module(struct kmod_module *mod, unsigned int flags,
> >     const char *args)
> >  {
> > + enum kmod_file_compression_type compression, kernel_compression;
> >  unsigned int kernel_flags = 0;
> >  int err;
> >
> >  /*
> > - * Re-use ENOSYS, returned when there is no such syscall, so the
> > - * fallback to init_module applies
> > + * When module is not compressed or its compression type matches the
> > + * one in use by the kernel, there is no need to read the file
> > + * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
> > + * as when finit_module() is not supported.
> >  */
> > - if (!kmod_file_get_direct(mod->file))
> > - return -ENOSYS;
> > + compression = kmod_file_get_compression(mod->file);
> > + kernel_compression = kmod_get_kernel_compression(mod->ctx);
> > + if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
> > +       compression == kernel_compression))
> > + return ENOSYS;
> > +
>
> Old code returns negative -ENOSYS (negative), the new one a positive
> ENOSYS. Where the fallback, mentioned in the comment just above,
> triggers on the former negative ENOSYS.
>
> Mind you I'm still sipping coffee, so chances are I'm missing something here.
>
> Thanks again and HTH o/
> Emil

Somewhat related:

Would it make sense to read /sys/module/compression if it contained
multiple lines - one for each supported compression. This way, kmod
will just work when the kernel is updated to advertise them all.

There is about 0.000001% chance that changing the format of the sysfs
file might cause regression, which can be looked into if an issue.
After all the sysfs entry is just 1 year old and is undocumented
(cough) so nobody should be using it, right :-P

I'm really tempted to send a patch tweaking the sysfs file and adding
documentation. Please let me know if you think that's a bad idea, or
you have one already queued.

Thanks
Emil

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Lucas De Marchi]

On Thu, Jun 15, 2023 at 11:09:59AM +0100, Emil Velikov wrote:
>On Thu, 15 Jun 2023 at 10:36, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>
>> Greetings Lucas, list,
>>
>> I've pulled the email off lore.kernel.org manually (haven't played
>> with lei yet), so chances are the following will be "slightly"
>> malformed.
>>
>> Above all - hell yeah, thank you for wiring this neat functionality.
>>
>> Out of curiosity: have you done any measurements - CPU cycles, memory
>> or other - how well the kernel decompression performs vs the userspace
>> one?
>>
>> That said, I may have spotted a small bug, namely:
>>
>> > --- a/libkmod/libkmod-module.c
>> > +++ b/libkmod/libkmod-module.c
>> > @@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
>> >  static int do_finit_module(struct kmod_module *mod, unsigned int flags,
>> >     const char *args)
>> >  {
>> > + enum kmod_file_compression_type compression, kernel_compression;
>> >  unsigned int kernel_flags = 0;
>> >  int err;
>> >
>> >  /*
>> > - * Re-use ENOSYS, returned when there is no such syscall, so the
>> > - * fallback to init_module applies
>> > + * When module is not compressed or its compression type matches the
>> > + * one in use by the kernel, there is no need to read the file
>> > + * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
>> > + * as when finit_module() is not supported.
>> >  */
>> > - if (!kmod_file_get_direct(mod->file))
>> > - return -ENOSYS;
>> > + compression = kmod_file_get_compression(mod->file);
>> > + kernel_compression = kmod_get_kernel_compression(mod->ctx);
>> > + if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
>> > +       compression == kernel_compression))
>> > + return ENOSYS;
>> > +
>>
>> Old code returns negative -ENOSYS (negative), the new one a positive
>> ENOSYS. Where the fallback, mentioned in the comment just above,
>> triggers on the former negative ENOSYS.
>>
>> Mind you I'm still sipping coffee, so chances are I'm missing something here.
>>
>> Thanks again and HTH o/
>> Emil
>
>Somewhat related:
>
>Would it make sense to read /sys/module/compression if it contained
>multiple lines - one for each supported compression. This way, kmod
>will just work when the kernel is updated to advertise them all.

I think it makes sense to report all the decompressions supported by the
kernel. Looking to the commit messages where this was added, it seems
it was decided to add just 1 for simplicity.

>
>There is about 0.000001% chance that changing the format of the sysfs
>file might cause regression, which can be looked into if an issue.
>After all the sysfs entry is just 1 year old and is undocumented
>(cough) so nobody should be using it, right :-P

However I don't think userspace should go ahead and assume the kernel
interface will be updated in a certain way. Particularly because if the
kernel decides it'd be better to do it in different way and is not
compatible with what userspace assumed, then there would be a
regression.  +Luis on if there is intention to update the decompression
list.

>
>I'm really tempted to send a patch tweaking the sysfs file and adding
>documentation. Please let me know if you think that's a bad idea, or
>you have one already queued.

at least the documentation, yes... would be good to have it.

Lucas De Marchi

>
>Thanks
>Emil

Re: [PATCH 5/5] libkmod: Use kernel decompression when available

[Lucas De Marchi]

On Thu, Jun 15, 2023 at 10:36:31AM +0100, Emil Velikov wrote:
>Greetings Lucas, list,
>
>I've pulled the email off lore.kernel.org manually (haven't played
>with lei yet), so chances are the following will be "slightly"
>malformed.

One easy easy if you want to pick single patch series is to use b4

b4 mbox 20230601224001.23397-1-lucas.de.marchi@gmail.com
neomut -f ./20230601224001.23397-1-lucas.de.marchi@gmail.com.mbx

or instead of neomutt, import the mbox to your favorite email client.

>
>Above all - hell yeah, thank you for wiring this neat functionality.
>
>Out of curiosity: have you done any measurements - CPU cycles, memory
>or other - how well the kernel decompression performs vs the userspace
>one?

no, I didn't.  I think one important aspect is when paired with the
patch to the kernel that deduplicates the module loading. With
compressed modules, that would be irrelevant as libkmod would go the
init_module() path rather than finit_module().

A reasonable test would be to create a VM with a bunch of CPUs (so we
get many parallel requests or the same modules) and then measure
a) 6.4-rcX
b) 6.4-rcX + that kernel patch
c) 6.4-rcX + that kernel patch + this patch series


>
>That said, I may have spotted a small bug, namely:
>
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
>>  static int do_finit_module(struct kmod_module *mod, unsigned int flags,
>>     const char *args)
>>  {
>> + enum kmod_file_compression_type compression, kernel_compression;
>>  unsigned int kernel_flags = 0;
>>  int err;
>>
>>  /*
>> - * Re-use ENOSYS, returned when there is no such syscall, so the
>> - * fallback to init_module applies
>> + * When module is not compressed or its compression type matches the
>> + * one in use by the kernel, there is no need to read the file
>> + * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
>> + * as when finit_module() is not supported.
>>  */
>> - if (!kmod_file_get_direct(mod->file))
>> - return -ENOSYS;
>> + compression = kmod_file_get_compression(mod->file);
>> + kernel_compression = kmod_get_kernel_compression(mod->ctx);
>> + if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
>> +       compression == kernel_compression))
>> + return ENOSYS;
>> +
>
>Old code returns negative -ENOSYS (negative), the new one a positive
>ENOSYS. Where the fallback, mentioned in the comment just above,
>triggers on the former negative ENOSYS.
>
>Mind you I'm still sipping coffee, so chances are I'm missing something here.

no, apparently I didn't have enough coffee when I wrote this.
I'll fix this up on next version.

thanks
Lucas De Marchi

>
>Thanks again and HTH o/
>Emil