[PATCH kmod 00/13] Load compressed modules with compression-less kmod

[PATCH kmod 00/13] Load compressed modules with compression-less kmod

[Emil Velikov via B4 Relay]

While I was digging into a dkms bug, I've noticed that kmod does not
fallback to the normal init_module() code path when the in-kernel
decompression fails.

As I looked closer, I realised that the current module compression
detection relies on kmod being build with zstd, xz, zlib ... Even in the
case where we rely on the kernel module itself to decompress - aka we
don't use the external libraries all together.

By building kmod without zstd and friends, we shave some space off the
initrd which helps with boot time.

Majority of this series handles the latter and fixes a few related bugs
along the way. The last commits is somewhat questionable commit and
addresses the former issue.

Happy to hear your feedback,
Emil

---
Emil Velikov (13):
      libkmod: use a dup()'d fd for zlib
      libkmod: keep gzFile gzf local to load_zlib()
      libkmod: remove kmod_file::{zstd,xz}_used flags
      libkmod: clear file->memory if map fails
      libkmod: nuke struct file_ops
      libkmod: propagate {zstd,xz,zlib}_load errors
      libkmod: move kmod_file_load_contents as applicable
      libkmod: always detect the module compression
      libkmod: swap alloca usage for a few assert_cc
      libkmod: tidy-up kmod_file_open()
      libkmod: move load_reg() further up
      libkmod: keep KMOD_FILE_COMPRESSION_NONE/load_reg in comp_types
      libkmod: always fallback to do_init_module()

 libkmod/libkmod-file.c     | 206 ++++++++++++++++++---------------------------
 libkmod/libkmod-internal.h |   2 +-
 libkmod/libkmod-module.c   |   8 +-
 3 files changed, 90 insertions(+), 126 deletions(-)
---
base-commit: b29704cd448aaa455dba4e656fc0f0d3c686df3f
change-id: 20240212-decompression-fixes-56371eeb707d

Best regards,
-- 
Emil Velikov <emil.l.velikov@gmail.com>

[PATCH kmod 01/13] libkmod: use a dup()'d fd for zlib

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

The gzdopen() API used, takes ownership of the fd. To make that more
explicit we clear it (-1) as applicable.

Yet again, kmod has explicit API to return the fd to the user - which
currently is used solely when uncompressed, so we're safe.

Regardless - simply duplicate the fd locally and use that with zlib.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index b138e7e..b97062b 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -317,12 +317,18 @@ static int load_zlib(struct kmod_file *file)
 	int err = 0;
 	off_t did = 0, total = 0;
 	_cleanup_free_ unsigned char *p = NULL;
+	int gzfd;
 
 	errno = 0;
-	file->gzf = gzdopen(file->fd, "rb");
-	if (file->gzf == NULL)
+	gzfd = fcntl(file->fd, F_DUPFD_CLOEXEC, 3);
+	if (gzfd < 0)
+		return -errno;
+
+	file->gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
+	if (file->gzf == NULL) {
+		close(gzfd);
 		return -errno;
-	file->fd = -1; /* now owned by gzf due gzdopen() */
+	}
 
 	for (;;) {
 		int r;
@@ -359,7 +365,7 @@ static int load_zlib(struct kmod_file *file)
 	return 0;
 
 error:
-	gzclose(file->gzf);
+	gzclose(file->gzf); /* closes the gzfd */
 	return err;
 }
 
@@ -368,7 +374,7 @@ static void unload_zlib(struct kmod_file *file)
 	if (file->gzf == NULL)
 		return;
 	free(file->memory);
-	gzclose(file->gzf); /* closes file->fd */
+	gzclose(file->gzf);
 }
 
 static const char magic_zlib[] = {0x1f, 0x8b};
@@ -535,7 +541,6 @@ void kmod_file_unref(struct kmod_file *file)
 	if (file->memory)
 		file->ops->unload(file);
 
-	if (file->fd >= 0)
-		close(file->fd);
+	close(file->fd);
 	free(file);
 }

-- 
2.43.0

[PATCH kmod 02/13] libkmod: keep gzFile gzf local to load_zlib()

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

There is no need to keep the root gzFile context open for the whole
duration. Once we've copied the decompressed module to file->memory we
can close the handle.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index b97062b..9a014ea 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -53,9 +53,6 @@ struct kmod_file {
 #endif
 #ifdef ENABLE_XZ
 	bool xz_used;
-#endif
-#ifdef ENABLE_ZLIB
-	gzFile gzf;
 #endif
 	int fd;
 	enum kmod_file_compression_type compression;
@@ -317,6 +314,7 @@ static int load_zlib(struct kmod_file *file)
 	int err = 0;
 	off_t did = 0, total = 0;
 	_cleanup_free_ unsigned char *p = NULL;
+	gzFile gzf;
 	int gzfd;
 
 	errno = 0;
@@ -324,8 +322,8 @@ static int load_zlib(struct kmod_file *file)
 	if (gzfd < 0)
 		return -errno;
 
-	file->gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
-	if (file->gzf == NULL) {
+	gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
+	if (gzf == NULL) {
 		close(gzfd);
 		return -errno;
 	}
@@ -343,12 +341,12 @@ static int load_zlib(struct kmod_file *file)
 			p = tmp;
 		}
 
-		r = gzread(file->gzf, p + did, total - did);
+		r = gzread(gzf, p + did, total - did);
 		if (r == 0)
 			break;
 		else if (r < 0) {
 			int gzerr;
-			const char *gz_errmsg = gzerror(file->gzf, &gzerr);
+			const char *gz_errmsg = gzerror(gzf, &gzerr);
 
 			ERR(file->ctx, "gzip: %s\n", gz_errmsg);
 
@@ -362,19 +360,17 @@ static int load_zlib(struct kmod_file *file)
 	file->memory = p;
 	file->size = did;
 	p = NULL;
+	gzclose(gzf);
 	return 0;
 
 error:
-	gzclose(file->gzf); /* closes the gzfd */
+	gzclose(gzf); /* closes the gzfd */
 	return err;
 }
 
 static void unload_zlib(struct kmod_file *file)
 {
-	if (file->gzf == NULL)
-		return;
 	free(file->memory);
-	gzclose(file->gzf);
 }
 
 static const char magic_zlib[] = {0x1f, 0x8b};

-- 
2.43.0

[PATCH kmod 03/13] libkmod: remove kmod_file::{zstd,xz}_used flags

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

These are used to protect a free(file->memory), within their respective
unload functions. Where the sole caller of the unload function already
does a NULL check prior.

Even so, free(NULL) is guaranteed to be safe by the standard.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 9a014ea..abd4723 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -48,12 +48,6 @@ struct file_ops {
 };
 
 struct kmod_file {
-#ifdef ENABLE_ZSTD
-	bool zstd_used;
-#endif
-#ifdef ENABLE_XZ
-	bool xz_used;
-#endif
 	int fd;
 	enum kmod_file_compression_type compression;
 	off_t size;
@@ -176,7 +170,6 @@ static int load_zstd(struct kmod_file *file)
 
 	ZSTD_freeDStream(dstr);
 	free((void *)zst_inb.src);
-	file->zstd_used = true;
 	file->memory = zst_outb.dst;
 	file->size = zst_outb.pos;
 	return 0;
@@ -190,8 +183,6 @@ out:
 
 static void unload_zstd(struct kmod_file *file)
 {
-	if (!file->zstd_used)
-		return;
 	free(file->memory);
 }
 
@@ -269,7 +260,6 @@ static int xz_uncompress(lzma_stream *strm, struct kmod_file *file)
 			goto out;
 		}
 	}
-	file->xz_used = true;
 	file->memory = p;
 	file->size = total;
 	return 0;
@@ -299,8 +289,6 @@ static int load_xz(struct kmod_file *file)
 
 static void unload_xz(struct kmod_file *file)
 {
-	if (!file->xz_used)
-		return;
 	free(file->memory);
 }
 

-- 
2.43.0

[PATCH kmod 04/13] libkmod: clear file->memory if map fails

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

On mmap failure file->memory is set to -1, which we'll happily pass down
to munmap later on.

More importantly, since we do a NULL check in kmod_file_load_contents()
we will exit the function without (re)attempting the load again.

Since we ignore the return code for the load function(s), one can end up
calling kmod_elf_get_memory() and feed that -1 into init_module.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index abd4723..b408aed 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -392,8 +392,10 @@ static int load_reg(struct kmod_file *file)
 	file->size = st.st_size;
 	file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
 			    file->fd, 0);
-	if (file->memory == MAP_FAILED)
+	if (file->memory == MAP_FAILED) {
+		file->memory = NULL;
 		return -errno;
+	}
 
 	return 0;
 }

-- 
2.43.0

[PATCH kmod 05/13] libkmod: nuke struct file_ops

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

With the previous commits, we removed the need for a distinct unload
callback.

So nuke the struct all together and only use/keep the load one around.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 62 +++++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 44 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index b408aed..8a0336f 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -41,18 +41,12 @@
 #include "libkmod.h"
 #include "libkmod-internal.h"
 
