| Message ID | 20230314093725.3814-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5] madvise11.c:Check loadable module before rmmod | expand |
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 */
Hi! The changes looks good, however I would split the patch into the part that addes new function into the library and second patch should add the additional check to the madvise test.
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. + * + * @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. * 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 */
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(-)