[LTP] [PATCH 0/4] Add .modprobe (loading modules in C API)

[LTP] [PATCH 0/4] Add .modprobe (loading modules in C API)

[Petr Vorel]

Hi,

when I started this patch it looked to me that quite a few C tests executes
modprobe to load kernel module. In the end, so far only can_bcm01.c and
madvise11.c uses it. But maybe more could be used.

If I add support for module parameters (it would be easy to add), it could be
used also in can_common.h testcases/network/can/filter-tests/.

It could also be used in the old API C tests in testcases/kernel/input (which
use input_helper.c), of course after we rewrite them to the new API.

Tests which use modprobe but using .modprobe is not usable:
* kvm_pagefault01.c (more complicated use - it requires checks).
* zram03.c, zram_lib.sh (too complicated check due /sys/class/zram-control
  introduced in v4.2-rc1 vs. the old API, but maybe this could be simplified)
* netstress.c (used only when testing dccp, which is determined by getopts)

But if we move code I put into tst_test.c into separate function, we could be
unified interface which would be usable for those tests as well.

I haven't added support for parameters (it would be easy to add), atm only
kvm_pagefault01.c and can_common.h use them.

If is .modprobe (as TST_MODPROBE) supported in shell API, then these could use it:
* new API shell tests: binfmt_misc_lib.sh, rcu_torture.sh, ip_tests.sh (if we
  don't delete this test), mpls01.sh, tests which use mpls_lib.sh, tests which
  use tcp_cc_lib.sh, tc01.sh
* fou01.sh (new API) needs to load module after getopts, but it could use some
  unified interface.
* old API shell tests (after they are rewritten): lock_torture.sh

Few notes on modprobe:
* Do we even need to remove modules?
* should we use "modprobe -r" instead of "rmmod" on cleanup? rmmod is a simple
program which removes (unloads) a module from the Linux kernel. "modprobe -r"
handles a dependencies (if more modules loded, they are also removed).

* Network tests use -s on remote (log errors into syslog), I guess we probably
prefer for general use error on stderr.

Petr Vorel (4):
  tst_kernel: Add safe_check_driver()
  lib: Add .modprobe
  madvise11: Replace .needs_drivers with .modprobe
  can_bcm01: Move vcan to .modprobe

 doc/C-Test-API.asciidoc                       |  5 ++
 doc/Test-Writing-Guidelines.asciidoc          |  1 +
 include/tst_kernel.h                          |  9 +++
 include/tst_test.h                            |  5 +-
 lib/tst_kernel.c                              |  6 ++
 lib/tst_test.c                                | 56 ++++++++++++++++++-
 testcases/kernel/syscalls/madvise/madvise11.c | 36 +-----------
 testcases/network/can/cve/can_bcm01.c         | 19 ++++---
 8 files changed, 90 insertions(+), 47 deletions(-)

-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/4] tst_kernel: Add safe_check_driver()

[Petr Vorel]

And use it in tst_test.c. It will be more reused in the next commit.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_kernel.h | 9 +++++++++
 lib/tst_kernel.c     | 6 ++++++
 lib/tst_test.c       | 3 +--
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/tst_kernel.h b/include/tst_kernel.h
index 9d3a8d315..8caf3f733 100644
--- a/include/tst_kernel.h
+++ b/include/tst_kernel.h
@@ -30,4 +30,13 @@ int tst_check_builtin_driver(const char *driver);
  */
 int tst_check_driver(const char *driver);
 
+/*
+ * Checks support for the kernel module (both built-in and loadable)
+ * and exit with TCONF if driver not available.
+ *
+ * @param driver The name of the driver.
+ * On Android it *always* passes (always expect the driver is available).
+ */
+void safe_check_driver(const char *driver);
+
 #endif	/* TST_KERNEL_H__ */
diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
index 4b75ceadb..de4c28308 100644
--- a/lib/tst_kernel.c
+++ b/lib/tst_kernel.c
@@ -198,3 +198,9 @@ int tst_check_driver(const char *driver)
 
 	return -1;
 }
+
+void safe_check_driver(const char *driver)
+{
+	if (tst_check_driver(driver))
+		tst_brkm(TCONF, NULL, "%s driver not available", driver);
+}
diff --git a/lib/tst_test.c b/lib/tst_test.c
index c2f8f503f..087c62a16 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1191,8 +1191,7 @@ static void do_setup(int argc, char *argv[])
 		int i;
 
 		for (i = 0; (name = tst_test->needs_drivers[i]); ++i)
-			if (tst_check_driver(name))
-				tst_brk(TCONF, "%s driver not available", name);
+			safe_check_driver(name);
 	}
 
 	if (tst_test->mount_device)
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 doc/C-Test-API.asciidoc              |  5 +++
 doc/Test-Writing-Guidelines.asciidoc |  1 +
 include/tst_test.h                   |  5 ++-
 lib/tst_test.c                       | 53 ++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
index dab811564..f2ba302e2 100644
--- a/doc/C-Test-API.asciidoc
+++ b/doc/C-Test-API.asciidoc
@@ -1609,6 +1609,11 @@ first missing driver.
 The detection is based on reading 'modules.dep' and 'modules.builtin' files
 generated by kmod. The check is skipped on Android.
 
+NULL terminated array '.modprobe' of kernel module names are tried to be loaded
+with 'modprobe' unless they are builtin or already loaded. Test exits with
+'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
+loaded by the test unloaded with 'rmmod'.
+
 1.27 Saving & restoring /proc|sys values
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/doc/Test-Writing-Guidelines.asciidoc b/doc/Test-Writing-Guidelines.asciidoc
index 0db852ae6..19487816e 100644
--- a/doc/Test-Writing-Guidelines.asciidoc
+++ b/doc/Test-Writing-Guidelines.asciidoc
@@ -371,6 +371,7 @@ https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API].
 | '.min_mem_avail' | not applicable
 | '.mnt_flags' | 'TST_MNT_PARAMS'
 | '.min_swap_avail' | not applicable
+| '.modprobe' | –
 | '.mntpoint', '.mnt_data' | 'TST_MNTPOINT'
 | '.mount_device' | 'TST_MOUNT_DEVICE'
 | '.needs_cgroup_ctrls' | –
diff --git a/include/tst_test.h b/include/tst_test.h
index 75c2109b9..6b4fac985 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -297,9 +297,12 @@ struct tst_test {
 	/* NULL terminated array of resource file names */
 	const char *const *resource_files;
 
-	/* NULL terminated array of needed kernel drivers */
+	/* NULL terminated array of needed kernel drivers to be checked */
 	const char * const *needs_drivers;
 
+	/* NULL terminated array of needed kernel drivers to be loaded with modprobe */
+	const char * const *modprobe;
+
 	/*
 	 * {NULL, NULL} terminated array of (/proc, /sys) files to save
 	 * before setup and restore after cleanup
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 087c62a16..ccbaa4c02 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
 #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
 
 #define DEFAULT_TIMEOUT 30
+#define MODULES_MAX_LEN 10
 
 struct tst_test *tst_test;
 
@@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;
 
 static char shm_path[1024];
 
+static int modules_loaded[MODULES_MAX_LEN];
+
 int TST_ERR;
 int TST_PASS;
 long TST_RET;
@@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
 	tst_cg_init();
 }
 
+/*
+ * Search kernel driver in /proc/modules.
+ *
+ * @param driver The name of the driver.
+ * @return 1 if driver is found, otherwise 0.
+ */
+static int module_loaded(const char *driver)
+{
+	char line[4096];
+	int found = 0;
+	FILE *file = SAFE_FOPEN("/proc/modules", "r");
+
+	while (fgets(line, sizeof(line), file)) {
+		if (strstr(line, driver)) {
+			found = 1;
+			break;
+		}
+	}
+	SAFE_FCLOSE(file);
+
+	return found;
+}
+
 static void do_setup(int argc, char *argv[])
 {
 	if (!tst_test)
@@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
 			safe_check_driver(name);
 	}
 
+	if (tst_test->modprobe) {
+		const char *name;
+		int i;
+
+		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
+			/* only load module if not already loaded */
+			if (!module_loaded(name) && tst_check_builtin_driver(name)) {
+				const char *const cmd_modprobe[] = {"modprobe", name, NULL};
+				SAFE_CMD(cmd_modprobe, NULL, NULL);
+				modules_loaded[i] = 1;
+			}
+		}
+	}
+
 	if (tst_test->mount_device)
 		tst_test->format_device = 1;
 
@@ -1362,6 +1402,19 @@ static void do_cleanup(void)
 
 	tst_sys_conf_restore(0);
 