-struct kmod_file;
-struct file_ops {
-	int (*load)(struct kmod_file *file);
-	void (*unload)(struct kmod_file *file);
-};
-
 struct kmod_file {
 	int fd;
 	enum kmod_file_compression_type compression;
 	off_t size;
 	void *memory;
-	const struct file_ops *ops;
+	int (*load)(struct kmod_file *file);
 	const struct kmod_ctx *ctx;
 	struct kmod_elf *elf;
 };
@@ -181,11 +175,6 @@ out:
 	return ret;
 }
 
-static void unload_zstd(struct kmod_file *file)
-{
-	free(file->memory);
-}
-
 static const char magic_zstd[] = {0x28, 0xB5, 0x2F, 0xFD};
 #endif
 
@@ -287,11 +276,6 @@ static int load_xz(struct kmod_file *file)
 	return ret;
 }
 
-static void unload_xz(struct kmod_file *file)
-{
-	free(file->memory);
-}
-
 static const char magic_xz[] = {0xfd, '7', 'z', 'X', 'Z', 0};
 #endif
 
@@ -356,11 +340,6 @@ error:
 	return err;
 }
 
-static void unload_zlib(struct kmod_file *file)
-{
-	free(file->memory);
-}
-
 static const char magic_zlib[] = {0x1f, 0x8b};
 #endif
 
@@ -368,18 +347,18 @@ static const struct comp_type {
 	size_t magic_size;
 	enum kmod_file_compression_type compression;
 	const char *magic_bytes;
-	const struct file_ops ops;
+	int (*load)(struct kmod_file *file);
 } comp_types[] = {
 #ifdef ENABLE_ZSTD
-	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, {load_zstd, unload_zstd}},
+	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
 #endif
 #ifdef ENABLE_XZ
-	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, {load_xz, unload_xz}},
+	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
 #endif
 #ifdef ENABLE_ZLIB
-	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, {load_zlib, unload_zlib}},
+	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
 #endif
-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, {NULL, NULL}}
+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
 };
 
 static int load_reg(struct kmod_file *file)
@@ -400,15 +379,6 @@ static int load_reg(struct kmod_file *file)
 	return 0;
 }
 
-static void unload_reg(struct kmod_file *file)
-{
-	munmap(file->memory, file->size);
-}
-
-static const struct file_ops reg_ops = {
-	load_reg, unload_reg
-};
-
 struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
 {
 	if (file->elf)
@@ -436,7 +406,7 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 		goto error;
 	}
 
-	for (itr = comp_types; itr->ops.load != NULL; itr++) {
+	for (itr = comp_types; itr->load != NULL; itr++) {
 		if (magic_size_max < itr->magic_size)
 			magic_size_max = itr->magic_size;
 	}
@@ -459,17 +429,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 			goto error;
 		}
 
-		for (itr = comp_types; itr->ops.load != NULL; itr++) {
+		for (itr = comp_types; itr->load != NULL; itr++) {
 			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
-				file->ops = &itr->ops;
+				file->load = itr->load;
 				file->compression = itr->compression;
 				break;
 			}
 		}
 	}
 
-	if (file->ops == NULL) {
-		file->ops = &reg_ops;
+	if (file->load == NULL) {
+		file->load = load_reg;
 		file->compression = KMOD_FILE_COMPRESSION_NONE;
 	}
 
@@ -496,7 +466,7 @@ void kmod_file_load_contents(struct kmod_file *file)
 		return;
 
 	/*  The load functions already log possible errors. */
-	file->ops->load(file);
+	file->load(file);
 }
 
 void *kmod_file_get_contents(const struct kmod_file *file)
@@ -524,8 +494,12 @@ void kmod_file_unref(struct kmod_file *file)
 	if (file->elf)
 		kmod_elf_unref(file->elf);
 
-	if (file->memory)
-		file->ops->unload(file);
+	if (file->compression == KMOD_FILE_COMPRESSION_NONE) {
+		if (file->memory)
+			munmap(file->memory, file->size);
+	} else {
+		free(file->memory);
+	}
 
 	close(file->fd);
 	free(file);

-- 
2.43.0

[PATCH kmod 06/13] libkmod: propagate {zstd,xz,zlib}_load errors

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Propagate any errors during decompression further up the call stack.
Without this we could easily pass NULL as mem to init_module(2).

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c     | 15 +++++++++++----
 libkmod/libkmod-internal.h |  2 +-
 libkmod/libkmod-module.c   |  4 +++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 8a0336f..3a79464 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -381,10 +381,17 @@ static int load_reg(struct kmod_file *file)
 
 struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
 {
+	int err;
+
 	if (file->elf)
 		return file->elf;
 
-	kmod_file_load_contents(file);
+	err = kmod_file_load_contents(file);
+	if (err) {
+		errno = err;
+		return NULL;
+	}
+
 	file->elf = kmod_elf_new(file->memory, file->size);
 	return file->elf;
 }
@@ -460,13 +467,13 @@ error:
 /*
  *  Callers should just check file->memory got updated
  */
-void kmod_file_load_contents(struct kmod_file *file)
+int kmod_file_load_contents(struct kmod_file *file)
 {
 	if (file->memory)
-		return;
+		return 0;
 
 	/*  The load functions already log possible errors. */
-	file->load(file);
+	return file->load(file);
 }
 
 void *kmod_file_get_contents(const struct kmod_file *file)
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 26a7e28..3bc6e11 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -160,7 +160,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)));
-void kmod_file_load_contents(struct kmod_file *file) __attribute__((nonnull(1)));
+int kmod_file_load_contents(struct kmod_file *file) __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)));
 enum kmod_file_compression_type kmod_file_get_compression(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 585da41..1e43482 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -903,7 +903,9 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
 	off_t size;
 	int err;
 
-	kmod_file_load_contents(mod->file);
+	err = kmod_file_load_contents(mod->file);
+	if (err)
+		return err;
 
 	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
 		elf = kmod_file_get_elf(mod->file);

-- 
2.43.0

[PATCH kmod 07/13] libkmod: move kmod_file_load_contents as applicable

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

When dealing with an elf, we don't know or care about loading the file.
The kmod_elf subsystem/API will deal with the required parts itself.

Which in this case, already calls kmod_file_load_contents() as
applicable.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-module.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 1e43482..d309948 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -903,10 +903,6 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
 	off_t size;
 	int err;
 
-	err = kmod_file_load_contents(mod->file);
-	if (err)
-		return err;
-
 	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
 		elf = kmod_file_get_elf(mod->file);
 		if (elf == NULL) {
@@ -928,6 +924,10 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
 
 		mem = kmod_elf_get_memory(elf);
 	} else {
+		err = kmod_file_load_contents(mod->file);
+		if (err)
+			return err;
+
 		mem = kmod_file_get_contents(mod->file);
 	}
 	size = kmod_file_get_size(mod->file);

-- 
2.43.0

[PATCH kmod 08/13] libkmod: always detect the module compression

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Currently, when built w/o given compression we'll incorrectly report a
"compression_none".

As we reach do_finit_module(), we'll naively assume that the kernel can
handle the compressed module, yet omit the MODULE_INIT_COMPRESSED_FILE
flag.

As result the kernel will barf at us, do_finit_module will fail with non
-ENOSYS and we won't end in the do_init_module codepath (which will also
fail).

In other words: with this change, you can build kmod without zstd, xz
and zlib support and the kernel will load the modules, assuming it
supports the format \o/

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 3a79464..b69f1ef 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -174,9 +174,14 @@ out:
 	free((void *)zst_outb.dst);
 	return ret;
 }
+#else
+static int load_zstd(struct kmod_file *file)
+{
+	return -ENOSYS;
+}
+#endif
 
 static const char magic_zstd[] = {0x28, 0xB5, 0x2F, 0xFD};
-#endif
 
 #ifdef ENABLE_XZ
 static void xz_uncompress_belch(struct kmod_file *file, lzma_ret ret)
@@ -275,9 +280,14 @@ static int load_xz(struct kmod_file *file)
 	lzma_end(&strm);
 	return ret;
 }
+#else
+static int load_xz(struct kmod_file *file)
+{
+	return -ENOSYS;
+}
+#endif
 
 static const char magic_xz[] = {0xfd, '7', 'z', 'X', 'Z', 0};
-#endif
 
 #ifdef ENABLE_ZLIB
 #define READ_STEP (4 * 1024 * 1024)
@@ -339,9 +349,14 @@ error:
 	gzclose(gzf); /* closes the gzfd */
 	return err;
 }
+#else
+static int load_zlib(struct kmod_file *file)
+{
+	return -ENOSYS;
+}
+#endif
 
 static const char magic_zlib[] = {0x1f, 0x8b};
