diff mbox series

[2/4] lib: Add .modprobe

Message ID 20231013074748.702214-3-pvorel@suse.cz
State Changes Requested
Headers show
Series Add .modprobe (loading modules in C API) | expand

Commit Message

Petr Vorel Oct. 13, 2023, 7:47 a.m. UTC
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(-)

Comments

Li Wang Oct. 13, 2023, 12:09 p.m. UTC | #1
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
>
Petr Vorel Oct. 13, 2023, 12:22 p.m. UTC | #2
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;
> > +}
Petr Vorel Oct. 13, 2023, 12:30 p.m. UTC | #3
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;
> +			}
> +		}
> +	}
> +
Li Wang Oct. 13, 2023, 1:27 p.m. UTC | #4
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;
> > +                     }
> > +             }
> > +     }
> > +
>
>
Petr Vorel Oct. 13, 2023, 1:50 p.m. UTC | #5
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
Li Wang Oct. 16, 2023, 8:28 a.m. UTC | #6
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.
Li Wang Oct. 16, 2023, 9:05 a.m. UTC | #7
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.
Petr Vorel Oct. 27, 2023, 12:01 p.m. UTC | #8
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
Cyril Hrubis Nov. 1, 2023, 4:26 p.m. UTC | #9
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 Nov. 1, 2023, 4:33 p.m. UTC | #10
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 Nov. 1, 2023, 4:35 p.m. UTC | #11
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.
Petr Vorel Nov. 3, 2023, 12:12 p.m. UTC | #12
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);
> > +		}
> > +	}
Cyril Hrubis Nov. 3, 2023, 12:21 p.m. UTC | #13
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.
Petr Vorel Nov. 3, 2023, 2:58 p.m. UTC | #14
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
Petr Vorel Nov. 3, 2023, 3:22 p.m. UTC | #15
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
Petr Vorel Nov. 3, 2023, 3:54 p.m. UTC | #16
> 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
Edward Liaw Nov. 3, 2023, 4:31 p.m. UTC | #17
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
diff mbox series

Patch

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();