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

[LTP] [PATCH v2 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.

On Android still assume all drivers are available as config files might
not be available).

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>
---
Changes v1->v2:
* Check also modules.builtin (built-in dependency).

Kind regards,
Petr

 lib/tst_kernel.c | 81 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 9 deletions(-)

diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
index 57fa4b2be..ab324a643 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) 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,79 @@ int tst_kernel_bits(void)
 	return kernel_bits;
 }
 
-int tst_check_driver(const char *name)
+#ifndef __ANDROID__
+# define MODULES_DIR "/lib/modules"
+#else
+# define MODULES_DIR "/system/lib/modules"
+#endif
+
+
+int tst_search_driver(const char *driver, const char *file)
 {
+	struct stat st;
+	char *path = NULL;
+	char buf[PATH_MAX], module[PATH_MAX], search[PATH_MAX] = "/";
+	FILE *f;
+
 #ifndef __ANDROID__
-	const char * const argv[] = { "modprobe", "-n", name, NULL };
-	int res = tst_cmd_(NULL, argv, "/dev/null", "/dev/null",
-			       TST_CMD_PASS_RETVAL);
+	struct utsname uts;
 
-	/* 255 - it looks like modprobe not available */
-	return (res == 255) ? 0 : res;
+	if (uname(&uts)) {
+		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
+		return -1;
+	}
+	SAFE_ASPRINTF(NULL, &path, "%s/%s/%s", MODULES_DIR, uts.release, file);
 #else
-	/* Android modprobe may not have '-n', or properly installed
-	 * module.*.bin files to determine built-in drivers. Assume
-	 * all drivers are available.
+	SAFE_ASPRINTF(NULL, &path, "%s/%s", MODULES_DIR, file);
+#endif
+
+	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
+#ifndef __ANDROID__
+		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
+#endif
+		return -1;
+	}
+
+	if (access(path, R_OK)) {
+#ifndef __ANDROID__
+		tst_resm(TWARN, "file %s cannot be read", path);
+#endif
+		return -1;
+	}
+
+	strcat(search, driver);
+	strcat(search, ".ko");
+
+	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)
+{
+#ifdef __ANDROID__
+	/*
+	 * Android may not have properly installed modules.* files to determine
+	 * built-in drivers. Assume all drivers are available.
 	 */
 	return 0;
 #endif
+
+	if (!tst_search_driver(driver, "modules.dep") ||
+		!tst_search_driver(driver, "modules.builtin"))
+		return 0;
+
+	return 1;
 }
-- 
2.29.2

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

[Petr Vorel]

Hi,

> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
...
> -int tst_check_driver(const char *name)
> +#ifndef __ANDROID__
> +# define MODULES_DIR "/lib/modules"
> +#else
> +# define MODULES_DIR "/system/lib/modules"
> +#endif

OK, MODULES_DIR is not needed now as I kept Android skipped (unless somebody
contributes code or share algorithm for Android).

Kind regards,
Petr