-#endif
 
 static const struct comp_type {
 	size_t magic_size;
@@ -349,15 +364,9 @@ static const struct comp_type {
 	const char *magic_bytes;
 	int (*load)(struct kmod_file *file);
 } comp_types[] = {
-#ifdef ENABLE_ZSTD
 	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
-#endif
-#ifdef ENABLE_XZ
 	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
-#endif
-#ifdef ENABLE_ZLIB
 	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
-#endif
 	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
 };
 

-- 
2.43.0

[PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Since all the compression magic is always available now, we don't need
to loop at runtime nor use alloca - latter of which comes with a handful
of caveats.

Simply throw in a few assert_cc(), which will trigger at build-time.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index b69f1ef..5b88d6c 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -410,7 +410,6 @@ 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 = 0;
 
 	if (file == NULL)
@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 		goto error;
 	}
 
-	for (itr = comp_types; itr->load != NULL; itr++) {
-		if (magic_size_max < itr->magic_size)
-			magic_size_max = itr->magic_size;
-	}
-
-	if (magic_size_max > 0) {
-		char *buf = alloca(magic_size_max + 1);
+	{
+		char buf[7];
 		ssize_t sz;
 
-		if (buf == NULL) {
-			err = -errno;
-			goto error;
-		}
-		sz = read_str_safe(file->fd, buf, magic_size_max + 1);
+		assert_cc(sizeof(magic_zstd) < sizeof(buf));
+		assert_cc(sizeof(magic_xz) < sizeof(buf));
+		assert_cc(sizeof(magic_zlib) < sizeof(buf));
+
+		sz = read_str_safe(file->fd, buf, sizeof(buf));
 		lseek(file->fd, 0, SEEK_SET);
-		if (sz != (ssize_t)magic_size_max) {
+		if (sz != (sizeof(buf) - 1)) {
 			if (sz < 0)
 				err = sz;
 			else

-- 
2.43.0

[PATCH kmod 10/13] libkmod: tidy-up kmod_file_open()

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

This commit cleans up the indentation and the error path of the
function. It bears no functional changes.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 60 +++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 5b88d6c..c4893fd 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -410,41 +410,40 @@ 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;
-	int err = 0;
+	char buf[7];
+	ssize_t sz;
 
 	if (file == NULL)
 		return NULL;
 
 	file->fd = open(filename, O_RDONLY|O_CLOEXEC);
 	if (file->fd < 0) {
-		err = -errno;
-		goto error;
+		free(file);
+		return NULL;
 	}
 
-	{
-		char buf[7];
-		ssize_t sz;
-
-		assert_cc(sizeof(magic_zstd) < sizeof(buf));
-		assert_cc(sizeof(magic_xz) < sizeof(buf));
-		assert_cc(sizeof(magic_zlib) < sizeof(buf));
-
-		sz = read_str_safe(file->fd, buf, sizeof(buf));
-		lseek(file->fd, 0, SEEK_SET);
-		if (sz != (sizeof(buf) - 1)) {
-			if (sz < 0)
-				err = sz;
-			else
-				err = -EINVAL;
-			goto error;
-		}
+	assert_cc(sizeof(magic_zstd) < sizeof(buf));
+	assert_cc(sizeof(magic_xz) < sizeof(buf));
+	assert_cc(sizeof(magic_zlib) < sizeof(buf));
 
-		for (itr = comp_types; itr->load != NULL; itr++) {
-			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
-				file->load = itr->load;
-				file->compression = itr->compression;
-				break;
-			}
+	sz = read_str_safe(file->fd, buf, sizeof(buf));
+	lseek(file->fd, 0, SEEK_SET);
+	if (sz != (sizeof(buf) - 1)) {
+		if (sz < 0)
+			errno = -sz;
+		else
+			errno = EINVAL;
+
+		close(file->fd);
+		free(file);
+		return NULL;
+	}
+
+	for (itr = comp_types; itr->load != NULL; itr++) {
+		if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
+			file->load = itr->load;
+			file->compression = itr->compression;
+			break;
 		}
 	}
 
@@ -455,15 +454,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 
 	file->ctx = ctx;
 
-error:
-	if (err < 0) {
-		if (file->fd >= 0)
-			close(file->fd);
-		free(file);
-		errno = -err;
-		return NULL;
-	}
-
 	return file;
 }
 

-- 
2.43.0

[PATCH kmod 11/13] libkmod: move load_reg() further up

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

We're about to reference it in comp_types with next commit.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index c4893fd..db775a6 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -358,18 +358,6 @@ static int load_zlib(struct kmod_file *file)
 
 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;
-	int (*load)(struct kmod_file *file);
-} comp_types[] = {
-	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
-	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
-	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
-};
-
 static int load_reg(struct kmod_file *file)
 {
 	struct stat st;
@@ -388,6 +376,18 @@ static int load_reg(struct kmod_file *file)
 	return 0;
 }
 
+static const struct comp_type {
+	size_t magic_size;
+	enum kmod_file_compression_type compression;
+	const char *magic_bytes;
+	int (*load)(struct kmod_file *file);
+} comp_types[] = {
+	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
+	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
+	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
+};
+
 struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
 {
 	int err;

-- 
2.43.0

[PATCH kmod 12/13] libkmod: keep KMOD_FILE_COMPRESSION_NONE/load_reg in comp_types

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

It's cleaner to handle all compression types and load functions in the
same style.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-file.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index db775a6..f162a10 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -385,7 +385,7 @@ static const struct comp_type {
 	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
 	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
 	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, load_reg}
 };
 
 struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
@@ -409,7 +409,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 						const char *filename)
 {
 	struct kmod_file *file = calloc(1, sizeof(struct kmod_file));
-	const struct comp_type *itr;
 	char buf[7];
 	ssize_t sz;
 
@@ -439,19 +438,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
 		return NULL;
 	}
 
-	for (itr = comp_types; itr->load != NULL; itr++) {
-		if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
-			file->load = itr->load;
-			file->compression = itr->compression;
+	for (unsigned int i = 0; i < ARRAY_SIZE(comp_types); i++) {
+		const struct comp_type *itr = &comp_types[i];
+
+		file->load = itr->load;
+		file->compression = itr->compression;
+		if (itr->magic_size &&
+		    memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
 			break;
 		}
 	}
 
-	if (file->load == NULL) {
-		file->load = load_reg;
-		file->compression = KMOD_FILE_COMPRESSION_NONE;
-	}
-
 	file->ctx = ctx;
 
 	return file;

-- 
2.43.0

[PATCH kmod 13/13] libkmod: always fallback to do_init_module()

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Since the direct loading was introduced a few bugs became obvious that
the compression args used were off - both in-kernel and dkms.

While both of those are fixed already, not all of those have reached all
users. For example: for dkms I'm aiming to do a release just as kmod has
theirs (to align /lib/modules <> /usr/lib/modules support).

Although I am wondering if we can indiscriminatingly callback to the old
do_init_module() in all the cases. This means that we'll catch any
in-kernel decompression issues - invalid args, ENOMEM, other....
Although for others (wrong magic, perm, etc) we will end up doing the
exact kernel work twice.

Overall the trade-off seems worth it, so flip this.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 libkmod/libkmod-module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index d309948..2c0d46d 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -980,7 +980,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 	}
 
 	err = do_finit_module(mod, flags, args);
-	if (err == -ENOSYS)
+	if (err)
 		err = do_init_module(mod, flags, args);
 
 	if (err < 0)

-- 
2.43.0

Re: [PATCH kmod 08/13] libkmod: always detect the module compression

[Emil Velikov]

On Mon, 12 Feb 2024 at 17:23, Emil Velikov via B4 Relay
<devnull+emil.l.velikov.gmail.com@kernel.org> wrote:
>
> From: Emil Velikov <emil.l.velikov@gmail.com>
>
> Currently, when built w/o given compression we'll incorrectly report a
> "compression_none".
>
> As we reach do_finit_module(), we'll naively assume that the kernel can
> handle the compressed module, yet omit the MODULE_INIT_COMPRESSED_FILE
> flag.
>
> As result the kernel will barf at us, do_finit_module will fail with non
> -ENOSYS and we won't end in the do_init_module codepath (which will also
> fail).
>
> In other words: with this change, you can build kmod without zstd, xz
> and zlib support and the kernel will load the modules, assuming it
> supports the format \o/
>

Important part to note here is that the above is only valid for insmod
and modprobe.

Tools such as depmod and modinfo, still depend on kmod being built
with e.g. zstd, in order for `.ko.zstd` modules to be considered valid
entries.
I'm not 100% sure about modinfo, at a glance those tools require
decompression support in order to parse the elfs and extract the
required module information.

Regardless of this caveat I do see value in this series:
 - it notably simplifies the codebase
 - You can use different kmod on the build vs target system.

Namely: Yocto recipe with full blown kmod/depmod generates the mutable
modules.xxx files, yet the target system uses compression-less kmod
for insmod/modprobe/rmmod/lsmod.

HTH
Emil

Re: [PATCH kmod 02/13] libkmod: keep gzFile gzf local to load_zlib()

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:03PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>There is no need to keep the root gzFile context open for the whole
>duration. Once we've copied the decompressed module to file->memory we
>can close the handle.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-file.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index b97062b..9a014ea 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -53,9 +53,6 @@ struct kmod_file {
> #endif
> #ifdef ENABLE_XZ
> 	bool xz_used;
>-#endif
>-#ifdef ENABLE_ZLIB
>-	gzFile gzf;
> #endif
> 	int fd;
> 	enum kmod_file_compression_type compression;
>@@ -317,6 +314,7 @@ static int load_zlib(struct kmod_file *file)
> 	int err = 0;
> 	off_t did = 0, total = 0;
> 	_cleanup_free_ unsigned char *p = NULL;
>+	gzFile gzf;
> 	int gzfd;
>
> 	errno = 0;
>@@ -324,8 +322,8 @@ static int load_zlib(struct kmod_file *file)
> 	if (gzfd < 0)
> 		return -errno;
>
>-	file->gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
>-	if (file->gzf == NULL) {
>+	gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
>+	if (gzf == NULL) {
> 		close(gzfd);
> 		return -errno;
> 	}
>@@ -343,12 +341,12 @@ static int load_zlib(struct kmod_file *file)
> 			p = tmp;
> 		}
>
>-		r = gzread(file->gzf, p + did, total - did);
>+		r = gzread(gzf, p + did, total - did);
> 		if (r == 0)
> 			break;
> 		else if (r < 0) {
> 			int gzerr;
>-			const char *gz_errmsg = gzerror(file->gzf, &gzerr);
>+			const char *gz_errmsg = gzerror(gzf, &gzerr);
>
> 			ERR(file->ctx, "gzip: %s\n", gz_errmsg);
>
>@@ -362,19 +360,17 @@ static int load_zlib(struct kmod_file *file)
> 	file->memory = p;
> 	file->size = did;
> 	p = NULL;
>+	gzclose(gzf);
> 	return 0;
>
> error:
>-	gzclose(file->gzf); /* closes the gzfd */
>+	gzclose(gzf); /* closes the gzfd */
> 	return err;
> }
>
> static void unload_zlib(struct kmod_file *file)
> {
>-	if (file->gzf == NULL)
>-		return;
> 	free(file->memory);
>-	gzclose(file->gzf);
> }
>
> static const char magic_zlib[] = {0x1f, 0x8b};
>
>-- 
>2.43.0
>

Re: [PATCH kmod 03/13] libkmod: remove kmod_file::{zstd,xz}_used flags

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:04PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>These are used to protect a free(file->memory), within their respective
>unload functions. Where the sole caller of the unload function already
>does a NULL check prior.
>
>Even so, free(NULL) is guaranteed to be safe by the standard.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-file.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index 9a014ea..abd4723 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -48,12 +48,6 @@ struct file_ops {
> };
>
> struct kmod_file {
>-#ifdef ENABLE_ZSTD
>-	bool zstd_used;
>-#endif
>-#ifdef ENABLE_XZ
>-	bool xz_used;
>-#endif
> 	int fd;
> 	enum kmod_file_compression_type compression;
> 	off_t size;
>@@ -176,7 +170,6 @@ static int load_zstd(struct kmod_file *file)
>
> 	ZSTD_freeDStream(dstr);
> 	free((void *)zst_inb.src);
>-	file->zstd_used = true;
> 	file->memory = zst_outb.dst;
> 	file->size = zst_outb.pos;
> 	return 0;
>@@ -190,8 +183,6 @@ out:
>
> static void unload_zstd(struct kmod_file *file)
> {
>-	if (!file->zstd_used)
>-		return;
> 	free(file->memory);
> }
>
>@@ -269,7 +260,6 @@ static int xz_uncompress(lzma_stream *strm, struct kmod_file *file)
> 			goto out;
> 		}
> 	}
>-	file->xz_used = true;
> 	file->memory = p;
> 	file->size = total;
> 	return 0;
>@@ -299,8 +289,6 @@ static int load_xz(struct kmod_file *file)
>
> static void unload_xz(struct kmod_file *file)
> {
>-	if (!file->xz_used)
>-		return;
> 	free(file->memory);
> }
>
>
>-- 
>2.43.0
>

Re: [PATCH kmod 01/13] libkmod: use a dup()'d fd for zlib

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:02PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>The gzdopen() API used, takes ownership of the fd. To make that more
>explicit we clear it (-1) as applicable.
>
>Yet again, kmod has explicit API to return the fd to the user - which
>currently is used solely when uncompressed, so we're safe.
>
>Regardless - simply duplicate the fd locally and use that with zlib.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>---
> libkmod/libkmod-file.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index b138e7e..b97062b 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -317,12 +317,18 @@ static int load_zlib(struct kmod_file *file)
> 	int err = 0;
> 	off_t did = 0, total = 0;
> 	_cleanup_free_ unsigned char *p = NULL;
>+	int gzfd;
>
> 	errno = 0;
>-	file->gzf = gzdopen(file->fd, "rb");
>-	if (file->gzf == NULL)
>+	gzfd = fcntl(file->fd, F_DUPFD_CLOEXEC, 3);
>+	if (gzfd < 0)
>+		return -errno;
>+
>+	file->gzf = gzdopen(gzfd, "rb"); /* takes ownership of the fd */
>+	if (file->gzf == NULL) {
>+		close(gzfd);
> 		return -errno;
>-	file->fd = -1; /* now owned by gzf due gzdopen() */
>+	}
>
> 	for (;;) {
> 		int r;
>@@ -359,7 +365,7 @@ static int load_zlib(struct kmod_file *file)
> 	return 0;
>
> error:
>-	gzclose(file->gzf);
>+	gzclose(file->gzf); /* closes the gzfd */
> 	return err;
> }
>
>@@ -368,7 +374,7 @@ static void unload_zlib(struct kmod_file *file)
> 	if (file->gzf == NULL)
> 		return;
> 	free(file->memory);
>-	gzclose(file->gzf); /* closes file->fd */
>+	gzclose(file->gzf);
> }
>
> static const char magic_zlib[] = {0x1f, 0x8b};
>@@ -535,7 +541,6 @@ void kmod_file_unref(struct kmod_file *file)
> 	if (file->memory)
> 		file->ops->unload(file);
>
>-	if (file->fd >= 0)
>-		close(file->fd);
>+	close(file->fd);
> 	free(file);
> }
>
>-- 
>2.43.0
>

