[LTP] [PATCH v3 0/2] kernel module detection (own implementation)

[LTP] [PATCH v3 0/2] kernel module detection (own implementation)

[Petr Vorel]

Hi,

changes v2->v3:
* treat '-' and '_' as the same (follow kmod implementation)
* drop unused MODULES_DIR definition
* code readability (more use of SAFE_ASPRINTF())

It'd deserve a test.

Kind regards,
Petr


Petr Vorel (2):
  lib: Fix kernel module detection on BusyBox
  zram: Fix module detection on BusyBox

 lib/tst_kernel.c                              | 99 ++++++++++++++++---
 .../kernel/device-drivers/zram/zram_lib.sh    |  6 +-
 2 files changed, 89 insertions(+), 16 deletions(-)

-- 
2.30.0

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Petr Vorel]

BusyBox modprobe implementation does not support -n switch.

It does support -D, which could be used, *but* unless is busybox binary
configured with CONFIG_MODPROBE_SMALL=y (IMHO the default).

We could use modinfo and grep output for 'filename:', but we agreed on
ML that having our own implementation will be the best as it also
does not require modinfo as external dependency.

Implementation searches for for module presence in /lib/modules/$(uname
-r)/modules.{dep,builtin}. On Android expect files in /system/lib/modules
directory.

Also treat '-' and '_' in module name as the same (follow kmod implementation).

On Android still assume all drivers are available because modules.* files might
not be available. We could search modules in /system/lib/modules, but to
to determine built-in drivers we need modules.builtin (it's required
also by Busybox mod{info,probe} implementation).

This fixes many tests on BusyBox, e.g. *all* network tests (tests using
tst_net.sh) after 305a78e4c ("tst_net.sh: Require veth for netns").

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_kernel.c | 99 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 87 insertions(+), 12 deletions(-)

diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
index 57fa4b2be..279c8936c 100644
--- a/lib/tst_kernel.c
+++ b/lib/tst_kernel.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (c) 2020-2021 Petr Vorel <pvorel@suse.cz>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -17,8 +18,11 @@
 
 #include <sys/personality.h>
 #include <sys/utsname.h>
+#include <limits.h>
+
 #include "test.h"
 #include "tst_kernel.h"
+#include "old_safe_stdio.h"
 
 static int get_kernel_bits_from_uname(struct utsname *buf)
 {
@@ -81,20 +85,91 @@ int tst_kernel_bits(void)
 	return kernel_bits;
 }
 
