| Message ID | 20220630141226.2802-1-mpdesouza@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | livepatch: Move tests from lib/livepatch to selftests/livepatch | expand |
On 6/30/22 8:12 AM, Marcos Paulo de Souza wrote: > Hi there, > > this is the v2 of the patchset. The v1 can be found at [1]. There is only one > change in patch 1, which changed the target directory to build the test modules. > All other changes happen in patch 2. > > Thanks for reviewing! > > Changes from v1: > # test_modules/Makefile > * Build the test modules targeting /lib/modules, instead of ksrc when building > from the kernel source. > > # test_modules/test_klp_syscall.c > * Added a parameter array to receive the pids that should transition to the > new system call. (suggedted by Joe) > * Create a new sysfs file /sys/kernel/test_klp_syscall/npids to show how many > pids from the argument need to transition to the new state. (suggested by > Joe) > * Fix the PPC32 support by adding the syscall wrapper for archs that select it > by default, without erroring out. PPC does not set SYSCALL_WRAPPER, so > having it set in v1 was a mistake. (suggested by Joe) > * The aarch64 syscall prefix was added too, since the livepatch support will come soon. > > # test_binaries/test_klp-call_getpid.c > * Change %d/%u in printf (suggested byu Joe) > * Change run -> stop variable name, and inverted the assignments (suggested by > * Joe). > > # File test-syscall.sh > * Fixed test-syscall.sh to call test_klp-call-getpid in test_binaries dir > * Load test_klp_syscall passed the pids of the test_klp-call_getpid instances. > Check the sysfs file from test_klp_syscall module to check that all pids > transitioned correctly. (suggested by Joe) > * Simplified the loop that calls test_klp-call_getpid. (suggested by Joe) > * Removed the "success" comment from the script, as it's implicit that it > succeed. Otherwise load_lp would error out. (suggested by Joe) > > * Changed the commit message of patch 2 to further detail what means "tricky" > when livepatching syscalls. (suggested by Joe) > > [1]: 20220603143242.870-1-mpdesouza@suse.com > Sorry Nack on this. Let's not add modules under selftests. Any usage of module_init() doesn't belong under selftests. Leave these under lib and use KSTM_MODULE_LOADERS to load these modules that live under lib. thanks, -- Shuah
Hi Shuah, On Thu, 30 Jun 2022, Shuah Khan wrote: > On 6/30/22 8:12 AM, Marcos Paulo de Souza wrote: > > Hi there, > > > > this is the v2 of the patchset. The v1 can be found at [1]. There is only > > one > > change in patch 1, which changed the target directory to build the test > > modules. > > All other changes happen in patch 2. > > > > Thanks for reviewing! > > > > Changes from v1: > > # test_modules/Makefile > > * Build the test modules targeting /lib/modules, instead of ksrc when > > building > > from the kernel source. > > > > # test_modules/test_klp_syscall.c > > * Added a parameter array to receive the pids that should transition to > > the > > new system call. (suggedted by Joe) > > * Create a new sysfs file /sys/kernel/test_klp_syscall/npids to show how > > many > > pids from the argument need to transition to the new state. (suggested > > by > > Joe) > > * Fix the PPC32 support by adding the syscall wrapper for archs that > > select it > > by default, without erroring out. PPC does not set SYSCALL_WRAPPER, so > > having it set in v1 was a mistake. (suggested by Joe) > > * The aarch64 syscall prefix was added too, since the livepatch support > > will come soon. > > > > # test_binaries/test_klp-call_getpid.c > > * Change %d/%u in printf (suggested byu Joe) > > * Change run -> stop variable name, and inverted the assignments > > (suggested by > > * Joe). > > > > # File test-syscall.sh > > * Fixed test-syscall.sh to call test_klp-call-getpid in test_binaries dir > > * Load test_klp_syscall passed the pids of the test_klp-call_getpid > > instances. > > Check the sysfs file from test_klp_syscall module to check that all > > pids > > transitioned correctly. (suggested by Joe) > > * Simplified the loop that calls test_klp-call_getpid. (suggested by Joe) > > * Removed the "success" comment from the script, as it's implicit that it > > succeed. Otherwise load_lp would error out. (suggested by Joe) > > > > * Changed the commit message of patch 2 to further detail what means > > "tricky" > > when livepatching syscalls. (suggested by Joe) > > > > [1]: 20220603143242.870-1-mpdesouza@suse.com > > > > Sorry Nack on this. Let's not add modules under selftests. Any usage of > module_init() > doesn't belong under selftests. as mentioned before, that ship has already sailed with tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c. Anyway... You wrote before that you did not have a problem with it. And you would not have a problem with Marcos' approach if modules can be compiled and if not, the tests would fail gracefully. What has changed? If you see a problem in the patch set regarding this, can we fix it? > Leave these under lib and use KSTM_MODULE_LOADERS to load these modules that > live under lib. I may misunderstand but KSTM_MODULE_LOADERS does not seem to provide the flexibility we need (yes, it could be hacked around, but I do not think that the result would be nice). See what we have in tools/testing/selftests/livepatch/functions.sh to make sure that a live patch module is properly loaded and unloaded. My main question is different though. As Marcos mentioned before, we would like to have our tests really flexible and a possibility to prepare and load different live patch modules based on a template is a part of it. What is your proposal regarding this? I can imagine having a template in lib/livepatch/ which would not be compilable and a script in tools/testing/selftests/livepatch/ would copy it many times, amend the copies (meaning parameters would be filled in with sed or the code would be changed), compile them and load them. But this sounds horrible to me, especially when compared to Marcos' approach in this patch set which is quite straightforward. Then there is an opportunity which Joe described. To run the latest livepatch kselftests on an older kernel. Having test modules in lib/ is kind of an obstacle there. Regards Miroslav
On 7/1/22 1:48 AM, Miroslav Benes wrote: > Hi Shuah, > > On Thu, 30 Jun 2022, Shuah Khan wrote: > >> >> Sorry Nack on this. Let's not add modules under selftests. Any usage of >> module_init() >> doesn't belong under selftests. > > as mentioned before, that ship has already sailed with > tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c. Anyway... > Just because of one module under bpf doesn't mean that now we can add more. bpf test is some ways is not a good example or model to use. bpf test requires specific environment and its needs are different from other tests. > You wrote before that you did not have a problem with it. And you would > not have a problem with Marcos' approach if modules can be compiled and if > not, the tests would fail gracefully. What has changed? If you see a > problem in the patch set regarding this, can we fix it? > Yes I did and after reviewing and thinking about it some more, I decided this is the right direction go down on. >> Leave these under lib and use KSTM_MODULE_LOADERS to load these modules that >> live under lib. > > I may misunderstand but KSTM_MODULE_LOADERS does not seem to provide the > flexibility we need (yes, it could be hacked around, but I do not think > that the result would be nice). See what we have in > tools/testing/selftests/livepatch/functions.sh to make sure that a live > patch module is properly loaded and unloaded. > > My main question is different though. As Marcos mentioned before, we would > like to have our tests really flexible and a possibility to prepare and > load different live patch modules based on a template is a part of it. > What is your proposal regarding this? I can imagine having a template in > lib/livepatch/ which would not be compilable and a script in > tools/testing/selftests/livepatch/ would copy it many times, amend the > copies (meaning parameters would be filled in with sed or the code would > be changed), compile them and load them. But this sounds horrible to me, > especially when compared to Marcos' approach in this patch set which is > quite straightforward. > I have to think about this some more to get a better feel for the use-case. > Then there is an opportunity which Joe described. To run the latest > livepatch kselftests on an older kernel. Having test modules in lib/ is > kind of an obstacle there. > You can revision match if you think you have to have kernel and livepatch test be the same version. thanks, -- Shuah
On Fri 2022-07-01 16:13:50, Shuah Khan wrote: > On 7/1/22 1:48 AM, Miroslav Benes wrote: > > On Thu, 30 Jun 2022, Shuah Khan wrote: > > > > > > Sorry Nack on this. Let's not add modules under selftests. Any usage of > > > module_init() > > > doesn't belong under selftests. > > Yes I did and after reviewing and thinking about it some more, I decided this > is the right direction go down on. Do you have some particular reason why building modules in selftests directory might cause problems, please? IMHO, the reason that the test modules are in lib is because the modules were there before selftests. Developers historically loaded them manually or they were built-in. Selftest were added later and are just another way how the module can be loaded. This is the case, for example, for lib/test_printf.c. Otherwise, I do not see any big difference between building binaries and modules under tools/tests/selftests. As I said, in the older thread, IMHO, it makes more sense to have the selftest sources self-contained. There actually seems to be a principal problem in the following use case: --- cut Documentation/dev-tools/kselftest.rst --- Kselftest from mainline can be run on older stable kernels. Running tests from mainline offers the best coverage. Several test rings run mainline kselftest suite on stable releases. The reason is that when a new test gets added to test existing code to regression test a bug, we should be able to run that test on an older kernel. Hence, it is important to keep code that can still test an older kernel and make sure it skips the test gracefully on newer releases. --- cut Documentation/dev-tools/kselftest.rst --- together with --- cut Documentation/dev-tools/kselftest.rst --- * First use the headers inside the kernel source and/or git repo, and then the system headers. Headers for the kernel release as opposed to headers installed by the distro on the system should be the primary focus to be able to find regressions. --- cut Documentation/dev-tools/kselftest.rst --- It means that selftests should support running binaries built against newer kernel sources on system running older kernel. But this might be pretty hard to achieve and maintain. The normal kernel rules are exactly the opposite. Old binaries must be able to run on newer kernels. The old binaries were built against older headers. IMHO, the testing of stable kernels makes perfect sense. And if we want to support it seriously than we need to allow building new selftests against headers from the old to-be-tested kernel. And it will be possible only when the selftests sources are as much selfcontained as possible. Does this makes any sense, please? Best Regards, Petr
On 7/15/22 10:45 AM, Petr Mladek wrote: > On Fri 2022-07-01 16:13:50, Shuah Khan wrote: >> On 7/1/22 1:48 AM, Miroslav Benes wrote: >>> On Thu, 30 Jun 2022, Shuah Khan wrote: >>>> >>>> Sorry Nack on this. Let's not add modules under selftests. Any usage of >>>> module_init() >>>> doesn't belong under selftests. >> >> Yes I did and after reviewing and thinking about it some more, I decided this >> is the right direction go down on. > > Do you have some particular reason why building modules in selftests > directory might cause problems, please? > > IMHO, the reason that the test modules are in lib is because the > modules were there before selftests. Developers historically loaded them > manually or they were built-in. Selftest were added later and are just > another way how the module can be loaded. This is the case, > for example, for lib/test_printf.c. > > Otherwise, I do not see any big difference between building binaries > and modules under tools/tests/selftests. As I said, in the older > thread, IMHO, it makes more sense to have the selftest sources > self-contained. > > > There actually seems to be a principal problem in the following use > case: > > --- cut Documentation/dev-tools/kselftest.rst --- > Kselftest from mainline can be run on older stable kernels. Running tests > from mainline offers the best coverage. Several test rings run mainline > kselftest suite on stable releases. The reason is that when a new test > gets added to test existing code to regression test a bug, we should be > able to run that test on an older kernel. Hence, it is important to keep > code that can still test an older kernel and make sure it skips the test > gracefully on newer releases. > --- cut Documentation/dev-tools/kselftest.rst --- > > together with > > --- cut Documentation/dev-tools/kselftest.rst --- > * First use the headers inside the kernel source and/or git repo, and then the > system headers. Headers for the kernel release as opposed to headers > installed by the distro on the system should be the primary focus to be able > to find regressions. > --- cut Documentation/dev-tools/kselftest.rst --- > > It means that selftests should support running binaries built against > newer kernel sources on system running older kernel. But this might > be pretty hard to achieve and maintain. > > The normal kernel rules are exactly the opposite. Old binaries must > be able to run on newer kernels. The old binaries were built against > older headers. > > IMHO, the testing of stable kernels makes perfect sense. And if we > want to support it seriously than we need to allow building new > selftests against headers from the old to-be-tested kernel. And > it will be possible only when the selftests sources are as much > selfcontained as possible. > > Does this makes any sense, please? > Gentle bump. Shuah, I believe that Marcos will be preparing a v3 based on review comments on the second patch. We never resolved questions surrounding building modules selftests/ (the first patch) though. Regards,
On 11/30/22 15:22, Joe Lawrence wrote: > On 7/15/22 10:45 AM, Petr Mladek wrote: >> On Fri 2022-07-01 16:13:50, Shuah Khan wrote: >>> On 7/1/22 1:48 AM, Miroslav Benes wrote: >>>> On Thu, 30 Jun 2022, Shuah Khan wrote: >>>>> >>>>> Sorry Nack on this. Let's not add modules under selftests. Any usage of >>>>> module_init() >>>>> doesn't belong under selftests. >>> >>> Yes I did and after reviewing and thinking about it some more, I decided this >>> is the right direction go down on. >> >> Do you have some particular reason why building modules in selftests >> directory might cause problems, please? >> My reasons are that with this change module_init() propagates out of strictly kernel space and now is in selftests which are user-space. Any changes to this interface will be tied to user-space change. This is my main concern. That is reason why I still ask the question about why is it necessary to make this change other than self-contained sources? >> IMHO, the reason that the test modules are in lib is because the >> modules were there before selftests. Developers historically loaded them >> manually or they were built-in. Selftest were added later and are just >> another way how the module can be loaded. This is the case, >> for example, for lib/test_printf.c. >> >> Otherwise, I do not see any big difference between building binaries >> and modules under tools/tests/selftests. As I said, in the older >> thread, IMHO, it makes more sense to have the selftest sources >> self-contained. Modules under lib are built when kernel gets built as opposed to when tests are built. So there is the difference in build order. I do see a difference from that point of view. Yes, moving modules under selftests does make the tests self contained. >> >> >> There actually seems to be a principal problem in the following use >> case: >> >> --- cut Documentation/dev-tools/kselftest.rst --- >> Kselftest from mainline can be run on older stable kernels. Running tests >> from mainline offers the best coverage. Several test rings run mainline >> kselftest suite on stable releases. The reason is that when a new test >> gets added to test existing code to regression test a bug, we should be >> able to run that test on an older kernel. Hence, it is important to keep >> code that can still test an older kernel and make sure it skips the test >> gracefully on newer releases. >> --- cut Documentation/dev-tools/kselftest.rst --- >> >> together with >> >> --- cut Documentation/dev-tools/kselftest.rst --- >> * First use the headers inside the kernel source and/or git repo, and then the >> system headers. Headers for the kernel release as opposed to headers >> installed by the distro on the system should be the primary focus to be able >> to find regressions. >> --- cut Documentation/dev-tools/kselftest.rst --- >> >> It means that selftests should support running binaries built against >> newer kernel sources on system running older kernel. But this might >> be pretty hard to achieve and maintain. >> >> The normal kernel rules are exactly the opposite. Old binaries must >> be able to run on newer kernels. The old binaries were built against >> older headers. >> This case is applicable to when tests are built on a development system and binaries are moved to run on a target system. In general, newer tests offer the best coverage, hence the recommendation to run newer tests on older kernels assuming that the tests are built on a newer kernel and backwards should run in a backwards compatible way on older kernels. Your use-case might be different from this where you do build tests on older kernels and run them on it in which case, you might have a requirement to revision match the tests and kernel. You can still do so. >> IMHO, the testing of stable kernels makes perfect sense. And if we >> want to support it seriously than we need to allow building new >> selftests against headers from the old to-be-tested kernel. And >> it will be possible only when the selftests sources are as much >> selfcontained as possible. >> Do you have a requirement that livepatch test has to be revision matched with the kernel? Even if that is the case, there is no real reason to move modules under selftests other than keeping them in one location. Also I didn't see any changes to README that explains this move and that this change now makes this test now depends on kernel only interfaces and hence will have to follow modules built outside of kernel build. >> Does this makes any sense, please? >> > > Gentle bump. Shuah, I believe that Marcos will be preparing a v3 based > on review comments on the second patch. We never resolved questions > surrounding building modules selftests/ (the first patch) though. > You can send patches again and I would like to hear good reasons other than self-containing the sources. thanks, -- Shuah
On Thu, 1 Dec 2022, Shuah Khan wrote: > On 11/30/22 15:22, Joe Lawrence wrote: > > On 7/15/22 10:45 AM, Petr Mladek wrote: > >> On Fri 2022-07-01 16:13:50, Shuah Khan wrote: > >>> On 7/1/22 1:48 AM, Miroslav Benes wrote: > >>>> On Thu, 30 Jun 2022, Shuah Khan wrote: > >>>>> > >>>>> Sorry Nack on this. Let's not add modules under selftests. Any usage of > >>>>> module_init() > >>>>> doesn't belong under selftests. > >>> > >>> Yes I did and after reviewing and thinking about it some more, I decided > >>> this > >>> is the right direction go down on. > >> > >> Do you have some particular reason why building modules in selftests > >> directory might cause problems, please? > >> > > My reasons are that with this change module_init() propagates out of > strictly kernel space and now is in selftests which are user-space. > Any changes to this interface will be tied to user-space change. I do not understand this (have not had a cup of tea yet). module_init() is defined in include/linux/module.h. It is API. Every kernel module, in-tree or out-of-tree, uses it. There is only one usage of module_init() in Marcos's patch. In a kernel module, in tools/ subdirectory yes. If there is a change to module_init, it might need editing all the call sites, yes. That is inherent. > This is my main concern. That is reason why I still ask the question > about why is it necessary to make this change other than self-contained > sources? I will quote myself from an earlier email in the thread which you have not replied to... " My main question is different though. As Marcos mentioned before, we would like to have our tests really flexible and a possibility to prepare and load different live patch modules based on a template is a part of it. What is your proposal regarding this? I can imagine having a template in lib/livepatch/ which would not be compilable and a script in tools/testing/selftests/livepatch/ would copy it many times, amend the copies (meaning parameters would be filled in with sed or the code would be changed), compile them and load them. But this sounds horrible to me, especially when compared to Marcos' approach in this patch set which is quite straightforward. " Thank you Miroslav
On Thu 2022-12-01 16:58:38, Shuah Khan wrote: > On 11/30/22 15:22, Joe Lawrence wrote: > > On 7/15/22 10:45 AM, Petr Mladek wrote: > > > On Fri 2022-07-01 16:13:50, Shuah Khan wrote: > > > > On 7/1/22 1:48 AM, Miroslav Benes wrote: > > > > > On Thu, 30 Jun 2022, Shuah Khan wrote: > > > > > > > > > > > > Sorry Nack on this. Let's not add modules under selftests. Any usage of > > > > > > module_init() > > > > > > doesn't belong under selftests. > > > > > > > > Yes I did and after reviewing and thinking about it some more, I decided this > > > > is the right direction go down on. > > > > > > Do you have some particular reason why building modules in selftests > > > directory might cause problems, please? > > > > > My reasons are that with this change module_init() propagates out of > strictly kernel space and now is in selftests which are user-space. > Any changes to this interface will be tied to user-space change. I am sorry but I do not understand the meaning here. module_init() is called when module is loaded. It is not called in userspace. Maybe, you mean that modules under lib/ are clearly in-tree modules. If we move then under tools/ then they will be build like out-of-tree modules. Except that they will be maintained in-tree so that it will be easy to keep them in sync. And I am sure that they will be actively maintained. The fixes are there to make sure that livepatching still works as expected. They must pass when any change is done in the livepatch subsystem. And they must pass when any kernel is released. The only concern might be how build failure is handled. IMHO, we need to handle it the same way and test failure. > In general, newer tests offer the best coverage, hence the recommendation > to run newer tests on older kernels assuming that the tests are built > on a newer kernel and backwards should run in a backwards compatible > way on older kernels. This works for the userspace interface that should always be backward compatible. But it does not work for kABI. > Do you have a requirement that livepatch test has to be revision > matched with the kernel? Even if that is the case, there is no real > reason to move modules under selftests other than keeping them in > one location. Yes, kABI is not backward compatible. But building the tests modules out-of-tree way would allow to build test modules with different kABI from the same sources. IMHO, this is common for any selttests using kernel modules. I am sure that neither test_printf nor test_scanf modules could be loaded in older kernels. Even though it might be useful to run improved tests for stable-like kernels. Best Regards, Petr
On 12/2/22 02:25, Petr Mladek wrote: > On Thu 2022-12-01 16:58:38, Shuah Khan wrote: >> On 11/30/22 15:22, Joe Lawrence wrote: >>> On 7/15/22 10:45 AM, Petr Mladek wrote: >>>> On Fri 2022-07-01 16:13:50, Shuah Khan wrote: >>>>> On 7/1/22 1:48 AM, Miroslav Benes wrote: >>>>>> On Thu, 30 Jun 2022, Shuah Khan wrote: >>>>>>> >>>>>>> Sorry Nack on this. Let's not add modules under selftests. Any usage of >>>>>>> module_init() >>>>>>> doesn't belong under selftests. >>>>> >>>>> Yes I did and after reviewing and thinking about it some more, I decided this >>>>> is the right direction go down on. >>>> >>>> Do you have some particular reason why building modules in selftests >>>> directory might cause problems, please? >>>> >> >> My reasons are that with this change module_init() propagates out of >> strictly kernel space and now is in selftests which are user-space. >> Any changes to this interface will be tied to user-space change. > > I am sorry but I do not understand the meaning here. module_init() is > called when module is loaded. It is not called in userspace. > > Maybe, you mean that modules under lib/ are clearly in-tree > modules. If we move then under tools/ then they will be build > like out-of-tree modules. Except that they will be maintained in-tree > so that it will be easy to keep them in sync. > Yes. That is what I mean. > And I am sure that they will be actively maintained. The fixes are > there to make sure that livepatching still works as expected. > They must pass when any change is done in the livepatch subsystem. > And they must pass when any kernel is released. > In other words, livepatch and kernel are revision matched. > The only concern might be how build failure is handled. IMHO, we > need to handle it the same way and test failure. > > Yes. This is another concern - build failures. Let's experiment with modules under selftests and see if this becomes a problem. >> In general, newer tests offer the best coverage, hence the recommendation >> to run newer tests on older kernels assuming that the tests are built >> on a newer kernel and backwards should run in a backwards compatible >> way on older kernels. > > This works for the userspace interface that should always be backward > compatible. But it does not work for kABI. > This is broader than revision matching. Tests should gracefully exit with skip when a config option they depend on is disabled. The same gets extended to older kernel versions. > >> Do you have a requirement that livepatch test has to be revision >> matched with the kernel? Even if that is the case, there is no real >> reason to move modules under selftests other than keeping them in >> one location. > > Yes, kABI is not backward compatible. But building the tests > modules out-of-tree way would allow to build test modules with > different kABI from the same sources. > Okay. This is a solid reason for livepatch modules to live under sefltests. Let's capture this in README and the other updates that need to be made to it in v3. thanks, -- Shuah
On 12/2/22 00:33, Miroslav Benes wrote: > On Thu, 1 Dec 2022, Shuah Khan wrote: > >> On 11/30/22 15:22, Joe Lawrence wrote: >>> On 7/15/22 10:45 AM, Petr Mladek wrote: >>>> On Fri 2022-07-01 16:13:50, Shuah Khan wrote: >>>>> On 7/1/22 1:48 AM, Miroslav Benes wrote: >>>>>> On Thu, 30 Jun 2022, Shuah Khan wrote: >>>>>>> >>>>>>> Sorry Nack on this. Let's not add modules under selftests. Any usage of >>>>>>> module_init() >>>>>>> doesn't belong under selftests. >>>>> >>>>> Yes I did and after reviewing and thinking about it some more, I decided >>>>> this >>>>> is the right direction go down on. >>>> >>>> Do you have some particular reason why building modules in selftests >>>> directory might cause problems, please? >>>> >> >> My reasons are that with this change module_init() propagates out of >> strictly kernel space and now is in selftests which are user-space. >> Any changes to this interface will be tied to user-space change. > > I do not understand this (have not had a cup of tea yet). > > module_init() is defined in include/linux/module.h. It is API. Every > kernel module, in-tree or out-of-tree, uses it. There is only one usage of > module_init() in Marcos's patch. In a kernel module, in tools/ > subdirectory yes. > > If there is a change to module_init, it might need editing all the call > sites, yes. That is inherent. > >> This is my main concern. That is reason why I still ask the question >> about why is it necessary to make this change other than self-contained >> sources? > > I will quote myself from an earlier email in the thread which you have not > replied to... > > " > My main question is different though. As Marcos mentioned before, we would > like to have our tests really flexible and a possibility to prepare and > load different live patch modules based on a template is a part of it. > What is your proposal regarding this? I can imagine having a template in > lib/livepatch/ which would not be compilable and a script in > tools/testing/selftests/livepatch/ would copy it many times, amend the > copies (meaning parameters would be filled in with sed or the code would > be changed), compile them and load them. But this sounds horrible to me, > especially when compared to Marcos' approach in this patch set which is > quite straightforward. > " > Responded to Petr's message - "Yes, kABI is not backward compatible. But building the tests modules out-of-tree way would allow to build test modules with different kABI from the same sources." This is a solid reason for livepatch modules to live under sefltests. Let's capture this in README and the other updates that need to be made to it in v3. thanks, -- Shuah
On 12/2/22 3:03 PM, Shuah Khan wrote: > On 12/2/22 02:25, Petr Mladek wrote: >> >> Yes, kABI is not backward compatible. But building the tests >> modules out-of-tree way would allow to build test modules with >> different kABI from the same sources. >> > > Okay. This is a solid reason for livepatch modules to live under > sefltests. Let's capture this in README and the other updates that > need to be made to it in v3. > One additional benefit, however small, is that I think everyone is building production livepatches, source based or via kpatch-build, as out-of-tree modules. (Miroslav/Petr/Marcos please correct me if I'm wrong about source based.) Having the livepatch selftests live under selftests/ would imply that new subsystem (build) features would have to support the production build use case from the get go. Regards,
On Fri, Dec 02, 2022 at 04:05:25PM -0500, Joe Lawrence wrote: > On 12/2/22 3:03 PM, Shuah Khan wrote: > > On 12/2/22 02:25, Petr Mladek wrote: > >> > >> Yes, kABI is not backward compatible. But building the tests > >> modules out-of-tree way would allow to build test modules with > >> different kABI from the same sources. > >> > > > > Okay. This is a solid reason for livepatch modules to live under > > sefltests. Let's capture this in README and the other updates that > > need to be made to it in v3. > > > > One additional benefit, however small, is that I think everyone is > building production livepatches, source based or via kpatch-build, as > out-of-tree modules. (Miroslav/Petr/Marcos please correct me if I'm > wrong about source based.) You're correct. We construct our livepatches source based and build them as out-of-tree modules. > > Having the livepatch selftests live under selftests/ would imply that > new subsystem (build) features would have to support the production > build use case from the get go. Agreed. This change makes the modules to be built as real world livepatches. > > Regards, > > -- > Joe >
On Fri, Dec 02, 2022 at 01:03:25PM -0700, Shuah Khan wrote: > On 12/2/22 02:25, Petr Mladek wrote: > > On Thu 2022-12-01 16:58:38, Shuah Khan wrote: > > > On 11/30/22 15:22, Joe Lawrence wrote: > > > > On 7/15/22 10:45 AM, Petr Mladek wrote: > > > > > On Fri 2022-07-01 16:13:50, Shuah Khan wrote: > > > > > > On 7/1/22 1:48 AM, Miroslav Benes wrote: > > > > > > > On Thu, 30 Jun 2022, Shuah Khan wrote: > > > > > > > > > > > > > > > > Sorry Nack on this. Let's not add modules under selftests. Any usage of > > > > > > > > module_init() > > > > > > > > doesn't belong under selftests. > > > > > > > > > > > > Yes I did and after reviewing and thinking about it some more, I decided this > > > > > > is the right direction go down on. > > > > > > > > > > Do you have some particular reason why building modules in selftests > > > > > directory might cause problems, please? > > > > > > > > > > > My reasons are that with this change module_init() propagates out of > > > strictly kernel space and now is in selftests which are user-space. > > > Any changes to this interface will be tied to user-space change. > > > > I am sorry but I do not understand the meaning here. module_init() is > > called when module is loaded. It is not called in userspace. > > > > Maybe, you mean that modules under lib/ are clearly in-tree > > modules. If we move then under tools/ then they will be build > > like out-of-tree modules. Except that they will be maintained in-tree > > so that it will be easy to keep them in sync. > > > > Yes. That is what I mean. > > > And I am sure that they will be actively maintained. The fixes are > > there to make sure that livepatching still works as expected. > > They must pass when any change is done in the livepatch subsystem. > > And they must pass when any kernel is released. > > > > In other words, livepatch and kernel are revision matched. > > > The only concern might be how build failure is handled. IMHO, we > > need to handle it the same way and test failure. > > > > > > Yes. This is another concern - build failures. Let's experiment with > modules under selftests and see if this becomes a problem. > > > > In general, newer tests offer the best coverage, hence the recommendation > > > to run newer tests on older kernels assuming that the tests are built > > > on a newer kernel and backwards should run in a backwards compatible > > > way on older kernels. > > > > This works for the userspace interface that should always be backward > > compatible. But it does not work for kABI. > > > > This is broader than revision matching. Tests should gracefully exit > with skip when a config option they depend on is disabled. The same > gets extended to older kernel versions. Agreed. I'll change functions.sh to do better checking when compiling modules. > > > > > > Do you have a requirement that livepatch test has to be revision > > > matched with the kernel? Even if that is the case, there is no real > > > reason to move modules under selftests other than keeping them in > > > one location. > > > > Yes, kABI is not backward compatible. But building the tests > > modules out-of-tree way would allow to build test modules with > > different kABI from the same sources. > > > > Okay. This is a solid reason for livepatch modules to live under > sefltests. Let's capture this in README and the other updates that > need to be made to it in v3. I'm happy that we reached to a point where we agreed to have the modules moved into kselftests, as the move will allow us to implement more tests that are currently downstream. The v3 is being prepared now. I hope to send it soon. Thanks, Marcos > > thanks, > -- Shuah
Hi there, this is the v2 of the patchset. The v1 can be found at [1]. There is only one change in patch 1, which changed the target directory to build the test modules. All other changes happen in patch 2. Thanks for reviewing! Changes from v1: # test_modules/Makefile * Build the test modules targeting /lib/modules, instead of ksrc when building from the kernel source. # test_modules/test_klp_syscall.c * Added a parameter array to receive the pids that should transition to the new system call. (suggedted by Joe) * Create a new sysfs file /sys/kernel/test_klp_syscall/npids to show how many pids from the argument need to transition to the new state. (suggested by Joe) * Fix the PPC32 support by adding the syscall wrapper for archs that select it by default, without erroring out. PPC does not set SYSCALL_WRAPPER, so having it set in v1 was a mistake. (suggested by Joe) * The aarch64 syscall prefix was added too, since the livepatch support will come soon. # test_binaries/test_klp-call_getpid.c * Change %d/%u in printf (suggested byu Joe) * Change run -> stop variable name, and inverted the assignments (suggested by * Joe). # File test-syscall.sh * Fixed test-syscall.sh to call test_klp-call-getpid in test_binaries dir * Load test_klp_syscall passed the pids of the test_klp-call_getpid instances. Check the sysfs file from test_klp_syscall module to check that all pids transitioned correctly. (suggested by Joe) * Simplified the loop that calls test_klp-call_getpid. (suggested by Joe) * Removed the "success" comment from the script, as it's implicit that it succeed. Otherwise load_lp would error out. (suggested by Joe) * Changed the commit message of patch 2 to further detail what means "tricky" when livepatching syscalls. (suggested by Joe) [1]: 20220603143242.870-1-mpdesouza@suse.com Marcos Paulo de Souza (2): livepatch: Move tests from lib/livepatch to selftests/livepatch selftests: livepatch: Test livepatching a heavily called syscall arch/s390/configs/debug_defconfig | 1 - arch/s390/configs/defconfig | 1 - lib/Kconfig.debug | 22 --- lib/Makefile | 2 - lib/livepatch/Makefile | 14 -- tools/testing/selftests/livepatch/Makefile | 35 +++- tools/testing/selftests/livepatch/README | 5 +- tools/testing/selftests/livepatch/config | 1 - .../testing/selftests/livepatch/functions.sh | 34 ++-- .../selftests/livepatch/test-callbacks.sh | 50 +++--- .../selftests/livepatch/test-ftrace.sh | 6 +- .../selftests/livepatch/test-livepatch.sh | 10 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +- .../testing/selftests/livepatch/test-state.sh | 18 +-- .../selftests/livepatch/test-syscall.sh | 52 ++++++ .../test_binaries/test_klp-call_getpid.c | 48 ++++++ .../selftests/livepatch/test_modules/Makefile | 20 +++ .../test_modules}/test_klp_atomic_replace.c | 0 .../test_modules}/test_klp_callbacks_busy.c | 0 .../test_modules}/test_klp_callbacks_demo.c | 0 .../test_modules}/test_klp_callbacks_demo2.c | 0 .../test_modules}/test_klp_callbacks_mod.c | 0 .../test_modules}/test_klp_livepatch.c | 0 .../test_modules}/test_klp_shadow_vars.c | 0 .../livepatch/test_modules}/test_klp_state.c | 0 .../livepatch/test_modules}/test_klp_state2.c | 0 .../livepatch/test_modules}/test_klp_state3.c | 0 .../livepatch/test_modules/test_klp_syscall.c | 150 ++++++++++++++++++ 28 files changed, 360 insertions(+), 111 deletions(-) delete mode 100644 lib/livepatch/Makefile create mode 100755 tools/testing/selftests/livepatch/test-syscall.sh create mode 100644 tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c create mode 100644 tools/testing/selftests/livepatch/test_modules/Makefile rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_atomic_replace.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_callbacks_busy.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_callbacks_demo.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_callbacks_demo2.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_callbacks_mod.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_livepatch.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_shadow_vars.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_state.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_state2.c (100%) rename {lib/livepatch => tools/testing/selftests/livepatch/test_modules}/test_klp_state3.c (100%) create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c