Re: [PATCH kmod 04/13] libkmod: clear file->memory if map fails

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:05PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>On mmap failure file->memory is set to -1, which we'll happily pass down
>to munmap later on.
>
>More importantly, since we do a NULL check in kmod_file_load_contents()
>we will exit the function without (re)attempting the load again.
>
>Since we ignore the return code for the load function(s), one can end up
>calling kmod_elf_get_memory() and feed that -1 into init_module.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>---
> libkmod/libkmod-file.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index abd4723..b408aed 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -392,8 +392,10 @@ static int load_reg(struct kmod_file *file)
> 	file->size = st.st_size;
> 	file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
> 			    file->fd, 0);
>-	if (file->memory == MAP_FAILED)
>+	if (file->memory == MAP_FAILED) {
>+		file->memory = NULL;
> 		return -errno;
>+	}
>
> 	return 0;
> }
>
>-- 
>2.43.0
>

Re: [PATCH kmod 05/13] libkmod: nuke struct file_ops

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:06PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>With the previous commits, we removed the need for a distinct unload
>callback.
>
>So nuke the struct all together and only use/keep the load one around.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>---


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

> libkmod/libkmod-file.c | 62 +++++++++++++++-----------------------------------
> 1 file changed, 18 insertions(+), 44 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index b408aed..8a0336f 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -41,18 +41,12 @@
> #include "libkmod.h"
> #include "libkmod-internal.h"
>
>-struct kmod_file;
>-struct file_ops {
>-	int (*load)(struct kmod_file *file);
>-	void (*unload)(struct kmod_file *file);
>-};
>-
> struct kmod_file {
> 	int fd;
> 	enum kmod_file_compression_type compression;
> 	off_t size;
> 	void *memory;
>-	const struct file_ops *ops;
>+	int (*load)(struct kmod_file *file);
> 	const struct kmod_ctx *ctx;
> 	struct kmod_elf *elf;
> };
>@@ -181,11 +175,6 @@ out:
> 	return ret;
> }
>
>-static void unload_zstd(struct kmod_file *file)
>-{
>-	free(file->memory);
>-}
>-
> static const char magic_zstd[] = {0x28, 0xB5, 0x2F, 0xFD};
> #endif
>
>@@ -287,11 +276,6 @@ static int load_xz(struct kmod_file *file)
> 	return ret;
> }
>
>-static void unload_xz(struct kmod_file *file)
>-{
>-	free(file->memory);
>-}
>-
> static const char magic_xz[] = {0xfd, '7', 'z', 'X', 'Z', 0};
> #endif
>
>@@ -356,11 +340,6 @@ error:
> 	return err;
> }
>
>-static void unload_zlib(struct kmod_file *file)
>-{
>-	free(file->memory);
>-}
>-
> static const char magic_zlib[] = {0x1f, 0x8b};
> #endif
>
>@@ -368,18 +347,18 @@ static const struct comp_type {
> 	size_t magic_size;
> 	enum kmod_file_compression_type compression;
> 	const char *magic_bytes;
>-	const struct file_ops ops;
>+	int (*load)(struct kmod_file *file);
> } comp_types[] = {
> #ifdef ENABLE_ZSTD
>-	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, {load_zstd, unload_zstd}},
>+	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
> #endif
> #ifdef ENABLE_XZ
>-	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, {load_xz, unload_xz}},
>+	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
> #endif
> #ifdef ENABLE_ZLIB
>-	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, {load_zlib, unload_zlib}},
>+	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
> #endif
>-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, {NULL, NULL}}
>+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
> };
>
> static int load_reg(struct kmod_file *file)
>@@ -400,15 +379,6 @@ static int load_reg(struct kmod_file *file)
> 	return 0;
> }
>
>-static void unload_reg(struct kmod_file *file)
>-{
>-	munmap(file->memory, file->size);
>-}
>-
>-static const struct file_ops reg_ops = {
>-	load_reg, unload_reg
>-};
>-
> struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
> {
> 	if (file->elf)
>@@ -436,7 +406,7 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> 		goto error;
> 	}
>
>-	for (itr = comp_types; itr->ops.load != NULL; itr++) {
>+	for (itr = comp_types; itr->load != NULL; itr++) {
> 		if (magic_size_max < itr->magic_size)
> 			magic_size_max = itr->magic_size;
> 	}
>@@ -459,17 +429,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> 			goto error;
> 		}
>
>-		for (itr = comp_types; itr->ops.load != NULL; itr++) {
>+		for (itr = comp_types; itr->load != NULL; itr++) {
> 			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
>-				file->ops = &itr->ops;
>+				file->load = itr->load;
> 				file->compression = itr->compression;
> 				break;
> 			}
> 		}
> 	}
>
>-	if (file->ops == NULL) {
>-		file->ops = &reg_ops;
>+	if (file->load == NULL) {
>+		file->load = load_reg;
> 		file->compression = KMOD_FILE_COMPRESSION_NONE;
> 	}
>
>@@ -496,7 +466,7 @@ void kmod_file_load_contents(struct kmod_file *file)
> 		return;
>
> 	/*  The load functions already log possible errors. */
>-	file->ops->load(file);
>+	file->load(file);
> }
>
> void *kmod_file_get_contents(const struct kmod_file *file)
>@@ -524,8 +494,12 @@ void kmod_file_unref(struct kmod_file *file)
> 	if (file->elf)
> 		kmod_elf_unref(file->elf);
>
>-	if (file->memory)
>-		file->ops->unload(file);
>+	if (file->compression == KMOD_FILE_COMPRESSION_NONE) {
>+		if (file->memory)
>+			munmap(file->memory, file->size);
>+	} else {
>+		free(file->memory);
>+	}
>
> 	close(file->fd);
> 	free(file);
>
>-- 
>2.43.0
>