-int tst_check_driver(const char *name)
+int tst_search_driver(const char *driver, const char *file)
 {
-#ifndef __ANDROID__
-	const char * const argv[] = { "modprobe", "-n", name, NULL };
-	int res = tst_cmd_(NULL, argv, "/dev/null", "/dev/null",
-			       TST_CMD_PASS_RETVAL);
-
-	/* 255 - it looks like modprobe not available */
-	return (res == 255) ? 0 : res;
-#else
-	/* Android modprobe may not have '-n', or properly installed
-	 * module.*.bin files to determine built-in drivers. Assume
-	 * all drivers are available.
+	struct stat st;
+	char *path = NULL, *search = NULL;
+	char buf[PATH_MAX], module[PATH_MAX];
+	FILE *f;
+
+	struct utsname uts;
+
+	if (uname(&uts)) {
+		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
+		return -1;
+	}
+	SAFE_ASPRINTF(NULL, &path, "/lib/modules/%s/%s", uts.release, file);
+
+	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
+		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
+		return -1;
+	}
+
+	if (access(path, R_OK)) {
+		tst_resm(TWARN, "file %s cannot be read", path);
+		return -1;
+	}
+
+	SAFE_ASPRINTF(NULL, &search, "/%s.ko", driver);
+
+	f = SAFE_FOPEN(NULL, path, "r");
+
+	while (fgets(buf, sizeof(buf), f)) {
+		if (sscanf(buf, "%s", module) != 1)
+			continue;
+
+		if (strstr(module, search) != NULL) {
+			SAFE_FCLOSE(NULL, f);
+			return 0;
+		}
+	}
+
+	SAFE_FCLOSE(NULL, f);
+
+	return -1;
+}
+
+int tst_check_driver_(const char *driver)
+{
+	if (!tst_search_driver(driver, "modules.dep") ||
+		!tst_search_driver(driver, "modules.builtin"))
+		return 0;
+
+	return 1;
+}
+
+int tst_check_driver(const char *driver)
+{
+#ifdef __ANDROID__
+	/*
+	 * Android may not have properly installed modules.* files. We could
+	 * search modules in /system/lib/modules, but to to determine built-in
+	 * drivers we need modules.builtin. Therefore assume all drivers are
+	 * available.
 	 */
 	return 0;
 #endif
+
+	if (!tst_check_driver_(driver))
+		return 0;
+
+	if (strrchr(driver, '-') || strrchr(driver, '_')) {
+		char *driver2 = strdup(driver);
+		char *ix = driver2;
+		char find = '-', replace = '_';
+
+		if (strrchr(driver, '_')) {
+			find = '_';
+			replace = '-';
+		}
+
+		while ((ix = strchr(ix, find)) != NULL) {
+			*ix++ = replace;
+		}
+
+		if (!tst_check_driver_(driver2))
+			return 0;
+	}
+
+	return 1;
 }
-- 
2.30.0

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Sandeep Patil]

