| Message ID | 20221125105201.59048-1-david@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent | expand |
Hi David, > When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the > checkpoint happy, otherwise our parent process will run into a timeout. > Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by > the UFFD_API ioctl: if the kernel knows about the feature but doesn't > support it, it will be masked off. > Reported-by: Martin Doucha <mdoucha@suse.cz> > Cc: Petr Vorel <pvorel@suse.cz> > Cc: Cyril Hrubis <chrubis@suse.cz> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > index cb2e9df0c..c117c6f39 100644 > --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > @@ -24,12 +24,12 @@ > #include <linux/userfaultfd.h> > #endif > -#ifdef UFFD_FEATURE_MINOR_SHMEM Shouldn't be the check and TST_TEST_TCONF() actually be in dirtyc0w_shmem.c? I overlooked that, but IMHO test does not make sense at all if UFFD_FEATURE_MINOR_SHMEM not defined, right? Also Martin noted that ("The parent process should not even fork() when UFFD_FEATURE_MINOR_SHMEM is not defined in config.h."). Therefore one fix would be to move the check to parent and second (maybe better in separate commit) to add check for uffdio_api.features. Kind regards, Petr > - > #define TST_NO_DEFAULT_MAIN > #include "tst_test.h" > #include "tst_safe_macros.h" > #include "tst_safe_pthread.h" > + > +#ifdef UFFD_FEATURE_MINOR_SHMEM > #include "lapi/syscalls.h" > #define TMP_DIR "tmp_dirtyc0w_shmem" > @@ -162,6 +162,10 @@ retry: > "Could not create userfault file descriptor"); > } > + if (!(uffdio_api.features & UFFD_FEATURE_MINOR_SHMEM)) > + tst_brk(TCONF, > + "System does not have userfaultfd minor fault support for shmem"); > + > uffdio_register.range.start = (unsigned long) map; > uffdio_register.range.len = page_size; > uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR; > @@ -236,6 +240,10 @@ int main(void) > return 0; > } > #else /* UFFD_FEATURE_MINOR_SHMEM */ > -#include "tst_test.h" > -TST_TEST_TCONF("System does not have userfaultfd minor fault support for shmem"); > +int main(void) > +{ > + tst_reinit(); > + TST_CHECKPOINT_WAKE(0); > + tst_brk(TCONF, "System does not have userfaultfd minor fault support for shmem"); > +} > #endif /* UFFD_FEATURE_MINOR_SHMEM */
On 25.11.22 12:12, Petr Vorel wrote: > Hi David, > >> When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the >> checkpoint happy, otherwise our parent process will run into a timeout. >> Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by >> the UFFD_API ioctl: if the kernel knows about the feature but doesn't >> support it, it will be masked off. > >> Reported-by: Martin Doucha <mdoucha@suse.cz> >> Cc: Petr Vorel <pvorel@suse.cz> >> Cc: Cyril Hrubis <chrubis@suse.cz> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) > >> diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> index cb2e9df0c..c117c6f39 100644 >> --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> @@ -24,12 +24,12 @@ >> #include <linux/userfaultfd.h> >> #endif > >> -#ifdef UFFD_FEATURE_MINOR_SHMEM > Shouldn't be the check and TST_TEST_TCONF() actually be in dirtyc0w_shmem.c? > I overlooked that, but IMHO test does not make sense at all if > UFFD_FEATURE_MINOR_SHMEM not defined, right? > > Also Martin noted that ("The parent process should not even fork() when > UFFD_FEATURE_MINOR_SHMEM is not defined in config.h."). > I tried that first, but then we can still run into the runtime absence of UFFD_FEATURE_MINOR_SHMEM. Checking that also in the parent resulted in some IMHO unpleasant code while I worked on that. This is certainly the easiest approach, because we still have to make the child program compile either way. Anyhow, I'll do whatever you decide, because I want to cross this off my list. So any guidance on how to complete this would be appreciated.
Hi, On 25. 11. 22 11:52, David Hildenbrand wrote: > When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the > checkpoint happy, otherwise our parent process will run into a timeout. > Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by > the UFFD_API ioctl: if the kernel knows about the feature but doesn't > support it, it will be masked off. > > Reported-by: Martin Doucha <mdoucha@suse.cz> > Cc: Petr Vorel <pvorel@suse.cz> > Cc: Cyril Hrubis <chrubis@suse.cz> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > index cb2e9df0c..c117c6f39 100644 > --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > @@ -24,12 +24,12 @@ > #include <linux/userfaultfd.h> > #endif > > -#ifdef UFFD_FEATURE_MINOR_SHMEM > - > #define TST_NO_DEFAULT_MAIN > #include "tst_test.h" > #include "tst_safe_macros.h" > #include "tst_safe_pthread.h" > + > +#ifdef UFFD_FEATURE_MINOR_SHMEM > #include "lapi/syscalls.h" > > #define TMP_DIR "tmp_dirtyc0w_shmem" > @@ -162,6 +162,10 @@ retry: > "Could not create userfault file descriptor"); > } > > + if (!(uffdio_api.features & UFFD_FEATURE_MINOR_SHMEM)) > + tst_brk(TCONF, > + "System does not have userfaultfd minor fault support for shmem"); > + > uffdio_register.range.start = (unsigned long) map; > uffdio_register.range.len = page_size; > uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR; > @@ -236,6 +240,10 @@ int main(void) > return 0; > } > #else /* UFFD_FEATURE_MINOR_SHMEM */ > -#include "tst_test.h" > -TST_TEST_TCONF("System does not have userfaultfd minor fault support for shmem"); > +int main(void) > +{ > + tst_reinit(); > + TST_CHECKPOINT_WAKE(0); > + tst_brk(TCONF, "System does not have userfaultfd minor fault support for shmem"); > +} > #endif /* UFFD_FEATURE_MINOR_SHMEM */ This would work as a workaround but I'd recommend adding the necessary structures and constants to include/lapi/userfaultfd.h instead. Then you won't need this conditional compilation at all. I also recommend looking at the fuzzy sync library we use for race conditions: https://github.com/linux-test-project/ltp/wiki/C-Test-API#133-reproducing-race-conditions The original dirtyc0w test was written before this library but using it generally makes race condition reproducers faster, simpler and more reliable.
On 25.11.22 12:20, Martin Doucha wrote: > Hi, > > On 25. 11. 22 11:52, David Hildenbrand wrote: >> When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the >> checkpoint happy, otherwise our parent process will run into a timeout. >> Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by >> the UFFD_API ioctl: if the kernel knows about the feature but doesn't >> support it, it will be masked off. >> >> Reported-by: Martin Doucha <mdoucha@suse.cz> >> Cc: Petr Vorel <pvorel@suse.cz> >> Cc: Cyril Hrubis <chrubis@suse.cz> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> index cb2e9df0c..c117c6f39 100644 >> --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c >> @@ -24,12 +24,12 @@ >> #include <linux/userfaultfd.h> >> #endif >> >> -#ifdef UFFD_FEATURE_MINOR_SHMEM >> - >> #define TST_NO_DEFAULT_MAIN >> #include "tst_test.h" >> #include "tst_safe_macros.h" >> #include "tst_safe_pthread.h" >> + >> +#ifdef UFFD_FEATURE_MINOR_SHMEM >> #include "lapi/syscalls.h" >> >> #define TMP_DIR "tmp_dirtyc0w_shmem" >> @@ -162,6 +162,10 @@ retry: >> "Could not create userfault file descriptor"); >> } >> >> + if (!(uffdio_api.features & UFFD_FEATURE_MINOR_SHMEM)) >> + tst_brk(TCONF, >> + "System does not have userfaultfd minor fault support for shmem"); >> + >> uffdio_register.range.start = (unsigned long) map; >> uffdio_register.range.len = page_size; >> uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR; >> @@ -236,6 +240,10 @@ int main(void) >> return 0; >> } >> #else /* UFFD_FEATURE_MINOR_SHMEM */ >> -#include "tst_test.h" >> -TST_TEST_TCONF("System does not have userfaultfd minor fault support for shmem"); >> +int main(void) >> +{ >> + tst_reinit(); >> + TST_CHECKPOINT_WAKE(0); >> + tst_brk(TCONF, "System does not have userfaultfd minor fault support for shmem"); >> +} >> #endif /* UFFD_FEATURE_MINOR_SHMEM */ > > This would work as a workaround but I'd recommend adding the necessary > structures and constants to include/lapi/userfaultfd.h instead. Then you > won't need this conditional compilation at all. That seems to work as well, although I'm still a bit unsure what to include in that file, what not, what to strip, ... I'll use include/lapi/io_uring.h as an orientation but will most probably mess it up on the first attempt. > > I also recommend looking at the fuzzy sync library we use for race > conditions: > https://github.com/linux-test-project/ltp/wiki/C-Test-API#133-reproducing-race-conditions > I'll look into that as well. > The original dirtyc0w test was written before this library but using it > generally makes race condition reproducers faster, simpler and more > reliable. Thanks
> On 25.11.22 12:12, Petr Vorel wrote: > > Hi David, > > > When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the > > > checkpoint happy, otherwise our parent process will run into a timeout. > > > Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by > > > the UFFD_API ioctl: if the kernel knows about the feature but doesn't > > > support it, it will be masked off. > > > Reported-by: Martin Doucha <mdoucha@suse.cz> > > > Cc: Petr Vorel <pvorel@suse.cz> > > > Cc: Cyril Hrubis <chrubis@suse.cz> > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > --- > > > .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > > > index cb2e9df0c..c117c6f39 100644 > > > --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > > > +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c > > > @@ -24,12 +24,12 @@ > > > #include <linux/userfaultfd.h> > > > #endif > > > -#ifdef UFFD_FEATURE_MINOR_SHMEM > > Shouldn't be the check and TST_TEST_TCONF() actually be in dirtyc0w_shmem.c? > > I overlooked that, but IMHO test does not make sense at all if > > UFFD_FEATURE_MINOR_SHMEM not defined, right? > > Also Martin noted that ("The parent process should not even fork() when > > UFFD_FEATURE_MINOR_SHMEM is not defined in config.h."). > I tried that first, but then we can still run into the runtime absence of > UFFD_FEATURE_MINOR_SHMEM. Checking that also in the parent resulted in some > IMHO unpleasant code while I worked on that. > This is certainly the easiest approach, because we still have to make the > child program compile either way. Right, it needs to be in child. Using TST_TEST_TCONF() also in master does not look to me as too unpleasant code, but take it just a suggestion. Obviously the only requirement is code compiles and works on both defined and undefined UFFD_FEATURE_MINOR_SHMEM. > Anyhow, I'll do whatever you decide, because I want to cross this off my > list. So any guidance on how to complete this would be appreciated. Understand. Kind regards, Petr
diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c index cb2e9df0c..c117c6f39 100644 --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem_child.c @@ -24,12 +24,12 @@ #include <linux/userfaultfd.h> #endif -#ifdef UFFD_FEATURE_MINOR_SHMEM - #define TST_NO_DEFAULT_MAIN #include "tst_test.h" #include "tst_safe_macros.h" #include "tst_safe_pthread.h" + +#ifdef UFFD_FEATURE_MINOR_SHMEM #include "lapi/syscalls.h" #define TMP_DIR "tmp_dirtyc0w_shmem" @@ -162,6 +162,10 @@ retry: "Could not create userfault file descriptor"); } + if (!(uffdio_api.features & UFFD_FEATURE_MINOR_SHMEM)) + tst_brk(TCONF, + "System does not have userfaultfd minor fault support for shmem"); + uffdio_register.range.start = (unsigned long) map; uffdio_register.range.len = page_size; uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR; @@ -236,6 +240,10 @@ int main(void) return 0; } #else /* UFFD_FEATURE_MINOR_SHMEM */ -#include "tst_test.h" -TST_TEST_TCONF("System does not have userfaultfd minor fault support for shmem"); +int main(void) +{ + tst_reinit(); + TST_CHECKPOINT_WAKE(0); + tst_brk(TCONF, "System does not have userfaultfd minor fault support for shmem"); +} #endif /* UFFD_FEATURE_MINOR_SHMEM */
When UFFD_FEATURE_MINOR_SHMEM is not defined, we still have to make the checkpoint happy, otherwise our parent process will run into a timeout. Further, we have to test if UFFD_FEATURE_MINOR_SHMEM is really returned by the UFFD_API ioctl: if the kernel knows about the feature but doesn't support it, it will be masked off. Reported-by: Martin Doucha <mdoucha@suse.cz> Cc: Petr Vorel <pvorel@suse.cz> Cc: Cyril Hrubis <chrubis@suse.cz> Signed-off-by: David Hildenbrand <david@redhat.com> --- .../dirtyc0w_shmem/dirtyc0w_shmem_child.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)