+	if (tst_test->modprobe) {
+		const char *name;
+		int i;
+
+		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
+			if (!modules_loaded[i])
+				continue;
+
+			const char *const cmd_rmmod[] = {"rmmod", name, NULL};
+			SAFE_CMD(cmd_rmmod, NULL, NULL);
+		}
+	}
+
 	if (tst_test->restore_wallclock)
 		tst_wallclock_restore();
 
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/4] madvise11: Replace .needs_drivers with .modprobe

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/syscalls/madvise/madvise11.c | 36 +------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/testcases/kernel/syscalls/madvise/madvise11.c b/testcases/kernel/syscalls/madvise/madvise11.c
index 3cde85ef5..68f801610 100644
--- a/testcases/kernel/syscalls/madvise/madvise11.c
+++ b/testcases/kernel/syscalls/madvise/madvise11.c
@@ -47,7 +47,6 @@ static pthread_mutex_t sigbus_received_mtx = PTHREAD_MUTEX_INITIALIZER;
 
 static long pagesize;
 static char beginning_tag[BUFSIZ];
-static int hwpoison_probe;
 
 static void my_yield(void)
 {
@@ -266,22 +265,6 @@ static int populate_from_klog(char *begin_tag, unsigned long *pfns, int max)
  * Read the given file to search for the key.
  * Return 1 if the key is found.
  */
-static int find_in_file(char *path, char *key)
-{
-	char line[4096];
-	int found = 0;
-	FILE *file = SAFE_FOPEN(path, "r");
-
-	while (fgets(line, sizeof(line), file)) {
-		if (strstr(line, key)) {
-			found = 1;
-			break;
-		}
-	}
-	SAFE_FCLOSE(file);
-	return found;
-}
-
 static void unpoison_this_pfn(unsigned long pfn, int fd)
 {
 	char pfn_str[19];
@@ -294,18 +277,10 @@ static void unpoison_this_pfn(unsigned long pfn, int fd)
 static int open_unpoison_pfn(void)
 {
 	char *added_file_path = "/hwpoison/unpoison-pfn";
-	const char *const cmd_modprobe[] = {"modprobe", HW_MODULE, NULL};
 	char debugfs_fp[4096];
 	struct mntent *mnt;
 	FILE *mntf;
 
-	if (!find_in_file("/proc/modules", HW_MODULE) && tst_check_builtin_driver(HW_MODULE))
-		hwpoison_probe = 1;
-
-	/* probe hwpoison only if it isn't already there */
-	if (hwpoison_probe)
-		SAFE_CMD(cmd_modprobe, NULL, NULL);
-
 	/* debugfs mount point */
 	mntf = setmntent("/etc/mtab", "r");
 	if (!mntf) {
@@ -349,7 +324,6 @@ static int open_unpoison_pfn(void)
 static void unpoison_pfn(char *begin_tag)
 {
 	unsigned long *pfns;
-	const char *const cmd_rmmod[] = {"rmmod", HW_MODULE, NULL};
 	int found_pfns, fd;
 
 	pfns = SAFE_MALLOC(sizeof(pfns) * maximum_pfns * run_iterations);
@@ -365,9 +339,6 @@ static void unpoison_pfn(char *begin_tag)
 
 		SAFE_CLOSE(fd);
 	}
-	/* remove hwpoison only if we probed it */
-	if (hwpoison_probe)
-		SAFE_CMD(cmd_rmmod, NULL, NULL);
 }
 
 /*
@@ -417,15 +388,10 @@ static void cleanup(void)
 
 static struct tst_test test = {
 	.needs_root = 1,
-	.needs_drivers = (const char *const []) {
+	.modprobe = (const char *const []) {
 		HW_MODULE,
 		NULL
 	},
-	.needs_cmds = (const char *[]) {
-		"modprobe",
-		"rmmod",
-		NULL
-	},
 	.max_runtime = 30,
 	.needs_checkpoints = 1,
 	.setup = setup,
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 4/4] can_bcm01: Move vcan to .modprobe

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/network/can/cve/can_bcm01.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/testcases/network/can/cve/can_bcm01.c b/testcases/network/can/cve/can_bcm01.c
index d9a835b03..ec98db133 100644
--- a/testcases/network/can/cve/can_bcm01.c
+++ b/testcases/network/can/cve/can_bcm01.c
@@ -41,14 +41,6 @@ static void setup(void)
 {
 	struct sockaddr_can addr = { .can_family = AF_CAN };
 
-	/*
-	 * Older kernels require explicit modprobe of vcan. Newer kernels
-	 * will load the modules automatically and support CAN in network
-	 * namespace which would eliminate the need for running the test
-	 * with root privileges.
-	 */
-	tst_cmd((const char*[]){"modprobe", "vcan", NULL}, NULL, NULL, 0);
-
 	NETDEV_ADD_DEVICE(LTP_DEVICE, "vcan");
 	NETDEV_SET_STATE(LTP_DEVICE, 1);
 	addr.can_ifindex = NETDEV_INDEX_BY_NAME(LTP_DEVICE);
@@ -143,10 +135,19 @@ static struct tst_test test = {
 	.skip_in_compat = 1,
 	.max_runtime = 30,
 	.needs_drivers = (const char *const[]) {
-		"vcan",
 		"can-bcm",
 		NULL
 	},
+	/*
+	 * Older kernels require explicit modprobe of vcan. Newer kernels
+	 * will load the modules automatically and support CAN in network
+	 * namespace which would eliminate the need for running the test
+	 * with root privileges.
+	 */
+	.modprobe = (const char *const[]) {
+		"vcan",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "d5f9023fa61e"},
 		{"CVE", "2021-3609"},
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Li Wang]

Hi Petr,

On Fri, Oct 13, 2023 at 3:48 PM Petr Vorel <pvorel@suse.cz> wrote:

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  doc/C-Test-API.asciidoc              |  5 +++
>  doc/Test-Writing-Guidelines.asciidoc |  1 +
>  include/tst_test.h                   |  5 ++-
>  lib/tst_test.c                       | 53 ++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index dab811564..f2ba302e2 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -1609,6 +1609,11 @@ first missing driver.
>  The detection is based on reading 'modules.dep' and 'modules.builtin'
> files
>  generated by kmod. The check is skipped on Android.
>
> +NULL terminated array '.modprobe' of kernel module names are tried to be
> loaded
> +with 'modprobe' unless they are builtin or already loaded. Test exits with
> +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
> +loaded by the test unloaded with 'rmmod'.
> +
>  1.27 Saving & restoring /proc|sys values
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/doc/Test-Writing-Guidelines.asciidoc
> b/doc/Test-Writing-Guidelines.asciidoc
> index 0db852ae6..19487816e 100644
> --- a/doc/Test-Writing-Guidelines.asciidoc
> +++ b/doc/Test-Writing-Guidelines.asciidoc
> @@ -371,6 +371,7 @@
> https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test
> API].
>  | '.min_mem_avail' | not applicable
>  | '.mnt_flags' | 'TST_MNT_PARAMS'
>  | '.min_swap_avail' | not applicable
> +| '.modprobe' | –
>  | '.mntpoint', '.mnt_data' | 'TST_MNTPOINT'
>  | '.mount_device' | 'TST_MOUNT_DEVICE'
>  | '.needs_cgroup_ctrls' | –
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..6b4fac985 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -297,9 +297,12 @@ struct tst_test {
>         /* NULL terminated array of resource file names */
>         const char *const *resource_files;
>
> -       /* NULL terminated array of needed kernel drivers */
> +       /* NULL terminated array of needed kernel drivers to be checked */
>         const char * const *needs_drivers;
>
> +       /* NULL terminated array of needed kernel drivers to be loaded
> with modprobe */
> +       const char * const *modprobe;
> +
>         /*
>          * {NULL, NULL} terminated array of (/proc, /sys) files to save
>          * before setup and restore after cleanup
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 087c62a16..ccbaa4c02 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
>  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
>
>  #define DEFAULT_TIMEOUT 30
> +#define MODULES_MAX_LEN 10
>
>  struct tst_test *tst_test;
>
> @@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;
>
>  static char shm_path[1024];
>
> +static int modules_loaded[MODULES_MAX_LEN];
> +
>  int TST_ERR;
>  int TST_PASS;
>  long TST_RET;
> @@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
>         tst_cg_init();
>  }
>
> +/*
> + * Search kernel driver in /proc/modules.
> + *
> + * @param driver The name of the driver.
> + * @return 1 if driver is found, otherwise 0.
> + */
> +static int module_loaded(const char *driver)
>


What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
I guess we could make use of it widely for checking module loading or not.



> +{
> +       char line[4096];
> +       int found = 0;
> +       FILE *file = SAFE_FOPEN("/proc/modules", "r");
> +
> +       while (fgets(line, sizeof(line), file)) {
> +               if (strstr(line, driver)) {
> +                       found = 1;
> +                       break;
> +               }
> +       }
> +       SAFE_FCLOSE(file);
> +
> +       return found;
> +}
> +
>  static void do_setup(int argc, char *argv[])
>  {
>         if (!tst_test)
> @@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
>                         safe_check_driver(name);
>         }
>
> +       if (tst_test->modprobe) {
>



> +               const char *name;
> +               int i;
> +
> +               for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> +                       /* only load module if not already loaded */
> +                       if (!module_loaded(name) &&
> tst_check_builtin_driver(name)) {
> +                               const char *const cmd_modprobe[] =
> {"modprobe", name, NULL};
> +                               SAFE_CMD(cmd_modprobe, NULL, NULL);
>


And here print the name to tell people the module is loaded.



> +                               modules_loaded[i] = 1;
> +                       }
> +               }
> +       }
>


This part could be as a separate function like tst_load_module() and
built single into another lib. We prefer to keep the main tst_test.c
as a simple outline.

On the other hand, the extern functions can be used separately to let
modules to be loaded and unloaded during the test iteration.
It gives us more flexibility in test case design.




> +
>         if (tst_test->mount_device)
>                 tst_test->format_device = 1;
>
> @@ -1362,6 +1402,19 @@ static void do_cleanup(void)
>
>         tst_sys_conf_restore(0);
>
> +       if (tst_test->modprobe) {
>



> +               const char *name;
> +               int i;
> +
> +               for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> +                       if (!modules_loaded[i])
> +                               continue;
> +
> +                       const char *const cmd_rmmod[] = {"rmmod", name,
> NULL};
> +                       SAFE_CMD(cmd_rmmod, NULL, NULL);
>

Print unload module name.


> +               }
> +       }
>

Here as well. something maybe like tst_unload_module().


+
>         if (tst_test->restore_wallclock)
>                 tst_wallclock_restore();
>
> --
> 2.42.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi Li,

> > +/*
> > + * Search kernel driver in /proc/modules.
> > + *
> > + * @param driver The name of the driver.
> > + * @return 1 if driver is found, otherwise 0.
> > + */
> > +static int module_loaded(const char *driver)



> What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
> I guess we could make use of it widely for checking module loading or not.

+1

Note, it's based on find_in_file() from madvise11.c (which I then removed in the
3rd commit). This function takes 2 params (also file), maybe we can have use for
this generic function. It's kind of similar to safe_file_scanf().

