* [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
@ 2022-11-25 10:52 David Hildenbrand
2022-11-25 11:12 ` Petr Vorel
2022-11-25 11:20 ` Martin Doucha
0 siblings, 2 replies; 6+ messages in thread
From: David Hildenbrand @ 2022-11-25 10:52 UTC (permalink / raw)
To: ltp; +Cc: David Hildenbrand
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 */
--
2.38.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
2022-11-25 10:52 [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent David Hildenbrand
@ 2022-11-25 11:12 ` Petr Vorel
2022-11-25 11:16 ` David Hildenbrand
2022-11-25 11:20 ` Martin Doucha
1 sibling, 1 reply; 6+ messages in thread
From: Petr Vorel @ 2022-11-25 11:12 UTC (permalink / raw)
To: David Hildenbrand; +Cc: ltp
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 */
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
2022-11-25 11:12 ` Petr Vorel
@ 2022-11-25 11:16 ` David Hildenbrand
2022-11-25 12:34 ` Petr Vorel
0 siblings, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2022-11-25 11:16 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp
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.
--
Thanks,
David / dhildenb
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
2022-11-25 11:16 ` David Hildenbrand
@ 2022-11-25 12:34 ` Petr Vorel
0 siblings, 0 replies; 6+ messages in thread
From: Petr Vorel @ 2022-11-25 12:34 UTC (permalink / raw)
To: David Hildenbrand; +Cc: ltp
> 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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
2022-11-25 10:52 [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent David Hildenbrand
2022-11-25 11:12 ` Petr Vorel
@ 2022-11-25 11:20 ` Martin Doucha
2022-11-25 12:19 ` David Hildenbrand
1 sibling, 1 reply; 6+ messages in thread
From: Martin Doucha @ 2022-11-25 11:20 UTC (permalink / raw)
To: David Hildenbrand, ltp
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.
--
Martin Doucha mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent
2022-11-25 11:20 ` Martin Doucha
@ 2022-11-25 12:19 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2022-11-25 12:19 UTC (permalink / raw)
To: Martin Doucha, ltp
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
--
Thanks,
David / dhildenb
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-25 12:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-25 10:52 [LTP] [PATCH v1] security/dirtyc0w_shmem: Fix remaining cases where UFFD_FEATURE_MINOR_SHMEM is absent David Hildenbrand
2022-11-25 11:12 ` Petr Vorel
2022-11-25 11:16 ` David Hildenbrand
2022-11-25 12:34 ` Petr Vorel
2022-11-25 11:20 ` Martin Doucha
2022-11-25 12:19 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.