On Tue, Jan 19, 2021 at 05:03:15PM +0100, Petr Vorel wrote:
> BusyBox modprobe implementation does not support -n switch.
> 
> It does support -D, which could be used, *but* unless is busybox binary
> configured with CONFIG_MODPROBE_SMALL=y (IMHO the default).
> 
> We could use modinfo and grep output for 'filename:', but we agreed on
> ML that having our own implementation will be the best as it also
> does not require modinfo as external dependency.
> 
> Implementation searches for for module presence in /lib/modules/$(uname
> -r)/modules.{dep,builtin}. On Android expect files in /system/lib/modules
> directory.
> 
> Also treat '-' and '_' in module name as the same (follow kmod implementation).
> 
> On Android still assume all drivers are available because modules.* files might
> not be available. We could search modules in /system/lib/modules, but to
> to determine built-in drivers we need modules.builtin (it's required
> also by Busybox mod{info,probe} implementation).
> 
> This fixes many tests on BusyBox, e.g. *all* network tests (tests using
> tst_net.sh) after 305a78e4c ("tst_net.sh: Require veth for netns").
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  lib/tst_kernel.c | 99 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 87 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> index 57fa4b2be..279c8936c 100644
> --- a/lib/tst_kernel.c
> +++ b/lib/tst_kernel.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz>
> + * Copyright (c) 2020-2021 Petr Vorel <pvorel@suse.cz>
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -17,8 +18,11 @@
>  
>  #include <sys/personality.h>
>  #include <sys/utsname.h>
> +#include <limits.h>
> +
>  #include "test.h"
>  #include "tst_kernel.h"
> +#include "old_safe_stdio.h"
>  
>  static int get_kernel_bits_from_uname(struct utsname *buf)
>  {
> @@ -81,20 +85,91 @@ int tst_kernel_bits(void)
>  	return kernel_bits;
>  }
>  
> -int tst_check_driver(const char *name)
> +int tst_search_driver(const char *driver, const char *file)
>  {
> -#ifndef __ANDROID__
> -	const char * const argv[] = { "modprobe", "-n", name, NULL };
> -	int res = tst_cmd_(NULL, argv, "/dev/null", "/dev/null",
> -			       TST_CMD_PASS_RETVAL);
> -
> -	/* 255 - it looks like modprobe not available */
> -	return (res == 255) ? 0 : res;
> -#else
> -	/* Android modprobe may not have '-n', or properly installed
> -	 * module.*.bin files to determine built-in drivers. Assume
> -	 * all drivers are available.
> +	struct stat st;
> +	char *path = NULL, *search = NULL;
> +	char buf[PATH_MAX], module[PATH_MAX];
> +	FILE *f;
> +
> +	struct utsname uts;
> +
> +	if (uname(&uts)) {
> +		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
> +		return -1;
> +	}
> +	SAFE_ASPRINTF(NULL, &path, "/lib/modules/%s/%s", uts.release, file);

This is just the ramdisk location, the on-disk location is
/vendor/lib/modules/. I also think that the ramdisk one goes away after we
switch over 2nd stage init. Is there a test I can run that uses these
functions now to make sure this works?

Also, unfortunately (and sadly) we may have to do something Android specific
downstream as the /vendor/lib/modules and /lib/modules only started to appear
as of android 11 :(.

Once you share how I can test, I'm happy to test and add my Tested-by for
Android.

+cc: kernel-team@android.com
> +
> +	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
> +		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
> +		return -1;
> +	}
> +
> +	if (access(path, R_OK)) {
> +		tst_resm(TWARN, "file %s cannot be read", path);
> +		return -1;
> +	}
> +
> +	SAFE_ASPRINTF(NULL, &search, "/%s.ko", driver);
> +
> +	f = SAFE_FOPEN(NULL, path, "r");
> +
> +	while (fgets(buf, sizeof(buf), f)) {
> +		if (sscanf(buf, "%s", module) != 1)
> +			continue;
> +
> +		if (strstr(module, search) != NULL) {
> +			SAFE_FCLOSE(NULL, f);
> +			return 0;
> +		}
> +	}
> +
> +	SAFE_FCLOSE(NULL, f);
> +
> +	return -1;
> +}
> +
> +int tst_check_driver_(const char *driver)
> +{
> +	if (!tst_search_driver(driver, "modules.dep") ||
> +		!tst_search_driver(driver, "modules.builtin"))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +int tst_check_driver(const char *driver)
> +{
> +#ifdef __ANDROID__
> +	/*
> +	 * Android may not have properly installed modules.* files. We could
> +	 * search modules in /system/lib/modules, but to to determine built-in

the appropriate location would be /lib/modules OR /vendor/lib/modules.
> +	 * drivers we need modules.builtin. Therefore assume all drivers are
> +	 * available.
>  	 */
>  	return 0;
>  #endif
> +
> +	if (!tst_check_driver_(driver))
> +		return 0;
> +
> +	if (strrchr(driver, '-') || strrchr(driver, '_')) {
> +		char *driver2 = strdup(driver);
> +		char *ix = driver2;
> +		char find = '-', replace = '_';
> +
> +		if (strrchr(driver, '_')) {
> +			find = '_';
> +			replace = '-';
> +		}
> +
> +		while ((ix = strchr(ix, find)) != NULL) {
> +			*ix++ = replace;
> +		}
> +
> +		if (!tst_check_driver_(driver2))
> +			return 0;
> +	}
> +
> +	return 1;
>  }
> -- 
> 2.30.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Petr Vorel]

Hi Sandeep,

thanks for your comments!

> > +	SAFE_ASPRINTF(NULL, &path, "/lib/modules/%s/%s", uts.release, file);

