Re: [LTP] [PATCH v5] madvise11.c:Check loadable module before rmmod

[Petr Vorel <pvorel@suse.cz>]

Hi all,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

FYI although it turned out that CONFIG_HWPOISON_INJECT=y in one of our kernels
was a mistake (will be changed to CONFIG_HWPOISON_INJECT=m, which is the default
since kernel ~ 3.14 thus nobody noticed), the test still should work with
CONFIG_HWPOISON_INJECT=y.

BTW I'd personally put madvise11.c change to separate commit.

nit: the title should be:

madvise11: Check if module is loadable before rmmod

> Following fail msg will popup if we try to rmmod buildin module:
> rmmod: ERROR: Module hwpoison_inject is builtin

> So need add extra check.

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  include/tst_kernel.h                          | 10 ++++++
>  lib/tst_kernel.c                              | 36 +++++++++++--------
>  testcases/kernel/syscalls/madvise/madvise11.c |  2 +-
>  3 files changed, 33 insertions(+), 15 deletions(-)

> diff --git a/include/tst_kernel.h b/include/tst_kernel.h
> index 9e17bb71e..0c1262e2f 100644
> --- a/include/tst_kernel.h
> +++ b/include/tst_kernel.h
> @@ -10,6 +10,16 @@
>   */
>  int tst_kernel_bits(void);

> +/*
> + * Checks builtin module for the kernel driver.
Check if the kernel module is built-in .
> + *
> + * @param driver The name of the driver.
> + * @return Returns 0 if builtin driver
> + * -1 when driver is missing or config file not available.
> + * On Android *always* 0 (always expect the driver is available).
> + */
> +int tst_check_builtin_driver(const char *driver);
> +
>  /**
>   * Checks support for the kernel driver.
Checks support for the kernel module (both built-in and loadable).

I'm willing to do the changes before merge.

Kind regards,
Petr

>   *
> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> index ecf4b917e..be3ec92da 100644
> --- a/lib/tst_kernel.c
> +++ b/lib/tst_kernel.c
> @@ -90,7 +90,7 @@ int tst_kernel_bits(void)
>  	return kernel_bits;
>  }

> -static int tst_search_driver(const char *driver, const char *file)
> +static int tst_search_driver_(const char *driver, const char *file)
>  {
>  	struct stat st;
>  	char buf[PATH_MAX];
> @@ -144,28 +144,19 @@ static int tst_search_driver(const char *driver, const char *file)
>  	return ret;
>  }

> -static int tst_check_driver_(const char *driver)
> -{
> -	if (!tst_search_driver(driver, "modules.dep") ||
> -		!tst_search_driver(driver, "modules.builtin"))
> -		return 0;
> -
> -	return -1;
> -}
> -
> -int tst_check_driver(const char *driver)
> +static int tst_search_driver(const char *driver, const char *file)
>  {
>  #ifdef __ANDROID__
>  	/*
>  	 * Android may not have properly installed modules.* files. We could
> -	 * search modules in /system/lib/modules, but to to determine built-in
> +	 * search modules in /system/lib/modules, but to determine built-in
>  	 * drivers we need modules.builtin. Therefore assume all drivers are
>  	 * available.
>  	 */
>  	return 0;
>  #endif

> -	if (!tst_check_driver_(driver))
> +	if (!tst_search_driver_(driver, file))
>  		return 0;

>  	int ret = -1;
> @@ -183,9 +174,26 @@ int tst_check_driver(const char *driver)
>  		while ((ix = strchr(ix, find)))
>  			*ix++ = replace;

> -		ret = tst_check_driver_(driver2);
> +		ret = tst_search_driver_(driver2, file);
>  		free(driver2);
>  	}

>  	return ret;
>  }
> +
> +int tst_check_builtin_driver(const char *driver)
> +{
> +	if (!tst_search_driver(driver, "modules.builtin"))
> +		return 0;
> +
> +	return -1;
> +}
> +
> +int tst_check_driver(const char *driver)
> +{
> +	if (!tst_search_driver(driver, "modules.dep") ||
> +		!tst_search_driver(driver, "modules.builtin"))
> +		return 0;
> +
> +	return -1;
> +}
> diff --git a/testcases/kernel/syscalls/madvise/madvise11.c b/testcases/kernel/syscalls/madvise/madvise11.c
> index 7e291d571..4d99d5289 100644
> --- a/testcases/kernel/syscalls/madvise/madvise11.c
> +++ b/testcases/kernel/syscalls/madvise/madvise11.c
> @@ -300,7 +300,7 @@ static int open_unpoison_pfn(void)
>  	struct mntent *mnt;
>  	FILE *mntf;

> -	if (!find_in_file("/proc/modules", HW_MODULE))
> +	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 */

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