Kind regards,
Petr

> > +{
> > +       char line[4096];
> > +       int found = 0;
> > +       FILE *file = SAFE_FOPEN("/proc/modules", "r");
> > +
> > +       while (fgets(line, sizeof(line), file)) {
> > +               if (strstr(line, driver)) {
> > +                       found = 1;
> > +                       break;
> > +               }
> > +       }
> > +       SAFE_FCLOSE(file);
> > +
> > +       return found;
> > +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/4] tst_kernel: Add safe_check_driver()

[Petr Vorel]

Hi All,

...
>  int tst_check_driver(const char *driver);

> +/*
> + * Checks support for the kernel module (both built-in and loadable)
> + * and exit with TCONF if driver not available.
> + *
> + * @param driver The name of the driver.
> + * On Android it *always* passes (always expect the driver is available).
> + */
> +void safe_check_driver(const char *driver);

In the end, I haven't used this function in the second commit,
thus this function is useless (used only on single place).
I'll remove it in v2 (still waiting for more input if whole .modprobe option is
worth of merging).

Kind regards,
Petr

> +
>  #endif	/* TST_KERNEL_H__ */
> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> index 4b75ceadb..de4c28308 100644
> --- a/lib/tst_kernel.c
> +++ b/lib/tst_kernel.c
> @@ -198,3 +198,9 @@ int tst_check_driver(const char *driver)

>  	return -1;
>  }
> +
> +void safe_check_driver(const char *driver)
> +{
> +	if (tst_check_driver(driver))
> +		tst_brkm(TCONF, NULL, "%s driver not available", driver);
> +}
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index c2f8f503f..087c62a16 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1191,8 +1191,7 @@ static void do_setup(int argc, char *argv[])
>  		int i;

>  		for (i = 0; (name = tst_test->needs_drivers[i]); ++i)
> -			if (tst_check_driver(name))
> -				tst_brk(TCONF, "%s driver not available", name);
> +			safe_check_driver(name);
>  	}

>  	if (tst_test->mount_device)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi all,

maybe .modprobe is too short name, but I'm not sure what would be better.
Maybe .modprobe_module ?

...
> +++ b/doc/C-Test-API.asciidoc
> @@ -1609,6 +1609,11 @@ first missing driver.
>  The detection is based on reading 'modules.dep' and 'modules.builtin' files
>  generated by kmod. The check is skipped on Android.

> +NULL terminated array '.modprobe' of kernel module names are tried to be loaded
> +with 'modprobe' unless they are builtin or already loaded. Test exits with
> +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
> +loaded by the test unloaded with 'rmmod'.

...

> +	if (tst_test->modprobe) {
> +		const char *name;
> +		int i;
> +
> +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> +			/* only load module if not already loaded */
> +			if (!module_loaded(name) && tst_check_builtin_driver(name)) {

Also we could make it independent on modules.builtin (NixOS based problem -
missing these files). I.e. we would keep only module_loaded(), remove 
tst_check_builtin_driver(). But then we could not run rmmod / modprobe -r,
or we would have to ignore it's exit code (rmmod on builtin module) fails.

Kind regards,
Petr

> +				const char *const cmd_modprobe[] = {"modprobe", name, NULL};
> +				SAFE_CMD(cmd_modprobe, NULL, NULL);
> +				modules_loaded[i] = 1;
> +			}
> +		}
> +	}
> +

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Li Wang]

On Fri, Oct 13, 2023 at 8:31 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi all,
>
> maybe .modprobe is too short name, but I'm not sure what would be better.
> Maybe .modprobe_module ?
>

.modprobe_module sounds better.

Also, I think that maybe we can support modprobe some
third-party modules (written by users) in test case, there are
a few managed by shell scripts, but if .modprobe_module
manages them unify in C, it would be nice for test variety.



>
> ...
> > +++ b/doc/C-Test-API.asciidoc
> > @@ -1609,6 +1609,11 @@ first missing driver.
> >  The detection is based on reading 'modules.dep' and 'modules.builtin'
> files
> >  generated by kmod. The check is skipped on Android.
>
> > +NULL terminated array '.modprobe' of kernel module names are tried to
> be loaded
> > +with 'modprobe' unless they are builtin or already loaded. Test exits
> with
> > +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the
> modules
> > +loaded by the test unloaded with 'rmmod'.
>
> ...
>
> > +     if (tst_test->modprobe) {
> > +             const char *name;
> > +             int i;
> > +
> > +             for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +                     /* only load module if not already loaded */
> > +                     if (!module_loaded(name) &&
> tst_check_builtin_driver(name)) {
>
> Also we could make it independent on modules.builtin (NixOS based problem -
> missing these files). I.e. we would keep only module_loaded(), remove
> tst_check_builtin_driver(). But then we could not run rmmod / modprobe -r,
> or we would have to ignore it's exit code (rmmod on builtin module) fails.
>

Or we add one step to detect modules.builtin file, if no,
then just print a Warning at unload in fails?



>
> Kind regards,
> Petr
>
> > +                             const char *const cmd_modprobe[] =
> {"modprobe", name, NULL};
> > +                             SAFE_CMD(cmd_modprobe, NULL, NULL);
> > +                             modules_loaded[i] = 1;
> > +                     }
> > +             }
> > +     }
> > +
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi Li,

thanks for your input!

> > Hi all,

> > maybe .modprobe is too short name, but I'm not sure what would be better.
> > Maybe .modprobe_module ?


> .modprobe_module sounds better.
+1

> Also, I think that maybe we can support modprobe some
> third-party modules (written by users) in test case, there are
> a few managed by shell scripts, but if .modprobe_module
> manages them unify in C, it would be nice for test variety.

+1. Also I plan to move some of the LTP kernel modules - tests which use kernel
modules from LTP (e.g. delete_module, init_module, ...)to KUnit or kselftest (to
solve problem with signed modules required by distro kernels, kernel modules
from LTP are then untestable on lockdown).  But maybe these modules can stay in
LTP and also be added to KUnit.
But these modules use tst_module_exists_() and SAFE_OPEN(). So you might mean
3rd party modules like nvidia or other proprietary modules, right?

Then, some of the tests in testcases/kernel/device-drivers/ might be obsolete or
also be more suitable in kselftest or KUnit or elsewhere.