Re: [PATCH kmod 08/13] libkmod: always detect the module compression

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:09PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>Currently, when built w/o given compression we'll incorrectly report a
>"compression_none".
>
>As we reach do_finit_module(), we'll naively assume that the kernel can
>handle the compressed module, yet omit the MODULE_INIT_COMPRESSED_FILE
>flag.
>
>As result the kernel will barf at us, do_finit_module will fail with non
>-ENOSYS and we won't end in the do_init_module codepath (which will also
>fail).
>
>In other words: with this change, you can build kmod without zstd, xz
>and zlib support and the kernel will load the modules, assuming it
>supports the format \o/
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

nice.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>---
> libkmod/libkmod-file.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index 3a79464..b69f1ef 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -174,9 +174,14 @@ out:
> 	free((void *)zst_outb.dst);
> 	return ret;
> }
>+#else
>+static int load_zstd(struct kmod_file *file)
>+{
>+	return -ENOSYS;
>+}
>+#endif
>
> static const char magic_zstd[] = {0x28, 0xB5, 0x2F, 0xFD};
>-#endif
>
> #ifdef ENABLE_XZ
> static void xz_uncompress_belch(struct kmod_file *file, lzma_ret ret)
>@@ -275,9 +280,14 @@ static int load_xz(struct kmod_file *file)
> 	lzma_end(&strm);
> 	return ret;
> }
>+#else
>+static int load_xz(struct kmod_file *file)
>+{
>+	return -ENOSYS;
>+}
>+#endif
>
> static const char magic_xz[] = {0xfd, '7', 'z', 'X', 'Z', 0};
>-#endif
>
> #ifdef ENABLE_ZLIB
> #define READ_STEP (4 * 1024 * 1024)
>@@ -339,9 +349,14 @@ error:
> 	gzclose(gzf); /* closes the gzfd */
> 	return err;
> }
>+#else
>+static int load_zlib(struct kmod_file *file)
>+{
>+	return -ENOSYS;
>+}
>+#endif
>
> static const char magic_zlib[] = {0x1f, 0x8b};
>-#endif
>
> static const struct comp_type {
> 	size_t magic_size;
>@@ -349,15 +364,9 @@ static const struct comp_type {
> 	const char *magic_bytes;
> 	int (*load)(struct kmod_file *file);
> } comp_types[] = {
>-#ifdef ENABLE_ZSTD
> 	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
>-#endif
>-#ifdef ENABLE_XZ
> 	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
>-#endif
>-#ifdef ENABLE_ZLIB
> 	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
>-#endif
> 	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
> };
>
>
>-- 
>2.43.0
>

Re: [PATCH kmod 07/13] libkmod: move kmod_file_load_contents as applicable

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:08PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>When dealing with an elf, we don't know or care about loading the file.
>The kmod_elf subsystem/API will deal with the required parts itself.
>
>Which in this case, already calls kmod_file_load_contents() as
>applicable.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-module.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>index 1e43482..d309948 100644
>--- a/libkmod/libkmod-module.c
>+++ b/libkmod/libkmod-module.c
>@@ -903,10 +903,6 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
> 	off_t size;
> 	int err;
>
>-	err = kmod_file_load_contents(mod->file);
>-	if (err)
>-		return err;
>-
> 	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
> 		elf = kmod_file_get_elf(mod->file);
> 		if (elf == NULL) {
>@@ -928,6 +924,10 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
>
> 		mem = kmod_elf_get_memory(elf);
> 	} else {
>+		err = kmod_file_load_contents(mod->file);
>+		if (err)
>+			return err;
>+
> 		mem = kmod_file_get_contents(mod->file);
> 	}
> 	size = kmod_file_get_size(mod->file);
>
>-- 
>2.43.0
>

Re: [PATCH kmod 06/13] libkmod: propagate {zstd,xz,zlib}_load errors

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:07PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>Propagate any errors during decompression further up the call stack.
>Without this we could easily pass NULL as mem to init_module(2).
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-file.c     | 15 +++++++++++----
> libkmod/libkmod-internal.h |  2 +-
> libkmod/libkmod-module.c   |  4 +++-
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index 8a0336f..3a79464 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -381,10 +381,17 @@ static int load_reg(struct kmod_file *file)
>
> struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
> {
>+	int err;
>+
> 	if (file->elf)
> 		return file->elf;
>
>-	kmod_file_load_contents(file);
>+	err = kmod_file_load_contents(file);
>+	if (err) {
>+		errno = err;
>+		return NULL;
>+	}
>+
> 	file->elf = kmod_elf_new(file->memory, file->size);
> 	return file->elf;
> }
>@@ -460,13 +467,13 @@ error:
> /*
>  *  Callers should just check file->memory got updated
>  */
>-void kmod_file_load_contents(struct kmod_file *file)
>+int kmod_file_load_contents(struct kmod_file *file)
> {
> 	if (file->memory)
>-		return;
>+		return 0;
>
> 	/*  The load functions already log possible errors. */
>-	file->load(file);
>+	return file->load(file);
> }
>
> void *kmod_file_get_contents(const struct kmod_file *file)
>diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
>index 26a7e28..3bc6e11 100644
>--- a/libkmod/libkmod-internal.h
>+++ b/libkmod/libkmod-internal.h
>@@ -160,7 +160,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)));
>-void kmod_file_load_contents(struct kmod_file *file) __attribute__((nonnull(1)));
>+int kmod_file_load_contents(struct kmod_file *file) __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)));
> enum kmod_file_compression_type kmod_file_get_compression(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
>diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>index 585da41..1e43482 100644
>--- a/libkmod/libkmod-module.c
>+++ b/libkmod/libkmod-module.c
>@@ -903,7 +903,9 @@ static int do_init_module(struct kmod_module *mod, unsigned int flags,
> 	off_t size;
> 	int err;
>
>-	kmod_file_load_contents(mod->file);
>+	err = kmod_file_load_contents(mod->file);
>+	if (err)
>+		return err;
>
> 	if (flags & (KMOD_INSERT_FORCE_VERMAGIC | KMOD_INSERT_FORCE_MODVERSION)) {
> 		elf = kmod_file_get_elf(mod->file);
>
>-- 
>2.43.0
>

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>Since all the compression magic is always available now, we don't need
>to loop at runtime nor use alloca - latter of which comes with a handful
>of caveats.
>
>Simply throw in a few assert_cc(), which will trigger at build-time.

given the small size, we actually never needed it and could have added
the asserts.

But this gets rid of more, with the loop going away, and I'm happy with
it.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

Re: [PATCH kmod 10/13] libkmod: tidy-up kmod_file_open()

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:11PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>This commit cleans up the indentation and the error path of the
>function. It bears no functional changes.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-file.c | 60 +++++++++++++++++++++-----------------------------
> 1 file changed, 25 insertions(+), 35 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index 5b88d6c..c4893fd 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -410,41 +410,40 @@ 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;
>-	int err = 0;
>+	char buf[7];
>+	ssize_t sz;
>
> 	if (file == NULL)
> 		return NULL;
>
> 	file->fd = open(filename, O_RDONLY|O_CLOEXEC);
> 	if (file->fd < 0) {
>-		err = -errno;
>-		goto error;
>+		free(file);
>+		return NULL;
> 	}
>
>-	{
>-		char buf[7];
>-		ssize_t sz;
>-
>-		assert_cc(sizeof(magic_zstd) < sizeof(buf));
>-		assert_cc(sizeof(magic_xz) < sizeof(buf));
>-		assert_cc(sizeof(magic_zlib) < sizeof(buf));
>-
>-		sz = read_str_safe(file->fd, buf, sizeof(buf));
>-		lseek(file->fd, 0, SEEK_SET);
>-		if (sz != (sizeof(buf) - 1)) {
>-			if (sz < 0)
>-				err = sz;
>-			else
>-				err = -EINVAL;
>-			goto error;
>-		}
>+	assert_cc(sizeof(magic_zstd) < sizeof(buf));
>+	assert_cc(sizeof(magic_xz) < sizeof(buf));
>+	assert_cc(sizeof(magic_zlib) < sizeof(buf));
>
>-		for (itr = comp_types; itr->load != NULL; itr++) {
>-			if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
>-				file->load = itr->load;
>-				file->compression = itr->compression;
>-				break;
>-			}
>+	sz = read_str_safe(file->fd, buf, sizeof(buf));
>+	lseek(file->fd, 0, SEEK_SET);
>+	if (sz != (sizeof(buf) - 1)) {
>+		if (sz < 0)
>+			errno = -sz;
>+		else
>+			errno = EINVAL;
>+
>+		close(file->fd);
>+		free(file);
>+		return NULL;
>+	}
>+
>+	for (itr = comp_types; itr->load != NULL; itr++) {
>+		if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
>+			file->load = itr->load;
>+			file->compression = itr->compression;
>+			break;
> 		}
> 	}
>
>@@ -455,15 +454,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
>
> 	file->ctx = ctx;
>
>-error:
>-	if (err < 0) {
>-		if (file->fd >= 0)
>-			close(file->fd);
>-		free(file);
>-		errno = -err;
>-		return NULL;
>-	}
>-
> 	return file;
> }
>
>
>-- 
>2.43.0
>

Re: [PATCH kmod 11/13] libkmod: move load_reg() further up

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:12PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>We're about to reference it in comp_types with next commit.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi


>---
> libkmod/libkmod-file.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index c4893fd..db775a6 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -358,18 +358,6 @@ static int load_zlib(struct kmod_file *file)
>
> 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;
>-	int (*load)(struct kmod_file *file);
>-} comp_types[] = {
>-	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
>-	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
>-	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
>-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
>-};
>-
> static int load_reg(struct kmod_file *file)
> {
> 	struct stat st;
>@@ -388,6 +376,18 @@ static int load_reg(struct kmod_file *file)
> 	return 0;
> }
>
>+static const struct comp_type {
>+	size_t magic_size;
>+	enum kmod_file_compression_type compression;
>+	const char *magic_bytes;
>+	int (*load)(struct kmod_file *file);
>+} comp_types[] = {
>+	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
>+	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
>+	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
>+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
>+};
>+
> struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
> {
> 	int err;
>
>-- 
>2.43.0
>

Re: [PATCH kmod 12/13] libkmod: keep KMOD_FILE_COMPRESSION_NONE/load_reg in comp_types

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:13PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>It's cleaner to handle all compression types and load functions in the
>same style.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> libkmod/libkmod-file.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index db775a6..f162a10 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -385,7 +385,7 @@ static const struct comp_type {
> 	{sizeof(magic_zstd),	KMOD_FILE_COMPRESSION_ZSTD, magic_zstd, load_zstd},
> 	{sizeof(magic_xz),	KMOD_FILE_COMPRESSION_XZ, magic_xz, load_xz},
> 	{sizeof(magic_zlib),	KMOD_FILE_COMPRESSION_ZLIB, magic_zlib, load_zlib},
>-	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, NULL}
>+	{0,			KMOD_FILE_COMPRESSION_NONE, NULL, load_reg}
> };
>
> struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
>@@ -409,7 +409,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> 						const char *filename)
> {
> 	struct kmod_file *file = calloc(1, sizeof(struct kmod_file));
>-	const struct comp_type *itr;
> 	char buf[7];
> 	ssize_t sz;
>
>@@ -439,19 +438,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> 		return NULL;
> 	}
>
>-	for (itr = comp_types; itr->load != NULL; itr++) {
>-		if (memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
>-			file->load = itr->load;
>-			file->compression = itr->compression;
>+	for (unsigned int i = 0; i < ARRAY_SIZE(comp_types); i++) {
>+		const struct comp_type *itr = &comp_types[i];
>+
>+		file->load = itr->load;
>+		file->compression = itr->compression;
>+		if (itr->magic_size &&
>+		    memcmp(buf, itr->magic_bytes, itr->magic_size) == 0) {
> 			break;
> 		}
> 	}
>
>-	if (file->load == NULL) {
>-		file->load = load_reg;
>-		file->compression = KMOD_FILE_COMPRESSION_NONE;
>-	}
>-
> 	file->ctx = ctx;
>
> 	return file;
>
>-- 
>2.43.0
>

Re: [PATCH kmod 13/13] libkmod: always fallback to do_init_module()

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:14PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>Since the direct loading was introduced a few bugs became obvious that
>the compression args used were off - both in-kernel and dkms.
>
>While both of those are fixed already, not all of those have reached all
>users. For example: for dkms I'm aiming to do a release just as kmod has
>theirs (to align /lib/modules <> /usr/lib/modules support).
>
>Although I am wondering if we can indiscriminatingly callback to the old
>do_init_module() in all the cases. This means that we'll catch any
>in-kernel decompression issues - invalid args, ENOMEM, other....
>Although for others (wrong magic, perm, etc) we will end up doing the
>exact kernel work twice.

I'm not sure I like to repeat this for any error. Example: if we get an
EAGAIN we go and try again?  When thinking about the recent in-kernel
optimizations to stop loading the same file again over and over, just
doing it again seems like a wrong approach.

>
>Overall the trade-off seems worth it, so flip this.


I'm not sure. I will keep this out for now and apply the rest.
+Luis too.


Lucas De Marchi


>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>---
> libkmod/libkmod-module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>index d309948..2c0d46d 100644
>--- a/libkmod/libkmod-module.c
>+++ b/libkmod/libkmod-module.c
>@@ -980,7 +980,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> 	}
>
> 	err = do_finit_module(mod, flags, args);
>-	if (err == -ENOSYS)
>+	if (err)
> 		err = do_init_module(mod, flags, args);
>
> 	if (err < 0)
>
>-- 
>2.43.0
>

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Lucas De Marchi]