[LTP] [PATCH v2 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,79 @@ int tst_kernel_bits(void)
>  	return kernel_bits;
>  }
>  
> -int tst_check_driver(const char *name)
> +#ifndef __ANDROID__
> +# define MODULES_DIR "/lib/modules"
> +#else
> +# define MODULES_DIR "/system/lib/modules"
> +#endif
> +
> +
> +int tst_search_driver(const char *driver, const char *file)
>  {
> +	struct stat st;
> +	char *path = NULL;
> +	char buf[PATH_MAX], module[PATH_MAX], search[PATH_MAX] = "/";
> +	FILE *f;
> +
>  #ifndef __ANDROID__
> -	const char * const argv[] = { "modprobe", "-n", name, NULL };
> -	int res = tst_cmd_(NULL, argv, "/dev/null", "/dev/null",
> -			       TST_CMD_PASS_RETVAL);
> +	struct utsname uts;
>  
> -	/* 255 - it looks like modprobe not available */
> -	return (res == 255) ? 0 : res;
> +	if (uname(&uts)) {
> +		tst_brkm(TBROK | TERRNO, NULL, "uname() failed");
> +		return -1;
> +	}
> +	SAFE_ASPRINTF(NULL, &path, "%s/%s/%s", MODULES_DIR, uts.release, file);
>  #else
> -	/* Android modprobe may not have '-n', or properly installed
> -	 * module.*.bin files to determine built-in drivers. Assume
> -	 * all drivers are available.
> +	SAFE_ASPRINTF(NULL, &path, "%s/%s", MODULES_DIR, file);
> +#endif
> +
> +	if (stat(path, &st) || !(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) {
> +#ifndef __ANDROID__
> +		tst_resm(TWARN, "expected file %s does not exist or not a file", path);
> +#endif
> +		return -1;
> +	}
> +
> +	if (access(path, R_OK)) {
> +#ifndef __ANDROID__
> +		tst_resm(TWARN, "file %s cannot be read", path);
> +#endif
> +		return -1;
> +	}
> +
> +	strcat(search, driver);
> +	strcat(search, ".ko");

Why not just snprintf() or SAFE_ASPRINTF() here as well?

> +	f = SAFE_FOPEN(NULL, path, "r");
> +
> +	while (fgets(buf, sizeof(buf), f)) {
> +		if (sscanf(buf, "%s", module) != 1)
> +			continue;
> +
> +		if (strstr(module, search) != NULL) {

And I'm not sure that this is safe either, what about the case that one
module name is a substring of another.

E.g. if we look for "foo.ko" and the file contains "this_is_not_foo.ko"
it will still match here.


Also this seems to be rather distruptive change, so I guess it would be
safer to apply after the release.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Petr Vorel]

Hi Cyril,

> > +	char buf[PATH_MAX], module[PATH_MAX], search[PATH_MAX] = "/";
...
> > +	strcat(search, driver);
> > +	strcat(search, ".ko");

> Why not just snprintf() or SAFE_ASPRINTF() here as well?
+1

> > +	f = SAFE_FOPEN(NULL, path, "r");
> > +
> > +	while (fgets(buf, sizeof(buf), f)) {
> > +		if (sscanf(buf, "%s", module) != 1)
> > +			continue;
> > +
> > +		if (strstr(module, search) != NULL) {

> And I'm not sure that this is safe either, what about the case that one
> module name is a substring of another.

> E.g. if we look for "foo.ko" and the file contains "this_is_not_foo.ko"
> it will still match here.
char search[PATH_MAX] = "/"; => we search for "/foo.ko"
But that will be more obvious when I use SAFE_ASPRINTF() for search.

> Also this seems to be rather distruptive change, so I guess it would be
> safer to apply after the release.

I'll send v3, but no problem to postpone it.

But I'll revert 305a78e4c ("tst_net.sh: Require veth for netns") with
explanation that it wait for this fix, ok?

Kind regards,
Petr

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

[Joerg Vehlow]

Hi,

On 1/19/2021 8:41 AM, Petr Vorel wrote:
> Hi,
>
>> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> ...
>> -int tst_check_driver(const char *name)
>> +#ifndef __ANDROID__
>> +# define MODULES_DIR "/lib/modules"
>> +#else
>> +# define MODULES_DIR "/system/lib/modules"
>> +#endif
> OK, MODULES_DIR is not needed now as I kept Android skipped (unless somebody
> contributes code or share algorithm for Android).
I don't get this comment. MODULES_DIR is used in both code paths in 
tst_search_driver.
But you don't call it from tst_check_driver only if it is not android.
If tst_search_driver is supposed to be a new public interface, it should 
be added to the header,
otherwise it should be marked static.

J?rg

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

[Petr Vorel]

Hi J?rg,

> Hi,

> On 1/19/2021 8:41 AM, Petr Vorel wrote:
> > Hi,

> > > diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> > ...
> > > -int tst_check_driver(const char *name)
> > > +#ifndef __ANDROID__
> > > +# define MODULES_DIR "/lib/modules"
> > > +#else
> > > +# define MODULES_DIR "/system/lib/modules"
> > > +#endif
> > OK, MODULES_DIR is not needed now as I kept Android skipped (unless somebody
> > contributes code or share algorithm for Android).
> I don't get this comment. MODULES_DIR is used in both code paths in
> tst_search_driver.
Please have look at v3, where I removed MODULES_DIR
https://patchwork.ozlabs.org/project/ltp/patch/20210119160316.4776-2-pvorel@suse.cz/

> But you don't call it from tst_check_driver only if it is not android.
> If tst_search_driver is supposed to be a new public interface, it should be
> added to the header,
> otherwise it should be marked static.
+1 I'll make it static.

Kind regards,
Petr

> J?rg