...
> > > +     if (tst_test->modprobe) {
> > > +             const char *name;
> > > +             int i;
> > > +
> > > +             for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > > +                     /* only load module if not already loaded */
> > > +                     if (!module_loaded(name) &&
> > tst_check_builtin_driver(name)) {

> > Also we could make it independent on modules.builtin (NixOS based problem -
> > missing these files). I.e. we would keep only module_loaded(), remove
> > tst_check_builtin_driver(). But then we could not run rmmod / modprobe -r,
> > or we would have to ignore it's exit code (rmmod on builtin module) fails.


> Or we add one step to detect modules.builtin file, if no,
> then just print a Warning at unload in fails?

Unloading shouldn't be problem since it's in cleanup (thus TBROK => TWARN).
But I'll test it.

Or do you mean that on missing modules.builtin would test itself be working as
module is presented (have warning on that modules.* files are missing and
warning on rmmod?).

Would you do it for both .modprobe_module and .needs_drivers? Or just one
of them?

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/4] Add .modprobe (loading modules in C API)

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi,
>
> when I started this patch it looked to me that quite a few C tests executes
> modprobe to load kernel module. In the end, so far only can_bcm01.c and
> madvise11.c uses it. But maybe more could be used.

Just to be clear this is a great idea in principle.

What concerns me is that module detection and loading will be
mandatory. Then tests which can work without root, even run in a sandbox,
will require real root.

I want to be able to see if a bug is reachable from inside a container,
embedded system or nsjail without adding /lib/modules to the
environment or recompiling the test.

In particular the module detection is a problem because it stops the
test from running when the module is present, but some file is not. If
it is acceptable for Android to disable this check then it should be
acceptable for any system.

If we make the check "best efforts", then we don't need a special define
for Android either.

>
> If I add support for module parameters (it would be easy to add), it could be
> used also in can_common.h testcases/network/can/filter-tests/.
>
> It could also be used in the old API C tests in testcases/kernel/input (which
> use input_helper.c), of course after we rewrite them to the new API.
>
> Tests which use modprobe but using .modprobe is not usable:
> * kvm_pagefault01.c (more complicated use - it requires checks).
> * zram03.c, zram_lib.sh (too complicated check due /sys/class/zram-control
>   introduced in v4.2-rc1 vs. the old API, but maybe this could be simplified)
> * netstress.c (used only when testing dccp, which is determined by getopts)
>
> But if we move code I put into tst_test.c into separate function, we could be
> unified interface which would be usable for those tests as well.
>
> I haven't added support for parameters (it would be easy to add), atm only
> kvm_pagefault01.c and can_common.h use them.
>
> If is .modprobe (as TST_MODPROBE) supported in shell API, then these could use it:
> * new API shell tests: binfmt_misc_lib.sh, rcu_torture.sh, ip_tests.sh (if we
>   don't delete this test), mpls01.sh, tests which use mpls_lib.sh, tests which
>   use tcp_cc_lib.sh, tc01.sh
> * fou01.sh (new API) needs to load module after getopts, but it could use some
>   unified interface.
> * old API shell tests (after they are rewritten): lock_torture.sh
>
> Few notes on modprobe:
> * Do we even need to remove modules?

IDK, but it could be useful for triggering a double free or counter
underflow etc.

> * should we use "modprobe -r" instead of "rmmod" on cleanup? rmmod is a simple
> program which removes (unloads) a module from the Linux kernel. "modprobe -r"
> handles a dependencies (if more modules loded, they are also removed).

These should probably be configurable. There are different
implementations of these tools (e.g. busybox, toybox etc.). In Toybox
modprobe is in "pending", meanwhile lsmod, insmod and rmmod are complete.

>
> * Network tests use -s on remote (log errors into syslog), I guess we probably
> prefer for general use error on stderr.
>
> Petr Vorel (4):
>   tst_kernel: Add safe_check_driver()
>   lib: Add .modprobe
>   madvise11: Replace .needs_drivers with .modprobe
>   can_bcm01: Move vcan to .modprobe
>
>  doc/C-Test-API.asciidoc                       |  5 ++
>  doc/Test-Writing-Guidelines.asciidoc          |  1 +
>  include/tst_kernel.h                          |  9 +++
>  include/tst_test.h                            |  5 +-
>  lib/tst_kernel.c                              |  6 ++
>  lib/tst_test.c                                | 56 ++++++++++++++++++-
>  testcases/kernel/syscalls/madvise/madvise11.c | 36 +-----------
>  testcases/network/can/cve/can_bcm01.c         | 19 ++++---
>  8 files changed, 90 insertions(+), 47 deletions(-)
>
> -- 
> 2.42.0


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Li Wang]

Hi Petr,

On Fri, Oct 13, 2023 at 9:50 PM Petr Vorel <pvorel@suse.cz> wrote:

>
> Hi Li,
>
> thanks for your input!
>
> > > Hi all,
>
> > > maybe .modprobe is too short name, but I'm not sure what would be
> better.
> > > Maybe .modprobe_module ?
>
>
> > .modprobe_module sounds better.
> +1
>
> > Also, I think that maybe we can support modprobe some
> > third-party modules (written by users) in test case, there are
> > a few managed by shell scripts, but if .modprobe_module
> > manages them unify in C, it would be nice for test variety.
>
> +1. Also I plan to move some of the LTP kernel modules - tests which use
> kernel
> modules from LTP (e.g. delete_module, init_module, ...)to KUnit or
> kselftest (to
> solve problem with signed modules required by distro kernels, kernel
> modules
> from LTP are then untestable on lockdown).  But maybe these modules can
> stay in
> LTP and also be added to KUnit.
>

Yes, they can stay in LTP, because we do have '.skip_in_lockdown/secureboot'
to avoid the unsigned loading error.



> But these modules use tst_module_exists_() and SAFE_OPEN(). So you might
> mean
> 3rd party modules like nvidia or other proprietary modules, right?
>

Hmm,  for sure we can't satisfy all situations, but at least there could be
a
separate way to use init/finit_module() without .'modprobe_module'.

But for tst_module_load/unload, I think they could be integrated within
.modprobe_module whatever loading by `insmod` or `modprobe`.



>
> Then, some of the tests in testcases/kernel/device-drivers/ might be
> obsolete or
> also be more suitable in kselftest or KUnit or elsewhere.
>
> ...
> > > > +     if (tst_test->modprobe) {
> > > > +             const char *name;
> > > > +             int i;
> > > > +
> > > > +             for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > > > +                     /* only load module if not already loaded */
> > > > +                     if (!module_loaded(name) &&
> > > tst_check_builtin_driver(name)) {
>
> > > Also we could make it independent on modules.builtin (NixOS based
> problem -
> > > missing these files). I.e. we would keep only module_loaded(), remove
> > > tst_check_builtin_driver(). But then we could not run rmmod / modprobe
> -r,
> > > or we would have to ignore it's exit code (rmmod on builtin module)
> fails.
>
>
> > Or we add one step to detect modules.builtin file, if no,
> > then just print a Warning at unload in fails?
>
> Unloading shouldn't be problem since it's in cleanup (thus TBROK => TWARN).
> But I'll test it.
>
> Or do you mean that on missing modules.builtin would test itself be
> working as
> module is presented (have warning on that modules.* files are missing and
> warning on rmmod?).
>

No, I didn't think so deeply:).



>
> Would you do it for both .modprobe_module and .needs_drivers? Or just one
> of them?
>

Maybe both.

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/4] Add .modprobe (loading modules in C API)

[Richard Palethorpe]

Hello,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Petr Vorel <pvorel@suse.cz> writes:
>
>> Hi,
>>
>> when I started this patch it looked to me that quite a few C tests executes
>> modprobe to load kernel module. In the end, so far only can_bcm01.c and
>> madvise11.c uses it. But maybe more could be used.
>
> Just to be clear this is a great idea in principle.
>
> What concerns me is that module detection and loading will be
> mandatory. Then tests which can work without root, even run in a sandbox,
> will require real root.
>
> I want to be able to see if a bug is reachable from inside a container,
> embedded system or nsjail without adding /lib/modules to the
> environment or recompiling the test.
>
> In particular the module detection is a problem because it stops the
> test from running when the module is present, but some file is not. If
> it is acceptable for Android to disable this check then it should be
> acceptable for any system.
>
> If we make the check "best efforts", then we don't need a special define
> for Android either.

Also it occurred to me that in .modprobe you could add a field which
specifies the kconfig parameter name for the module.

So we could avoid modules.builtin, by checking the kernel configuration
first. If the module is builtin or set to N, then no more checks are
required.

>
>>
>> If I add support for module parameters (it would be easy to add), it could be
>> used also in can_common.h testcases/network/can/filter-tests/.
>>
>> It could also be used in the old API C tests in testcases/kernel/input (which
>> use input_helper.c), of course after we rewrite them to the new API.
>>
>> Tests which use modprobe but using .modprobe is not usable:
>> * kvm_pagefault01.c (more complicated use - it requires checks).
>> * zram03.c, zram_lib.sh (too complicated check due /sys/class/zram-control
>>   introduced in v4.2-rc1 vs. the old API, but maybe this could be simplified)
>> * netstress.c (used only when testing dccp, which is determined by getopts)
>>
>> But if we move code I put into tst_test.c into separate function, we could be
>> unified interface which would be usable for those tests as well.
>>
>> I haven't added support for parameters (it would be easy to add), atm only
>> kvm_pagefault01.c and can_common.h use them.
>>
>> If is .modprobe (as TST_MODPROBE) supported in shell API, then these could use it:
>> * new API shell tests: binfmt_misc_lib.sh, rcu_torture.sh, ip_tests.sh (if we
>>   don't delete this test), mpls01.sh, tests which use mpls_lib.sh, tests which
>>   use tcp_cc_lib.sh, tc01.sh
>> * fou01.sh (new API) needs to load module after getopts, but it could use some
>>   unified interface.
>> * old API shell tests (after they are rewritten): lock_torture.sh
>>
>> Few notes on modprobe:
>> * Do we even need to remove modules?
>
> IDK, but it could be useful for triggering a double free or counter
> underflow etc.
>
>> * should we use "modprobe -r" instead of "rmmod" on cleanup? rmmod is a simple
>> program which removes (unloads) a module from the Linux kernel. "modprobe -r"
>> handles a dependencies (if more modules loded, they are also removed).
>
> These should probably be configurable. There are different
> implementations of these tools (e.g. busybox, toybox etc.). In Toybox
> modprobe is in "pending", meanwhile lsmod, insmod and rmmod are complete.
>
>>
>> * Network tests use -s on remote (log errors into syslog), I guess we probably
>> prefer for general use error on stderr.
>>
>> Petr Vorel (4):
>>   tst_kernel: Add safe_check_driver()
>>   lib: Add .modprobe
>>   madvise11: Replace .needs_drivers with .modprobe
>>   can_bcm01: Move vcan to .modprobe
>>
>>  doc/C-Test-API.asciidoc                       |  5 ++
>>  doc/Test-Writing-Guidelines.asciidoc          |  1 +
>>  include/tst_kernel.h                          |  9 +++
>>  include/tst_test.h                            |  5 +-
>>  lib/tst_kernel.c                              |  6 ++
>>  lib/tst_test.c                                | 56 ++++++++++++++++++-
>>  testcases/kernel/syscalls/madvise/madvise11.c | 36 +-----------
>>  testcases/network/can/cve/can_bcm01.c         | 19 ++++---
>>  8 files changed, 90 insertions(+), 47 deletions(-)
>>
>> -- 
>> 2.42.0
>
>
> -- 
> Thank you,
> Richard.


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Li Wang]

On Fri, Oct 13, 2023 at 8:09 PM Li Wang <liwang@redhat.com> wrote:

> Hi Petr,
>
> On Fri, Oct 13, 2023 at 3:48 PM Petr Vorel <pvorel@suse.cz> wrote:
>
>> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>> ---
>>  doc/C-Test-API.asciidoc              |  5 +++
>>  doc/Test-Writing-Guidelines.asciidoc |  1 +
>>  include/tst_test.h                   |  5 ++-
>>  lib/tst_test.c                       | 53 ++++++++++++++++++++++++++++
>>  4 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
>> index dab811564..f2ba302e2 100644
>> --- a/doc/C-Test-API.asciidoc
>> +++ b/doc/C-Test-API.asciidoc
>> @@ -1609,6 +1609,11 @@ first missing driver.
>>  The detection is based on reading 'modules.dep' and 'modules.builtin'
>> files
>>  generated by kmod. The check is skipped on Android.
>>
>> +NULL terminated array '.modprobe' of kernel module names are tried to be
>> loaded
>> +with 'modprobe' unless they are builtin or already loaded. Test exits
>> with
>> +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
>> +loaded by the test unloaded with 'rmmod'.
>> +
>>  1.27 Saving & restoring /proc|sys values
>>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> diff --git a/doc/Test-Writing-Guidelines.asciidoc
>> b/doc/Test-Writing-Guidelines.asciidoc
>> index 0db852ae6..19487816e 100644
>> --- a/doc/Test-Writing-Guidelines.asciidoc
>> +++ b/doc/Test-Writing-Guidelines.asciidoc
>> @@ -371,6 +371,7 @@
>> https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test
>> API].
>>  | '.min_mem_avail' | not applicable
>>  | '.mnt_flags' | 'TST_MNT_PARAMS'
>>  | '.min_swap_avail' | not applicable
>> +| '.modprobe' | –
>>  | '.mntpoint', '.mnt_data' | 'TST_MNTPOINT'
>>  | '.mount_device' | 'TST_MOUNT_DEVICE'
>>  | '.needs_cgroup_ctrls' | –
>> diff --git a/include/tst_test.h b/include/tst_test.h
>> index 75c2109b9..6b4fac985 100644
>> --- a/include/tst_test.h
>> +++ b/include/tst_test.h
>> @@ -297,9 +297,12 @@ struct tst_test {
>>         /* NULL terminated array of resource file names */
>>         const char *const *resource_files;
>>
>> -       /* NULL terminated array of needed kernel drivers */
>> +       /* NULL terminated array of needed kernel drivers to be checked */
>>         const char * const *needs_drivers;
>>
>> +       /* NULL terminated array of needed kernel drivers to be loaded
>> with modprobe */
>> +       const char * const *modprobe;
>> +
>>         /*
>>          * {NULL, NULL} terminated array of (/proc, /sys) files to save
>>          * before setup and restore after cleanup
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index 087c62a16..ccbaa4c02 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>> @@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
>>  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
>>
>>  #define DEFAULT_TIMEOUT 30
>> +#define MODULES_MAX_LEN 10
>>
>>  struct tst_test *tst_test;
>>
>> @@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;
>>
>>  static char shm_path[1024];
>>
>> +static int modules_loaded[MODULES_MAX_LEN];
>> +
>>  int TST_ERR;
>>  int TST_PASS;
>>  long TST_RET;
>> @@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
>>         tst_cg_init();
>>  }
>>
>> +/*
>> + * Search kernel driver in /proc/modules.
>> + *
>> + * @param driver The name of the driver.
>> + * @return 1 if driver is found, otherwise 0.
>> + */
>> +static int module_loaded(const char *driver)
>>
>
>
> What about renaming it as tst_module_is_loaded() and move into
> tst_kernel.h?
> I guess we could make use of it widely for checking module loading or not.
>


tst_module.h should be a better place, I just find it when read some
existing LTP cases.



-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/4] Add .modprobe (loading modules in C API)

[Cyril Hrubis]

Hi!
> IDK, but it could be useful for triggering a double free or counter
> underflow etc.

I can imagine tests where one thread insert/remove modules while other
thread open/read/write/close devices exported by these module.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi all,

[ Cc Cyril ]

...
> > +++ b/include/tst_test.h
> > @@ -297,9 +297,12 @@ struct tst_test {
> >         /* NULL terminated array of resource file names */
> >         const char *const *resource_files;

> > -       /* NULL terminated array of needed kernel drivers */
> > +       /* NULL terminated array of needed kernel drivers to be checked */
> >         const char * const *needs_drivers;

> > +       /* NULL terminated array of needed kernel drivers to be loaded
> > with modprobe */
> > +       const char * const *modprobe;
> > +
> >         /*
> >          * {NULL, NULL} terminated array of (/proc, /sys) files to save
> >          * before setup and restore after cleanup
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 087c62a16..ccbaa4c02 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
> >  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"

> >  #define DEFAULT_TIMEOUT 30
> > +#define MODULES_MAX_LEN 10

> >  struct tst_test *tst_test;

> > @@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;

> >  static char shm_path[1024];

> > +static int modules_loaded[MODULES_MAX_LEN];
> > +
> >  int TST_ERR;
> >  int TST_PASS;
> >  long TST_RET;
> > @@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
> >         tst_cg_init();
> >  }

> > +/*
> > + * Search kernel driver in /proc/modules.
> > + *
> > + * @param driver The name of the driver.
> > + * @return 1 if driver is found, otherwise 0.
> > + */
> > +static int module_loaded(const char *driver)



> What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
> I guess we could make use of it widely for checking module loading or not.

I can do that, but lib/tst_kernel.c uses the old API. I guess it would fit
better in lib/tst_module.c, but that also uses the old API. Most of the tests
are converted, but at least these modules are still in the old API and use
tst_module_load from tst_module.h:

testcases/kernel/device-drivers/acpi/ltp_acpi.c
testcases/kernel/device-drivers/block/block_dev_user/block_dev.c
testcases/kernel/device-drivers/pci/tpci_user/tpci.c
testcases/kernel/device-drivers/uaccess/uaccess.c
testcases/kernel/firmware/fw_load_user/fw_load.c
testcases/kernel/device-drivers/tbio/tbio_user/tbio.c

All but the last one were written by Alexey, they look ok, so they should 
probably be converted. But I would rather not block this .modprobe_module
effort due this.

IMHO We need another file, which would be new API only. I'm also not sure if
it's a good idea to put another file with just single function to it. We already
have 38 lib/tst_*.c files which use new API. Any tip, what to use?
Or should I really put it into lib/tst_module.c ain include/tst_module.h, but
not into include/old/old_module.h (as we want old tests to be converted first?).

> > +{
> > +       char line[4096];
> > +       int found = 0;
> > +       FILE *file = SAFE_FOPEN("/proc/modules", "r");
> > +
> > +       while (fgets(line, sizeof(line), file)) {
> > +               if (strstr(line, driver)) {
> > +                       found = 1;
> > +                       break;
> > +               }
> > +       }
> > +       SAFE_FCLOSE(file);
> > +
> > +       return found;
> > +}
> > +
> >  static void do_setup(int argc, char *argv[])
> >  {
> >         if (!tst_test)
> > @@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
> >                         safe_check_driver(name);
> >         }

> > +       if (tst_test->modprobe) {




> > +               const char *name;
> > +               int i;
> > +
> > +               for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +                       /* only load module if not already loaded */
> > +                       if (!module_loaded(name) &&
> > tst_check_builtin_driver(name)) {
> > +                               const char *const cmd_modprobe[] =
> > {"modprobe", name, NULL};
> > +                               SAFE_CMD(cmd_modprobe, NULL, NULL);



> And here print the name to tell people the module is loaded.


+1

> > +                               modules_loaded[i] = 1;
> > +                       }
> > +               }
> > +       }



> This part could be as a separate function like tst_load_module() and
> built single into another lib. We prefer to keep the main tst_test.c
> as a simple outline.

+1 for a separate function, it should be in the same file as
tst_module_is_loaded().

> On the other hand, the extern functions can be used separately to let
> modules to be loaded and unloaded during the test iteration.
> It gives us more flexibility in test case design.

Having it as the separate function would allow to use it in
kvm_pagefault01.c and zram03.c - tiny simplification as they now call
SAFE_CMD().

kvm_pagefault01.c and can_common.h use them parameters, it might be worth
to implement them.

> > +
> >         if (tst_test->mount_device)
> >                 tst_test->format_device = 1;

> > @@ -1362,6 +1402,19 @@ static void do_cleanup(void)

> >         tst_sys_conf_restore(0);

> > +       if (tst_test->modprobe) {




> > +               const char *name;
> > +               int i;
> > +
> > +               for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +                       if (!modules_loaded[i])
> > +                               continue;
> > +
> > +                       const char *const cmd_rmmod[] = {"rmmod", name,
> > NULL};
> > +                       SAFE_CMD(cmd_rmmod, NULL, NULL);


> Print unload module name.

+1

> > +               }
> > +       }