On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.l.velikov@gmail.com>
>
>Since all the compression magic is always available now, we don't need
>to loop at runtime nor use alloca - latter of which comes with a handful
>of caveats.
>
>Simply throw in a few assert_cc(), which will trigger at build-time.
>
>Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>---
> libkmod/libkmod-file.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
>diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>index b69f1ef..5b88d6c 100644
>--- a/libkmod/libkmod-file.c
>+++ b/libkmod/libkmod-file.c
>@@ -410,7 +410,6 @@ 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 = 0;
>
> 	if (file == NULL)
>@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> 		goto error;
> 	}
>
>-	for (itr = comp_types; itr->load != NULL; itr++) {
>-		if (magic_size_max < itr->magic_size)
>-			magic_size_max = itr->magic_size;
>-	}
>-
>-	if (magic_size_max > 0) {
>-		char *buf = alloca(magic_size_max + 1);
>+	{
>+		char buf[7];
> 		ssize_t sz;
>
>-		if (buf == NULL) {
>-			err = -errno;
>-			goto error;
>-		}
>-		sz = read_str_safe(file->fd, buf, magic_size_max + 1);
>+		assert_cc(sizeof(magic_zstd) < sizeof(buf));
>+		assert_cc(sizeof(magic_xz) < sizeof(buf));
>+		assert_cc(sizeof(magic_zlib) < sizeof(buf));

../libkmod/libkmod-file.c: In function 'kmod_file_open':
../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    25 |         _Static_assert((expr), #expr)
       |         ^~~~~~~~~~~~~~
../libkmod/libkmod-file.c:424:9: note: in expansion of macro 'assert_cc'
   424 |         assert_cc(sizeof(magic_zstd) < sizeof(buf));
       |         ^~~~~~~~~


So I'd go with this on top of this patch...  I can squash it as needed
when applying:


|diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
|index f162a10..8baf12d 100644
|--- a/libkmod/libkmod-file.c
|+++ b/libkmod/libkmod-file.c
|@@ -408,10 +408,15 @@ struct kmod_elf *kmod_file_get_elf(struct kmod_file *file)
| struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
| 						const char *filename)
| {
|-	struct kmod_file *file = calloc(1, sizeof(struct kmod_file));
|+	struct kmod_file *file;
| 	char buf[7];
| 	ssize_t sz;
| 
|+	assert_cc(sizeof(magic_zstd) < sizeof(buf));
|+	assert_cc(sizeof(magic_xz) < sizeof(buf));
|+	assert_cc(sizeof(magic_zlib) < sizeof(buf));
|+
|+	file = calloc(1, sizeof(struct kmod_file));
| 	if (file == NULL)
| 		return NULL;
| 
|@@ -421,10 +426,6 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
| 		return NULL;
| 	}
| 
|-	assert_cc(sizeof(magic_zstd) < sizeof(buf));
|-	assert_cc(sizeof(magic_xz) < sizeof(buf));
|-	assert_cc(sizeof(magic_zlib) < sizeof(buf));
|-
| 	sz = read_str_safe(file->fd, buf, sizeof(buf));
| 	lseek(file->fd, 0, SEEK_SET);
| 	if (sz != (sizeof(buf) - 1)) {

Lucas De Marchi

Re: [PATCH kmod 13/13] libkmod: always fallback to do_init_module()

[Emil Velikov]

On Tue, 30 Apr 2024 at 00:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Mon, Feb 12, 2024 at 05:23:14PM GMT, Emil Velikov via B4 Relay wrote:
> >From: Emil Velikov <emil.l.velikov@gmail.com>
> >
> >Since the direct loading was introduced a few bugs became obvious that
> >the compression args used were off - both in-kernel and dkms.
> >
> >While both of those are fixed already, not all of those have reached all
> >users. For example: for dkms I'm aiming to do a release just as kmod has
> >theirs (to align /lib/modules <> /usr/lib/modules support).
> >
> >Although I am wondering if we can indiscriminatingly callback to the old
> >do_init_module() in all the cases. This means that we'll catch any
> >in-kernel decompression issues - invalid args, ENOMEM, other....
> >Although for others (wrong magic, perm, etc) we will end up doing the
> >exact kernel work twice.
>
> I'm not sure I like to repeat this for any error. Example: if we get an
> EAGAIN we go and try again?  When thinking about the recent in-kernel
> optimizations to stop loading the same file again over and over, just
> doing it again seems like a wrong approach.
>

To be honest I did notice the optimisation work in the kernel, but
wasn't able to quantify its benefits on my end. ¯\_(ツ)_/¯

The most wide-spread issue (incompatible compression arguments in
kernel and dkms) has been fixed and new releases are available. And
other triggers do not seem that common/likely. So deferring/dropping
this patch makes sense.

> >
> >Overall the trade-off seems worth it, so flip this.
>
>
> I'm not sure. I will keep this out for now and apply the rest.
> +Luis too.
>

Ack, thanks o/
-Emil

>
> Lucas De Marchi
>
>
> >
> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >---
> > libkmod/libkmod-module.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> >index d309948..2c0d46d 100644
> >--- a/libkmod/libkmod-module.c
> >+++ b/libkmod/libkmod-module.c
> >@@ -980,7 +980,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> >       }
> >
> >       err = do_finit_module(mod, flags, args);
> >-      if (err == -ENOSYS)
> >+      if (err)
> >               err = do_init_module(mod, flags, args);
> >
> >       if (err < 0)
> >
> >--
> >2.43.0
> >

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Emil Velikov]

On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
> >From: Emil Velikov <emil.l.velikov@gmail.com>
> >
> >Since all the compression magic is always available now, we don't need
> >to loop at runtime nor use alloca - latter of which comes with a handful
> >of caveats.
> >
> >Simply throw in a few assert_cc(), which will trigger at build-time.
> >
> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >---
> > libkmod/libkmod-file.c | 22 ++++++++--------------
> > 1 file changed, 8 insertions(+), 14 deletions(-)
> >
> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> >index b69f1ef..5b88d6c 100644
> >--- a/libkmod/libkmod-file.c
> >+++ b/libkmod/libkmod-file.c
> >@@ -410,7 +410,6 @@ 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 = 0;
> >
> >       if (file == NULL)
> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> >               goto error;
> >       }
> >
> >-      for (itr = comp_types; itr->load != NULL; itr++) {
> >-              if (magic_size_max < itr->magic_size)
> >-                      magic_size_max = itr->magic_size;
> >-      }
> >-
> >-      if (magic_size_max > 0) {
> >-              char *buf = alloca(magic_size_max + 1);
> >+      {
> >+              char buf[7];
> >               ssize_t sz;
> >
> >-              if (buf == NULL) {
> >-                      err = -errno;
> >-                      goto error;
> >-              }
> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
>
> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

Is there a particular use-case for explicitly forcing C90?

The configure.ac contains `AC_PROG_CC_C99`, which seems reasonable
IMHO. Plus the autogen.sh goes a step further with `-std=gnu11`

Thanks
Emil

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Lucas De Marchi]

On Tue, Apr 30, 2024 at 06:54:00PM GMT, Emil Velikov wrote:
>On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
>> >From: Emil Velikov <emil.l.velikov@gmail.com>
>> >
>> >Since all the compression magic is always available now, we don't need
>> >to loop at runtime nor use alloca - latter of which comes with a handful
>> >of caveats.
>> >
>> >Simply throw in a few assert_cc(), which will trigger at build-time.
>> >
>> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> >---
>> > libkmod/libkmod-file.c | 22 ++++++++--------------
>> > 1 file changed, 8 insertions(+), 14 deletions(-)
>> >
>> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>> >index b69f1ef..5b88d6c 100644
>> >--- a/libkmod/libkmod-file.c
>> >+++ b/libkmod/libkmod-file.c
>> >@@ -410,7 +410,6 @@ 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 = 0;
>> >
>> >       if (file == NULL)
>> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
>> >               goto error;
>> >       }
>> >
>> >-      for (itr = comp_types; itr->load != NULL; itr++) {
>> >-              if (magic_size_max < itr->magic_size)
>> >-                      magic_size_max = itr->magic_size;
>> >-      }
>> >-
>> >-      if (magic_size_max > 0) {
>> >-              char *buf = alloca(magic_size_max + 1);
>> >+      {
>> >+              char buf[7];
>> >               ssize_t sz;
>> >
>> >-              if (buf == NULL) {
>> >-                      err = -errno;
>> >-                      goto error;
>> >-              }
>> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
>> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
>> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
>> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
>>
>> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
>> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>
>Is there a particular use-case for explicitly forcing C90?

not forcing C90, but forcing -Wdeclaration-after-statement as per
flag passed in configure.ac. I think the message given by gcc is
misleading here.

Lucas De Marchi

>
>The configure.ac contains `AC_PROG_CC_C99`, which seems reasonable
>IMHO. Plus the autogen.sh goes a step further with `-std=gnu11`
>
>Thanks
>Emil

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Emil Velikov]

On Tue, 30 Apr 2024 at 19:18, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Tue, Apr 30, 2024 at 06:54:00PM GMT, Emil Velikov wrote:
> >On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >>
> >> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
> >> >From: Emil Velikov <emil.l.velikov@gmail.com>
> >> >
> >> >Since all the compression magic is always available now, we don't need
> >> >to loop at runtime nor use alloca - latter of which comes with a handful
> >> >of caveats.
> >> >
> >> >Simply throw in a few assert_cc(), which will trigger at build-time.
> >> >
> >> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >> >---
> >> > libkmod/libkmod-file.c | 22 ++++++++--------------
> >> > 1 file changed, 8 insertions(+), 14 deletions(-)
> >> >
> >> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> >> >index b69f1ef..5b88d6c 100644
> >> >--- a/libkmod/libkmod-file.c
> >> >+++ b/libkmod/libkmod-file.c
> >> >@@ -410,7 +410,6 @@ 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 = 0;
> >> >
> >> >       if (file == NULL)
> >> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> >> >               goto error;
> >> >       }
> >> >
> >> >-      for (itr = comp_types; itr->load != NULL; itr++) {
> >> >-              if (magic_size_max < itr->magic_size)
> >> >-                      magic_size_max = itr->magic_size;
> >> >-      }
> >> >-
> >> >-      if (magic_size_max > 0) {
> >> >-              char *buf = alloca(magic_size_max + 1);
> >> >+      {
> >> >+              char buf[7];
> >> >               ssize_t sz;
> >> >
> >> >-              if (buf == NULL) {
> >> >-                      err = -errno;
> >> >-                      goto error;
> >> >-              }
> >> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
> >> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
> >> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
> >> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
> >>
> >> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
> >> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> >
> >Is there a particular use-case for explicitly forcing C90?
>
> not forcing C90, but forcing -Wdeclaration-after-statement as per
> flag passed in configure.ac. I think the message given by gcc is
> misleading here.
>

Indeed thanks. Seems like I should add `export CFLAGS+=-Werror" to my dev box.

-Emil

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Lucas De Marchi]

On Tue, Apr 30, 2024 at 07:27:00PM GMT, Emil Velikov wrote:
>On Tue, 30 Apr 2024 at 19:18, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Tue, Apr 30, 2024 at 06:54:00PM GMT, Emil Velikov wrote:
>> >On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >>
>> >> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
>> >> >From: Emil Velikov <emil.l.velikov@gmail.com>
>> >> >
>> >> >Since all the compression magic is always available now, we don't need
>> >> >to loop at runtime nor use alloca - latter of which comes with a handful
>> >> >of caveats.
>> >> >
>> >> >Simply throw in a few assert_cc(), which will trigger at build-time.
>> >> >
>> >> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> >> >---
>> >> > libkmod/libkmod-file.c | 22 ++++++++--------------
>> >> > 1 file changed, 8 insertions(+), 14 deletions(-)
>> >> >
>> >> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>> >> >index b69f1ef..5b88d6c 100644
>> >> >--- a/libkmod/libkmod-file.c
>> >> >+++ b/libkmod/libkmod-file.c
>> >> >@@ -410,7 +410,6 @@ 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 = 0;
>> >> >
>> >> >       if (file == NULL)
>> >> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
>> >> >               goto error;
>> >> >       }
>> >> >
>> >> >-      for (itr = comp_types; itr->load != NULL; itr++) {
>> >> >-              if (magic_size_max < itr->magic_size)
>> >> >-                      magic_size_max = itr->magic_size;
>> >> >-      }
>> >> >-
>> >> >-      if (magic_size_max > 0) {
>> >> >-              char *buf = alloca(magic_size_max + 1);
>> >> >+      {
>> >> >+              char buf[7];
>> >> >               ssize_t sz;
>> >> >
>> >> >-              if (buf == NULL) {
>> >> >-                      err = -errno;
>> >> >-                      goto error;
>> >> >-              }
>> >> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
>> >> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
>> >> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
>> >> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
>> >>
>> >> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
>> >> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>> >
>> >Is there a particular use-case for explicitly forcing C90?
>>
>> not forcing C90, but forcing -Wdeclaration-after-statement as per
>> flag passed in configure.ac. I think the message given by gcc is
>> misleading here.
>>
>
>Indeed thanks. Seems like I should add `export CFLAGS+=-Werror" to my dev box.

so, are you ok that I just squash this?

Lucas De Marchi

>
>-Emil

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Emil Velikov]

On Tue, 30 Apr 2024 at 19:43, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Tue, Apr 30, 2024 at 07:27:00PM GMT, Emil Velikov wrote:
> >On Tue, 30 Apr 2024 at 19:18, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >>
> >> On Tue, Apr 30, 2024 at 06:54:00PM GMT, Emil Velikov wrote:
> >> >On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> >>
> >> >> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
> >> >> >From: Emil Velikov <emil.l.velikov@gmail.com>
> >> >> >
> >> >> >Since all the compression magic is always available now, we don't need
> >> >> >to loop at runtime nor use alloca - latter of which comes with a handful
> >> >> >of caveats.
> >> >> >
> >> >> >Simply throw in a few assert_cc(), which will trigger at build-time.
> >> >> >
> >> >> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >> >> >---
> >> >> > libkmod/libkmod-file.c | 22 ++++++++--------------
> >> >> > 1 file changed, 8 insertions(+), 14 deletions(-)
> >> >> >
> >> >> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> >> >> >index b69f1ef..5b88d6c 100644
> >> >> >--- a/libkmod/libkmod-file.c
> >> >> >+++ b/libkmod/libkmod-file.c
> >> >> >@@ -410,7 +410,6 @@ 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 = 0;
> >> >> >
> >> >> >       if (file == NULL)
> >> >> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> >> >> >               goto error;
> >> >> >       }
> >> >> >
> >> >> >-      for (itr = comp_types; itr->load != NULL; itr++) {
> >> >> >-              if (magic_size_max < itr->magic_size)
> >> >> >-                      magic_size_max = itr->magic_size;
> >> >> >-      }
> >> >> >-
> >> >> >-      if (magic_size_max > 0) {
> >> >> >-              char *buf = alloca(magic_size_max + 1);
> >> >> >+      {
> >> >> >+              char buf[7];
> >> >> >               ssize_t sz;
> >> >> >
> >> >> >-              if (buf == NULL) {
> >> >> >-                      err = -errno;
> >> >> >-                      goto error;
> >> >> >-              }
> >> >> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
> >> >> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
> >> >> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
> >> >> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
> >> >>
> >> >> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
> >> >> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> >> >
> >> >Is there a particular use-case for explicitly forcing C90?
> >>
> >> not forcing C90, but forcing -Wdeclaration-after-statement as per
> >> flag passed in configure.ac. I think the message given by gcc is
> >> misleading here.
> >>
> >
> >Indeed thanks. Seems like I should add `export CFLAGS+=-Werror" to my dev box.
>
> so, are you ok that I just squash this?
>

Yes, of course.

-Emil

Re: [PATCH kmod 09/13] libkmod: swap alloca usage for a few assert_cc

[Lucas De Marchi]

On Tue, Apr 30, 2024 at 07:47:46PM GMT, Emil Velikov wrote:
>On Tue, 30 Apr 2024 at 19:43, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Tue, Apr 30, 2024 at 07:27:00PM GMT, Emil Velikov wrote:
>> >On Tue, 30 Apr 2024 at 19:18, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >>
>> >> On Tue, Apr 30, 2024 at 06:54:00PM GMT, Emil Velikov wrote:
>> >> >On Tue, 30 Apr 2024 at 18:39, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >> >>
>> >> >> On Mon, Feb 12, 2024 at 05:23:10PM GMT, Emil Velikov via B4 Relay wrote:
>> >> >> >From: Emil Velikov <emil.l.velikov@gmail.com>
>> >> >> >
>> >> >> >Since all the compression magic is always available now, we don't need
>> >> >> >to loop at runtime nor use alloca - latter of which comes with a handful
>> >> >> >of caveats.
>> >> >> >
>> >> >> >Simply throw in a few assert_cc(), which will trigger at build-time.
>> >> >> >
>> >> >> >Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> >> >> >---
>> >> >> > libkmod/libkmod-file.c | 22 ++++++++--------------
>> >> >> > 1 file changed, 8 insertions(+), 14 deletions(-)
>> >> >> >
>> >> >> >diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
>> >> >> >index b69f1ef..5b88d6c 100644
>> >> >> >--- a/libkmod/libkmod-file.c
>> >> >> >+++ b/libkmod/libkmod-file.c
>> >> >> >@@ -410,7 +410,6 @@ 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 = 0;
>> >> >> >
>> >> >> >       if (file == NULL)
>> >> >> >@@ -422,22 +421,17 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
>> >> >> >               goto error;
>> >> >> >       }
>> >> >> >
>> >> >> >-      for (itr = comp_types; itr->load != NULL; itr++) {
>> >> >> >-              if (magic_size_max < itr->magic_size)
>> >> >> >-                      magic_size_max = itr->magic_size;
>> >> >> >-      }
>> >> >> >-
>> >> >> >-      if (magic_size_max > 0) {
>> >> >> >-              char *buf = alloca(magic_size_max + 1);
>> >> >> >+      {
>> >> >> >+              char buf[7];
>> >> >> >               ssize_t sz;
>> >> >> >
>> >> >> >-              if (buf == NULL) {
>> >> >> >-                      err = -errno;
>> >> >> >-                      goto error;
>> >> >> >-              }
>> >> >> >-              sz = read_str_safe(file->fd, buf, magic_size_max + 1);
>> >> >> >+              assert_cc(sizeof(magic_zstd) < sizeof(buf));
>> >> >> >+              assert_cc(sizeof(magic_xz) < sizeof(buf));
>> >> >> >+              assert_cc(sizeof(magic_zlib) < sizeof(buf));
>> >> >>
>> >> >> ../libkmod/libkmod-file.c: In function 'kmod_file_open':
>> >> >> ../shared/macro.h:25:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>> >> >
>> >> >Is there a particular use-case for explicitly forcing C90?
>> >>
>> >> not forcing C90, but forcing -Wdeclaration-after-statement as per
>> >> flag passed in configure.ac. I think the message given by gcc is
>> >> misleading here.
>> >>
>> >
>> >Indeed thanks. Seems like I should add `export CFLAGS+=-Werror" to my dev box.
>>
>> so, are you ok that I just squash this?
>>
>
>Yes, of course.

thanks, pushed everything except the last patch.

Lucas De Marchi

>
>-Emil