> This is just the ramdisk location, the on-disk location is
> /vendor/lib/modules/. I also think that the ramdisk one goes away after we
> switch over 2nd stage init. Is there a test I can run that uses these
> functions now to make sure this works?
Any C based test which defines needs_drivers (e.g. fsetxattr02, ioctl08,
uevent01, ...) (or shell based tests with TST_NEEDS_DRIVERS, but you probably
don't run any shell test).

> Also, unfortunately (and sadly) we may have to do something Android specific
> downstream as the /vendor/lib/modules and /lib/modules only started to appear
> as of android 11 :(.
Feel free to send a patch upstream. If not that much complicated and you're
willing to maintain it, it might get to upstream (depends on other maintainers,
but we're quite open).

> Once you share how I can test, I'm happy to test and add my Tested-by for
> Android.

> +cc: kernel-team@android.com
> > +
> > +	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
> > +		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
> > +		return -1;
> > +	}
> > +
> > +	if (access(path, R_OK)) {
> > +		tst_resm(TWARN, "file %s cannot be read", path);
> > +		return -1;
> > +	}
> > +
> > +	SAFE_ASPRINTF(NULL, &search, "/%s.ko", driver);
> > +
> > +	f = SAFE_FOPEN(NULL, path, "r");
> > +
> > +	while (fgets(buf, sizeof(buf), f)) {
> > +		if (sscanf(buf, "%s", module) != 1)
> > +			continue;
> > +
> > +		if (strstr(module, search) != NULL) {
> > +			SAFE_FCLOSE(NULL, f);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	SAFE_FCLOSE(NULL, f);
> > +
> > +	return -1;
> > +}
> > +
> > +int tst_check_driver_(const char *driver)
> > +{
> > +	if (!tst_search_driver(driver, "modules.dep") ||
> > +		!tst_search_driver(driver, "modules.builtin"))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> > +int tst_check_driver(const char *driver)
> > +{
> > +#ifdef __ANDROID__
> > +	/*
> > +	 * Android may not have properly installed modules.* files. We could
> > +	 * search modules in /system/lib/modules, but to to determine built-in

> the appropriate location would be /lib/modules OR /vendor/lib/modules.
OK. I tested only old aosp (Oreo, Nougat and KitKat).
Out of curiosity, does have Android 11 modules.{builtin,dep}? If yes, it'd make
sense to treat it as Linux (apply "modules always available" approach only if
files aren't available).

Kind regards,
Petr

> > +	 * drivers we need modules.builtin. Therefore assume all drivers are
> > +	 * available.
> >  	 */
> >  	return 0;
> >  #endif

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Petr Vorel]

Hi,

...
> +int tst_search_driver(const char *driver, const char *file)
As Joerg pointed out, this must be static.

...
> +int tst_check_driver_(const char *driver)
And this one as well.
> +{
> +	if (!tst_search_driver(driver, "modules.dep") ||
> +		!tst_search_driver(driver, "modules.builtin"))
> +		return 0;
> +
> +	return 1;
> +}
...

Kind regards,
Petr

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Cyril Hrubis]

Hi!
>  #include <sys/personality.h>
>  #include <sys/utsname.h>
> +#include <limits.h>
> +
>  #include "test.h"
>  #include "tst_kernel.h"
> +#include "old_safe_stdio.h"
>  
>  static int get_kernel_bits_from_uname(struct utsname *buf)
>  {
> @@ -81,20 +85,91 @@ int tst_kernel_bits(void)
>  	return kernel_bits;
>  }
>  
> -int tst_check_driver(const char *name)
> +int tst_search_driver(const char *driver, const char *file)
>  {
> -#ifndef __ANDROID__
> -	const char * const argv[] = { "modprobe", "-n", name, NULL };
> -	int res = tst_cmd_(NULL, argv, "/dev/null", "/dev/null",
> -			       TST_CMD_PASS_RETVAL);
> -
> -	/* 255 - it looks like modprobe not available */
> -	return (res == 255) ? 0 : res;
> -#else
> -	/* Android modprobe may not have '-n', or properly installed
> -	 * module.*.bin files to determine built-in drivers. Assume
> -	 * all drivers are available.
> +	struct stat st;
> +	char *path = NULL, *search = NULL;
> +	char buf[PATH_MAX], module[PATH_MAX];
> +	FILE *f;
> +
> +	struct utsname uts;
> +
> +	if (uname(&uts)) {
> +		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
> +		return -1;
> +	}
> +	SAFE_ASPRINTF(NULL, &path, "/lib/modules/%s/%s", uts.release, file);
> +
> +	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
> +		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
> +		return -1;
> +	}
> +
> +	if (access(path, R_OK)) {
> +		tst_resm(TWARN, "file %s cannot be read", path);
> +		return -1;
> +	}
> +
> +	SAFE_ASPRINTF(NULL, &search, "/%s.ko", driver);
> +
> +	f = SAFE_FOPEN(NULL, path, "r");
> +
> +	while (fgets(buf, sizeof(buf), f)) {
> +		if (sscanf(buf, "%s", module) != 1)
> +			continue;

What is the point in the sscanf() here?

Why can't we just strstr(buf, search) here?

> +		if (strstr(module, search) != NULL) {
> +			SAFE_FCLOSE(NULL, f);
> +			return 0;
> +		}
> +	}
> +
> +	SAFE_FCLOSE(NULL, f);
> +
> +	return -1;
> +}
> +
> +int tst_check_driver_(const char *driver)
> +{
> +	if (!tst_search_driver(driver, "modules.dep") ||
> +		!tst_search_driver(driver, "modules.builtin"))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +int tst_check_driver(const char *driver)
> +{
> +#ifdef __ANDROID__
> +	/*
> +	 * Android may not have properly installed modules.* files. We could
> +	 * search modules in /system/lib/modules, but to to determine built-in
> +	 * drivers we need modules.builtin. Therefore assume all drivers are
> +	 * available.
>  	 */
>  	return 0;
>  #endif
> +
> +	if (!tst_check_driver_(driver))
> +		return 0;
> +
> +	if (strrchr(driver, '-') || strrchr(driver, '_')) {
> +		char *driver2 = strdup(driver);
> +		char *ix = driver2;
> +		char find = '-', replace = '_';
> +
> +		if (strrchr(driver, '_')) {
> +			find = '_';
> +			replace = '-';
> +		}
> +
> +		while ((ix = strchr(ix, find)) != NULL) {
> +			*ix++ = replace;
> +		}

Just:
		while ((ix = strchr(ix, find))
			*ix++ = replace;

> +		if (!tst_check_driver_(driver2))

free(driver2) ?

> +			return 0;
> +	}
> +
> +	return 1;
>  }
> -- 
> 2.30.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Petr Vorel]

Hi Cyril,

...
> > +	while (fgets(buf, sizeof(buf), f)) {
> > +		if (sscanf(buf, "%s", module) != 1)
> > +			continue;

> What is the point in the sscanf() here?

> Why can't we just strstr(buf, search) here?

modules.dep has format:
module:[dependency [another-dependency ...]]

e.g.:
kernel/arch/x86/kernel/cpu/mce/mce-inject.ko.xz:
kernel/arch/x86/crypto/twofish-x86_64.ko.xz: kernel/crypto/twofish_common.ko.xz
kernel/arch/x86/crypto/aesni-intel.ko.xz: kernel/crypto/crypto_simd.ko.xz kernel/crypto/cryptd.ko.xz kernel/arch/x86/crypto/glue_helper.ko.xz

First reading "%s" reads only first module with ':' separator.
Searching without it could find first module which is as dependency (e.g.
"/twofish_common.ko.xz" instead of "/twofish-x86_64.ko.xz"). Although that
shouldn't be a problem, because it's very unlikely that module dependency is
missing. Do you want me to drop sscanf() or put some comment?

Also man modules.dep(5) warns about using text format as "their format is
subject to change in the future". Hopefully we can depend on it. Or should we
use binary format?

...
> > +	if (!tst_check_driver_(driver))
> > +		return 0;
> > +
> > +	if (strrchr(driver, '-') || strrchr(driver, '_')) {
> > +		char *driver2 = strdup(driver);
> > +		char *ix = driver2;
> > +		char find = '-', replace = '_';
> > +
> > +		if (strrchr(driver, '_')) {
> > +			find = '_';
> > +			replace = '-';
> > +		}
> > +
> > +		while ((ix = strchr(ix, find)) != NULL) {
> > +			*ix++ = replace;
> > +		}

> Just:
> 		while ((ix = strchr(ix, find))
> 			*ix++ = replace;

> > +		if (!tst_check_driver_(driver2))

> free(driver2) ?

Oops, you're right. + path and search path in tst_search_driver()
Below are new versions.

Kind regards,
Petr

static int tst_search_driver(const char *driver, const char *file)
{
	struct stat st;
	char *path = NULL, *search = NULL;
	char buf[PATH_MAX], module[PATH_MAX];
	FILE *f;
	int ret = -1;

	struct utsname uts;

	if (uname(&uts)) {
		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
		return -1;
	}
	SAFE_ASPRINTF(NULL, &path, "/lib/modules/%s/%s", uts.release, file);

	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
		return -1;
	}

	if (access(path, R_OK)) {
		tst_resm(TWARN, "file %s cannot be read", path);
		return -1;
	}

	SAFE_ASPRINTF(NULL, &search, "/%s.ko", driver);

	f = SAFE_FOPEN(NULL, path, "r");

	while (fgets(buf, sizeof(buf), f)) {
		if (sscanf(buf, "%s", module) != 1)
			continue;

		if (strstr(module, search) != NULL) {
			ret = 0;
			break;
		}
	}

	SAFE_FCLOSE(NULL, f);
	free(search);
	free(path);

	return ret;
}

static int tst_check_driver_(const char *driver)
{
	if (!tst_search_driver(driver, "modules.dep") ||
		!tst_search_driver(driver, "modules.builtin"))
		return 0;

	return 1;
}

int tst_check_driver(const char *driver)
{
#ifdef __ANDROID__
	/*
	 * Android may not have properly installed modules.* files. We could
	 * search modules in /system/lib/modules, but to to determine built-in
	 * drivers we need modules.builtin. Therefore assume all drivers are
	 * available.
	 */
	return 0;
#endif

	if (!tst_check_driver_(driver))
		return 0;

	int ret = 1;

	if (strrchr(driver, '-') || strrchr(driver, '_')) {
		char *driver2 = strdup(driver);
		char *ix = driver2;
		char find = '-', replace = '_';

		if (strrchr(driver, '_')) {
			find = '_';
			replace = '-';
		}

		while ((ix = strchr(ix, find)))
			*ix++ = replace;

		ret = tst_check_driver_(driver2);
		free(driver2);
	}

	return ret;
}

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Cyril Hrubis]

Hi!
> modules.dep has format:
> module:[dependency [another-dependency ...]]
> 
> e.g.:
> kernel/arch/x86/kernel/cpu/mce/mce-inject.ko.xz:
> kernel/arch/x86/crypto/twofish-x86_64.ko.xz: kernel/crypto/twofish_common.ko.xz
> kernel/arch/x86/crypto/aesni-intel.ko.xz: kernel/crypto/crypto_simd.ko.xz kernel/crypto/cryptd.ko.xz kernel/arch/x86/crypto/glue_helper.ko.xz
> 
> First reading "%s" reads only first module with ':' separator.
> Searching without it could find first module which is as dependency (e.g.
> "/twofish_common.ko.xz" instead of "/twofish-x86_64.ko.xz"). Although that
> shouldn't be a problem, because it's very unlikely that module dependency is
> missing. Do you want me to drop sscanf() or put some comment?

Well it would be probably cleaner to do something as:

	/* Cut dependencies after : */
	if ((sep = strchr(buf, ':')))
		*sep = 0;

No need to copy the string just because we neet to cut part of it.

> Also man modules.dep(5) warns about using text format as "their format is
> subject to change in the future". Hopefully we can depend on it. Or should we
> use binary format?

That depends on how complicated is to parse the binary format...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] lib: Fix kernel module detection on BusyBox

[Petr Vorel]

Hi,

> > modules.dep has format:
> > module:[dependency [another-dependency ...]]

> > e.g.:
> > kernel/arch/x86/kernel/cpu/mce/mce-inject.ko.xz:
> > kernel/arch/x86/crypto/twofish-x86_64.ko.xz: kernel/crypto/twofish_common.ko.xz
> > kernel/arch/x86/crypto/aesni-intel.ko.xz: kernel/crypto/crypto_simd.ko.xz kernel/crypto/cryptd.ko.xz kernel/arch/x86/crypto/glue_helper.ko.xz

> > First reading "%s" reads only first module with ':' separator.
> > Searching without it could find first module which is as dependency (e.g.
> > "/twofish_common.ko.xz" instead of "/twofish-x86_64.ko.xz"). Although that
> > shouldn't be a problem, because it's very unlikely that module dependency is
> > missing. Do you want me to drop sscanf() or put some comment?

> Well it would be probably cleaner to do something as:

> 	/* Cut dependencies after : */
> 	if ((sep = strchr(buf, ':')))
> 		*sep = 0;

> No need to copy the string just because we neet to cut part of it.
+1, thanks!

> > Also man modules.dep(5) warns about using text format as "their format is
> > subject to change in the future". Hopefully we can depend on it. Or should we
> > use binary format?

> That depends on how complicated is to parse the binary format...
I'll have a look.

I was also thinking whether whole thing really works on toolchains which use
BusyBox. BusyBox's depmod implementation does not generate modules.builtin
(it generates only modules.{alias,dep,symbols}, i.e. only few text files, no
binary). But the only distro I know which installs BusyBox depmod/modinfo/modprobe
instead of kmod is Alpine Linux and even this distro clearly generates all
modules.* files for its kernel package with kmod.

It looks to me that kernel supports generating modules.builtin.modinfo
since v5.2-rc1 (898490c010b5 "moduleparam: Save information about built-in
modules in separate file") and kmod used this change in v27 (in 60084cf
"libkmod: Add parser for modules.builtin.modinfo" released a year ago).
Thus built-in modules are impossible to detect on old kernels no matter we use
kmod or our own implementation.

But built-in modules are mostly problem for embedded: we require modules like
btrfs, loop, tun, uinput, ...  which are loadable modules on typical linux
distros.

Thus generally using modules.{dep,builtin} seems to me as a good approach for
linux distros including embedded ones and we could also use binary variants.

Other possible improvements:
* last resort effort search in /lib/modules/$(uname -r) or a directory defined
in environment variable (with or without version) - fallback for old android or
some embedded (again - not working for built-in modules)
* environment variable to skip the detection.

Kind regards,
Petr

[LTP] [PATCH v3 2/2] zram: Fix module detection on BusyBox

[Petr Vorel]

BusyBox modinfo implementation does not exit with 0 when module not
found. Our own API implementation used for module detection in
tst_check_driver() was fixed in previous commit thus use it.

Reported-by: Leo Yu-Chi Liang <ycliang@andestech.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/device-drivers/zram/zram_lib.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 3f4d1d55f..bdbf2453a 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.
-# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2019-2021 Petr Vorel <pvorel@suse.cz>
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 
 dev_makeswap=-1
@@ -9,6 +9,7 @@ dev_mounted=-1
 TST_NEEDS_TMPDIR=1
 TST_SETUP="zram_load"
 TST_CLEANUP="zram_cleanup"
+TST_NEEDS_DRIVERS="zram"
 . tst_test.sh
 
 zram_cleanup()
@@ -210,6 +211,3 @@ zram_mount()
 
 	tst_res TPASS "mount of zram device(s) succeeded"
 }
-
-modinfo zram > /dev/null 2>&1 ||
-	tst_brk TCONF "zram not configured in kernel"
-- 
2.30.0