> Here as well. something maybe like tst_unload_module().

+1

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Cyril Hrubis]

Hi!
> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index dab811564..f2ba302e2 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -1609,6 +1609,11 @@ first missing driver.
>  The detection is based on reading 'modules.dep' and 'modules.builtin' files
>  generated by kmod. The check is skipped on Android.
>  
> +NULL terminated array '.modprobe' of kernel module names are tried to be loaded
                                                                  ^
							      attempted
> +with 'modprobe' unless they are builtin or already loaded. Test exits with
> +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
                                   ^     ^
		                   failure
> +loaded by the test unloaded with 'rmmod'.

During the cleanup modules that have been loaded are unloaded by 'modprobe -r'.

>  1.27 Saving & restoring /proc|sys values
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/doc/Test-Writing-Guidelines.asciidoc b/doc/Test-Writing-Guidelines.asciidoc
> index 0db852ae6..19487816e 100644
> --- a/doc/Test-Writing-Guidelines.asciidoc
> +++ b/doc/Test-Writing-Guidelines.asciidoc
> @@ -371,6 +371,7 @@ https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API].
>  | '.min_mem_avail' | not applicable
>  | '.mnt_flags' | 'TST_MNT_PARAMS'
>  | '.min_swap_avail' | not applicable
> +| '.modprobe' | –
>  | '.mntpoint', '.mnt_data' | 'TST_MNTPOINT'
>  | '.mount_device' | 'TST_MOUNT_DEVICE'
>  | '.needs_cgroup_ctrls' | –
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..6b4fac985 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -297,9 +297,12 @@ struct tst_test {
>  	/* NULL terminated array of resource file names */
>  	const char *const *resource_files;
>  
> -	/* NULL terminated array of needed kernel drivers */
> +	/* NULL terminated array of needed kernel drivers to be checked */
>  	const char * const *needs_drivers;

Do we need this array? Are there tests that needs to check for a module
but does not want the library to load them?

> +	/* NULL terminated array of needed kernel drivers to be loaded with modprobe */
> +	const char * const *modprobe;
> +
>  	/*
>  	 * {NULL, NULL} terminated array of (/proc, /sys) files to save
>  	 * before setup and restore after cleanup
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 087c62a16..ccbaa4c02 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
>  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
>  
>  #define DEFAULT_TIMEOUT 30
> +#define MODULES_MAX_LEN 10
>  
>  struct tst_test *tst_test;
>  
> @@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;
>  
>  static char shm_path[1024];
>  
> +static int modules_loaded[MODULES_MAX_LEN];
> +
>  int TST_ERR;
>  int TST_PASS;
>  long TST_RET;
> @@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
>  	tst_cg_init();
>  }
>  
> +/*
> + * Search kernel driver in /proc/modules.
> + *
> + * @param driver The name of the driver.
> + * @return 1 if driver is found, otherwise 0.
> + */
> +static int module_loaded(const char *driver)
> +{
> +	char line[4096];
> +	int found = 0;
> +	FILE *file = SAFE_FOPEN("/proc/modules", "r");
> +
> +	while (fgets(line, sizeof(line), file)) {
> +		if (strstr(line, driver)) {

Is this realy okay? What if a module name is a substring of other
module? We have module names that ar as short as two letters, for
instance with 'sg' or 'ac' there quite a bit of room for error.

We really need to find first whitespace in the line and replace it with
'\0' then do strcmp().

And we have to do the '_' and '-' permutations as well, as we do in the
code that checks for buildin modules.

Maybe we need module_strcmp(), that would work like strcmp() but would
handle the '-' and '_' as the same letter.

> +			found = 1;
> +			break;
> +		}
> +	}
> +	SAFE_FCLOSE(file);
> +
> +	return found;
> +}
> +
>  static void do_setup(int argc, char *argv[])
>  {
>  	if (!tst_test)
> @@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
>  			safe_check_driver(name);
>  	}
>  
> +	if (tst_test->modprobe) {
> +		const char *name;
> +		int i;
> +
> +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> +			/* only load module if not already loaded */
> +			if (!module_loaded(name) && tst_check_builtin_driver(name)) {
> +				const char *const cmd_modprobe[] = {"modprobe", name, NULL};
> +				SAFE_CMD(cmd_modprobe, NULL, NULL);

We are supposed to TCONF here if modprobe failed, so we have to check
the return value and tst_brk(TCONF, "...") when it's non-zero, right?

> +				modules_loaded[i] = 1;
> +			}
> +		}
> +	}
> +
>  	if (tst_test->mount_device)
>  		tst_test->format_device = 1;
>  
> @@ -1362,6 +1402,19 @@ static void do_cleanup(void)
>  
>  	tst_sys_conf_restore(0);
>  
> +	if (tst_test->modprobe) {
> +		const char *name;
> +		int i;
> +
> +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> +			if (!modules_loaded[i])
> +				continue;
> +
> +			const char *const cmd_rmmod[] = {"rmmod", name, NULL};

modprobe -r please, rmmod has been deprecated for ages.

> +			SAFE_CMD(cmd_rmmod, NULL, NULL);
> +		}
> +	}
> +
>  	if (tst_test->restore_wallclock)
>  		tst_wallclock_restore();
>  
> -- 
> 2.42.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Cyril Hrubis]

Hi!
> > What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
> > I guess we could make use of it widely for checking module loading or not.
> 
> I can do that, but lib/tst_kernel.c uses the old API. I guess it would fit
> better in lib/tst_module.c, but that also uses the old API. Most of the tests
> are converted, but at least these modules are still in the old API and use
> tst_module_load from tst_module.h:
>
> IMHO We need another file, which would be new API only. I'm also not sure if
> it's a good idea to put another file with just single function to it. We already
> have 38 lib/tst_*.c files which use new API. Any tip, what to use?
> Or should I really put it into lib/tst_module.c ain include/tst_module.h, but
> not into include/old/old_module.h (as we want old tests to be converted first?).

I would just put the new functions into tst_module.h and we can put the
into tst_module_new.c in lib/ and move the function to tst_module.c once
the tst_module.c has been converted to new API.

> > And here print the name to tell people the module is loaded.
> +1

+1

> > This part could be as a separate function like tst_load_module() and
> > built single into another lib. We prefer to keep the main tst_test.c
> > as a simple outline.
> 
> +1 for a separate function, it should be in the same file as
> tst_module_is_loaded().

+1

> > On the other hand, the extern functions can be used separately to let
> > modules to be loaded and unloaded during the test iteration.
> > It gives us more flexibility in test case design.
> 
> Having it as the separate function would allow to use it in
> kvm_pagefault01.c and zram03.c - tiny simplification as they now call
> SAFE_CMD().
> 
> kvm_pagefault01.c and can_common.h use them parameters, it might be worth
> to implement them.
> 
> > Print unload module name.
> +1

+1

> > > +               }
> > > +       }
> 
> 
> > Here as well. something maybe like tst_unload_module().
> 
> +1

+1


-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Cyril Hrubis]

Hi!
> >  	if (tst_test->mount_device)
> >  		tst_test->format_device = 1;
> >  
> > @@ -1362,6 +1402,19 @@ static void do_cleanup(void)
> >  
> >  	tst_sys_conf_restore(0);
> >  
> > +	if (tst_test->modprobe) {
> > +		const char *name;
> > +		int i;
> > +
> > +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +			if (!modules_loaded[i])
> > +				continue;
> > +
> > +			const char *const cmd_rmmod[] = {"rmmod", name, NULL};
> 
> modprobe -r please, rmmod has been deprecated for ages.

And one more minor point, we should attempt to remove the module only if
it has shown up in the /proc/modules.

Assuming that we want to skip the tst_module_is_buildin() check on some
systems as Ritchie suggested we would attempt to remove build in modules
here if we blindly trusted the return value from modpprobe.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/4] can_bcm01: Move vcan to .modprobe

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/network/can/cve/can_bcm01.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/testcases/network/can/cve/can_bcm01.c b/testcases/network/can/cve/can_bcm01.c
> index d9a835b03..ec98db133 100644
> --- a/testcases/network/can/cve/can_bcm01.c
> +++ b/testcases/network/can/cve/can_bcm01.c
> @@ -41,14 +41,6 @@ static void setup(void)
>  {
>  	struct sockaddr_can addr = { .can_family = AF_CAN };
>  
> -	/*
> -	 * Older kernels require explicit modprobe of vcan. Newer kernels
> -	 * will load the modules automatically and support CAN in network
> -	 * namespace which would eliminate the need for running the test
> -	 * with root privileges.
> -	 */
> -	tst_cmd((const char*[]){"modprobe", "vcan", NULL}, NULL, NULL, 0);
> -
>  	NETDEV_ADD_DEVICE(LTP_DEVICE, "vcan");
>  	NETDEV_SET_STATE(LTP_DEVICE, 1);
>  	addr.can_ifindex = NETDEV_INDEX_BY_NAME(LTP_DEVICE);
> @@ -143,10 +135,19 @@ static struct tst_test test = {
>  	.skip_in_compat = 1,
>  	.max_runtime = 30,
>  	.needs_drivers = (const char *const[]) {
> -		"vcan",
>  		"can-bcm",
>  		NULL
>  	},
> +	/*
> +	 * Older kernels require explicit modprobe of vcan. Newer kernels
> +	 * will load the modules automatically and support CAN in network
> +	 * namespace which would eliminate the need for running the test
> +	 * with root privileges.
> +	 */

This comment is wrong and can be removed. It also (or only?) depends on
kernel config whether modules are loaded automatically. It is a security
feature to remove automatic modprobe. IDK if older kernels lacked auto
module loading.

> +	.modprobe = (const char *const[]) {
> +		"vcan",
> +		NULL
> +	},
>  	.tags = (const struct tst_tag[]) {
>  		{"linux-git", "d5f9023fa61e"},
>  		{"CVE", "2021-3609"},
> -- 
> 2.42.0


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi Cyril,

thanks for your comments. More questions bellow.

...
> > +++ b/doc/C-Test-API.asciidoc
> > @@ -1609,6 +1609,11 @@ first missing driver.
> >  The detection is based on reading 'modules.dep' and 'modules.builtin' files
> >  generated by kmod. The check is skipped on Android.

> > +NULL terminated array '.modprobe' of kernel module names are tried to be loaded
>                                                                   ^
> 							      attempted
> > +with 'modprobe' unless they are builtin or already loaded. Test exits with
> > +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
>                                    ^     ^
> 		                   failure
> > +loaded by the test unloaded with 'rmmod'.

> During the cleanup modules that have been loaded are unloaded by 'modprobe -r'.

Thanks for the wording improvement. I also agree that 'modprobe -r' is probably
better than 'rmmod'. But we already have tst_module_unload_() in lib/tst_module.c
(file for both APIs). I guess I'll add new functions to lib/tst_kernel.c, which
is new API only and already has some module specific files (not ideal name, but
after everything using lib/tst_module.c is converted to the new API we can move
module specific files to lib/tst_module.c).

...
> > +++ b/include/tst_test.h
> > @@ -297,9 +297,12 @@ struct tst_test {
> >  	/* NULL terminated array of resource file names */
> >  	const char *const *resource_files;

> > -	/* NULL terminated array of needed kernel drivers */
> > +	/* NULL terminated array of needed kernel drivers to be checked */
> >  	const char * const *needs_drivers;

> Do we need this array? Are there tests that needs to check for a module
> but does not want the library to load them?

So you would, as a part of this change, replace .needs_drivers with .modprobe_module.

I'm not sure if it's a good idea. Some kernel modules are loaded on demand. If
we call modprobe, we will skip testing of this auto-load functionality.
What come to my mind are various modules required by certain socket() call, see
bind0[45].c., but they don't use .needs_drivers.

Other example is loop module in .needs_drivers (e.g. ioctl/ioctl_loop05.c and
others) which load loop module via ioctl(fd, LOOP_CONFIGURE, ...) or quotactl
tests.

IMHO zram03.c cannot just load module. zram-control hot_add/hot_remove
functionality was added in 6566d1a32bf7 ("zram: add dynamic device add/remove
functionality") in v4.2-rc1 - still too new to drop the support.
I still believe we can start with modprobe via .modprobe_module for zram03.c,
and then do the other checks (I'll try to send patch first with Cc: Yang Xu).

> > +	/* NULL terminated array of needed kernel drivers to be loaded with modprobe */
> > +	const char * const *modprobe;
...

> > +/*
> > + * Search kernel driver in /proc/modules.
> > + *
> > + * @param driver The name of the driver.
> > + * @return 1 if driver is found, otherwise 0.
> > + */
> > +static int module_loaded(const char *driver)
> > +{
> > +	char line[4096];
> > +	int found = 0;
> > +	FILE *file = SAFE_FOPEN("/proc/modules", "r");
> > +
> > +	while (fgets(line, sizeof(line), file)) {
> > +		if (strstr(line, driver)) {

> Is this realy okay? What if a module name is a substring of other
> module? We have module names that ar as short as two letters, for
> instance with 'sg' or 'ac' there quite a bit of room for error.

> We really need to find first whitespace in the line and replace it with
> '\0' then do strcmp().

+1

> And we have to do the '_' and '-' permutations as well, as we do in the
> code that checks for buildin modules.

Yep, that part has been solved in tst_search_driver_(), which is in
lib/tst_kernel.c, but that function is searching in /lib/modules.

> Maybe we need module_strcmp(), that would work like strcmp() but would
> handle the '-' and '_' as the same letter.

+1, it will be then used in two places.

> > +			found = 1;
> > +			break;
> > +		}
> > +	}
> > +	SAFE_FCLOSE(file);
> > +
> > +	return found;
> > +}
> > +
> >  static void do_setup(int argc, char *argv[])
> >  {
> >  	if (!tst_test)
> > @@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
> >  			safe_check_driver(name);
> >  	}

> > +	if (tst_test->modprobe) {
> > +		const char *name;
> > +		int i;
> > +
> > +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +			/* only load module if not already loaded */
> > +			if (!module_loaded(name) && tst_check_builtin_driver(name)) {
> > +				const char *const cmd_modprobe[] = {"modprobe", name, NULL};
> > +				SAFE_CMD(cmd_modprobe, NULL, NULL);

> We are supposed to TCONF here if modprobe failed, so we have to check
> the return value and tst_brk(TCONF, "...") when it's non-zero, right?

Yes, but see safe_cmd() in lib/tst_safe_macros.c. tst_cmd() is called with 
TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING, thus this is covered.
I agree SAFE_CMD() is confusing, I'll send a patch documenting this in
include/tst_safe_macros.h.

> > +				modules_loaded[i] = 1;
> > +			}
> > +		}
> > +	}
> > +
> >  	if (tst_test->mount_device)
> >  		tst_test->format_device = 1;

> > @@ -1362,6 +1402,19 @@ static void do_cleanup(void)

> >  	tst_sys_conf_restore(0);

> > +	if (tst_test->modprobe) {
> > +		const char *name;
> > +		int i;
> > +
> > +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > +			if (!modules_loaded[i])
> > +				continue;
> > +
> > +			const char *const cmd_rmmod[] = {"rmmod", name, NULL};

> modprobe -r please, rmmod has been deprecated for ages.

Ah, here goes the reason. Should it be replaced also in tst_module_unload_()?

Kind regards,
Petr

> > +			SAFE_CMD(cmd_rmmod, NULL, NULL);
> > +		}
> > +	}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Cyril Hrubis]

Hi!
> Yes, but see safe_cmd() in lib/tst_safe_macros.c. tst_cmd() is called with 
> TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING, thus this is covered.

TCONF_ON_MISSING means to TCONF when the command is missing, i.e. if
modprobe is not installed on the system. When module is missing modprobe
will return 1 so I suppose we have to handle the return value from the
TST_CMD() or do I missing something?

> > modprobe -r please, rmmod has been deprecated for ages.
> 
> Ah, here goes the reason. Should it be replaced also in tst_module_unload_()?

Yes please.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi Cyril,

> Hi!
> > Yes, but see safe_cmd() in lib/tst_safe_macros.c. tst_cmd() is called with 
> > TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING, thus this is covered.

> TCONF_ON_MISSING means to TCONF when the command is missing, i.e. if
> modprobe is not installed on the system. When module is missing modprobe
> will return 1 so I suppose we have to handle the return value from the
> TST_CMD() or do I missing something?

Ah yes, you're right:
modprobe: FATAL: Module foo not found in directory /lib/modules/6.5.0-1-amd64
uevent02.c:134: TBROK: modprobe failed (1)

But unfortunately we will have to parse error log, because exit code is still 1 [1]:

return err >= 0 ? EXIT_SUCCESS : EXIT_FAILURE;

And indeed, although this would be expected:

# modprobe foo; echo $?
modprobe: FATAL: Module foo not found in directory /lib/modules/6.5.0-1-amd64
1

This would deserve different exit code:

$ modprobe dccp; echo $?
modprobe: ERROR: could not insert 'dccp': Operation not permitted
1

> > > modprobe -r please, rmmod has been deprecated for ages.

> > Ah, here goes the reason. Should it be replaced also in tst_module_unload_()?

> Yes please.

+1

Kind regards,
Petr

[1] https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/tools/modprobe.c#n1047

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/4] can_bcm01: Move vcan to .modprobe

[Petr Vorel]

Hi Richie,

> Hello,

> Petr Vorel <pvorel@suse.cz> writes:

...
> >  testcases/network/can/cve/can_bcm01.c | 19 ++++++++++---------
> >  	addr.can_ifindex = NETDEV_INDEX_BY_NAME(LTP_DEVICE);
> > @@ -143,10 +135,19 @@ static struct tst_test test = {
> >  	.skip_in_compat = 1,
> >  	.max_runtime = 30,
> >  	.needs_drivers = (const char *const[]) {
> > -		"vcan",
> >  		"can-bcm",
> >  		NULL
> >  	},
> > +	/*
> > +	 * Older kernels require explicit modprobe of vcan. Newer kernels
> > +	 * will load the modules automatically and support CAN in network
> > +	 * namespace which would eliminate the need for running the test
> > +	 * with root privileges.
> > +	 */

> This comment is wrong and can be removed. It also (or only?) depends on
> kernel config whether modules are loaded automatically. It is a security
> feature to remove automatic modprobe. IDK if older kernels lacked auto
> module loading.

Yes, "blacklist foo" in /etc/modprobe.d/*.conf. Actually loading kernel modules
with modprobe will detect problems which checking with .needs_drivers or
.needs_kconfigs does not detect. e.g. the problems with missing modules on
openSUSE JeOS.

But as I wrote elsewhere [1], with explicit loading we don't test module
auto-loading. The approach we have now, that load modules only when needed is
IMHO better. But maybe I'm wrong.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20231103121201.GA1005170@pevik/

> > +	.modprobe = (const char *const[]) {
> > +		"vcan",
> > +		NULL
> > +	},
> >  	.tags = (const struct tst_tag[]) {
> >  		{"linux-git", "d5f9023fa61e"},
> >  		{"CVE", "2021-3609"},

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

Hi Cyril,

> Hi!
> > > What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
> > > I guess we could make use of it widely for checking module loading or not.

> > I can do that, but lib/tst_kernel.c uses the old API. I guess it would fit
> > better in lib/tst_module.c, but that also uses the old API. Most of the tests
> > are converted, but at least these modules are still in the old API and use
> > tst_module_load from tst_module.h:

> > IMHO We need another file, which would be new API only. I'm also not sure if
> > it's a good idea to put another file with just single function to it. We already
> > have 38 lib/tst_*.c files which use new API. Any tip, what to use?
> > Or should I really put it into lib/tst_module.c ain include/tst_module.h, but
> > not into include/old/old_module.h (as we want old tests to be converted first?).

> I would just put the new functions into tst_module.h and we can put the
> into tst_module_new.c in lib/ and move the function to tst_module.c once
> the tst_module.c has been converted to new API.

+1. IMHO some other functions from lib/tst_kernel.c should be moved there
(tst_search_driver(), tst_check_builtin_driver(), tst_check_driver()).

> > > And here print the name to tell people the module is loaded.
> > +1

...
Thanks for your input.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Petr Vorel]

> Hi!
> > >  	if (tst_test->mount_device)
> > >  		tst_test->format_device = 1;

> > > @@ -1362,6 +1402,19 @@ static void do_cleanup(void)

> > >  	tst_sys_conf_restore(0);

> > > +	if (tst_test->modprobe) {
> > > +		const char *name;
> > > +		int i;
> > > +
> > > +		for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > > +			if (!modules_loaded[i])
> > > +				continue;
> > > +
> > > +			const char *const cmd_rmmod[] = {"rmmod", name, NULL};

> > modprobe -r please, rmmod has been deprecated for ages.

> And one more minor point, we should attempt to remove the module only if
> it has shown up in the /proc/modules.

+1

> Assuming that we want to skip the tst_module_is_buildin() check on some
> systems as Ritchie suggested we would attempt to remove build in modules
> here if we blindly trusted the return value from modpprobe.

I guess for most of distros tst_check_builtin_driver() (which reads
modules.builtin) makes sense. And with it we will have valid info if we should
remove module or not.

Then there is:

1) AOSP (Android), we should ask Edward what makes sense in Android.
IMHO old AOSP versions used insmod and rmmod, but newer could support it [2].
@Edward, am I correct? Also do AOSP even care about tests which use
tst_module_unload()?

2) NixOS
This should be IMHO fixed by checking also the correct directory (ideally
wrapped by some #ifdef, but can be even without it, if there is none).

BTW, there is also /proc/sys/kernel/modules_disabled [1], I'm not sure if we
want to just ignore it.

Kind regards,
Petr

[1] https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
[2] https://source.android.com/docs/core/architecture/kernel/loadable-kernel-modules

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/4] lib: Add .modprobe

[Edward Liaw via ltp]

Hi Petr,

On Fri, Nov 3, 2023 at 8:54 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> > Hi!
> > > >   if (tst_test->mount_device)
> > > >           tst_test->format_device = 1;
>
> > > > @@ -1362,6 +1402,19 @@ static void do_cleanup(void)
>
> > > >   tst_sys_conf_restore(0);
>
> > > > + if (tst_test->modprobe) {
> > > > +         const char *name;
> > > > +         int i;
> > > > +
> > > > +         for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> > > > +                 if (!modules_loaded[i])
> > > > +                         continue;
> > > > +
> > > > +                 const char *const cmd_rmmod[] = {"rmmod", name, NULL};
>
> > > modprobe -r please, rmmod has been deprecated for ages.
>
> > And one more minor point, we should attempt to remove the module only if
> > it has shown up in the /proc/modules.
>
> +1
>
> > Assuming that we want to skip the tst_module_is_buildin() check on some
> > systems as Ritchie suggested we would attempt to remove build in modules
> > here if we blindly trusted the return value from modpprobe.
>
> I guess for most of distros tst_check_builtin_driver() (which reads
> modules.builtin) makes sense. And with it we will have valid info if we should
> remove module or not.
>
> Then there is:
>
> 1) AOSP (Android), we should ask Edward what makes sense in Android.
> IMHO old AOSP versions used insmod and rmmod, but newer could support it [2].
> @Edward, am I correct? Also do AOSP even care about tests which use
> tst_module_unload()?

Android supports modprobe -r, but we're not currently running any of
the tests that use tst_module_unload because we're not building the
test modules.  I'll see if I can fix that, though.

>
> 2) NixOS
> This should be IMHO fixed by checking also the correct directory (ideally
> wrapped by some #ifdef, but can be even without it, if there is none).
>
> BTW, there is also /proc/sys/kernel/modules_disabled [1], I'm not sure if we
> want to just ignore it.
>
> Kind regards,
> Petr
>
> [1] https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
> [2] https://source.android.com/docs/core/architecture/kernel/loadable-kernel-modules

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp