| Message ID | 20240325134114.257544-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/secretmem: one fix and one refactoring | expand |
Hi David, On Mon, Mar 25, 2024 at 02:41:13PM +0100, David Hildenbrand wrote: > Let's add a simple reproducer for a scneario where GUP-fast could succeed > on secretmem folios, making vmsplice() succeed instead of failing. The > reproducer is based on a reproducer [1] by Miklos Szeredi. > > Perform the ftruncate() only once, and check the return value. > > For some reason, vmsplice() reliably fails (making the test succeed) when > we move the test_vmsplice() call after test_process_vm_read() / > test_ptrace(). That's because ftruncate() call was in test_remote_access() and you need it to mmap secretmem. > Properly cleaning up in test_remote_access(), which is not > part of this change, won't change that behavior. Therefore, run the > vmsplice() test for now first -- something is a bit off once we involve > fork(). > > [1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- > 1 file changed, 42 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c > index 9b298f6a04b3..0acbdcf8230e 100644 > --- a/tools/testing/selftests/mm/memfd_secret.c > +++ b/tools/testing/selftests/mm/memfd_secret.c > @@ -20,6 +20,7 @@ > #include <unistd.h> > #include <errno.h> > #include <stdio.h> > +#include <fcntl.h> > > #include "../kselftest.h" > > @@ -83,6 +84,43 @@ static void test_mlock_limit(int fd) > pass("mlock limit is respected\n"); > } > > +static void test_vmsplice(int fd) > +{ > + ssize_t transferred; > + struct iovec iov; > + int pipefd[2]; > + char *mem; > + > + if (pipe(pipefd)) { > + fail("pipe failed: %s\n", strerror(errno)); > + return; > + } > + > + mem = mmap(NULL, page_size, prot, mode, fd, 0); > + if (mem == MAP_FAILED) { > + fail("Unable to mmap secret memory\n"); > + goto close_pipe; > + } > + > + /* > + * vmsplice() may use GUP-fast, which must also fail. Prefault the > + * page table, so GUP-fast could find it. > + */ > + memset(mem, PATTERN, page_size); > + > + iov.iov_base = mem; > + iov.iov_len = page_size; > + transferred = vmsplice(pipefd[1], &iov, 1, 0); > + > + ksft_test_result(transferred < 0 && errno == EFAULT, > + "vmsplice is blocked as expected\n"); The same message will be printed on success and on failure. I think if (transferred < 0 && errno == EFAULT) pass("vmsplice is blocked as expected"); else fail("vmsplice: unexpected memory acccess"); is clearer than feeding different strings to ksft_test_result(). Other than that Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org> > + > + munmap(mem, page_size); > +close_pipe: > + close(pipefd[0]); > + close(pipefd[1]); > +}
On 26.03.24 07:17, Mike Rapoport wrote: > Hi David, > > On Mon, Mar 25, 2024 at 02:41:13PM +0100, David Hildenbrand wrote: >> Let's add a simple reproducer for a scneario where GUP-fast could succeed >> on secretmem folios, making vmsplice() succeed instead of failing. The >> reproducer is based on a reproducer [1] by Miklos Szeredi. >> >> Perform the ftruncate() only once, and check the return value. >> >> For some reason, vmsplice() reliably fails (making the test succeed) when >> we move the test_vmsplice() call after test_process_vm_read() / >> test_ptrace(). > > That's because ftruncate() call was in test_remote_access() and you need it > to mmap secretmem. I don't think that's the reason. I reshuffled the code a couple of times without luck. And in fact, even executing the vmsplice() test twice results in the second iteration succeeding on an old kernel (6.7.4-200.fc39.x86_64). ok 1 mlock limit is respected ok 2 file IO is blocked as expected not ok 3 vmsplice is blocked as expected ok 4 vmsplice is blocked as expected ok 5 process_vm_read is blocked as expected ok 6 ptrace is blocked as expected Note that the mmap()+memset() succeeded. So the secretmem pages should be in the page table. Even weirder, if I simply mmap()+memset()+munmap() secretmem *once*, the test passes diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c index 0acbdcf8230e..7a973ec6ac8f 100644 --- a/tools/testing/selftests/mm/memfd_secret.c +++ b/tools/testing/selftests/mm/memfd_secret.c @@ -96,6 +96,14 @@ static void test_vmsplice(int fd) return; } + mem = mmap(NULL, page_size, prot, mode, fd, 0); + if (mem == MAP_FAILED) { + fail("Unable to mmap secret memory\n"); + goto close_pipe; + } + memset(mem, PATTERN, page_size); + munmap(mem, page_size); + mem = mmap(NULL, page_size, prot, mode, fd, 0); if (mem == MAP_FAILED) { fail("Unable to mmap secret memory\n"); ok 1 mlock limit is respected ok 2 file IO is blocked as expected ok 3 vmsplice is blocked as expected ok 4 process_vm_read is blocked as expected ok 5 ptrace is blocked as expected ... could it be that munmap()+mmap() will end up turning these pages into LRU pages? I am 100% sure that is happening -- likely, because VM_LOCKED is involved, because on the patched kernel, I see the following: ok 1 mlock limit is respected ok 2 file IO is blocked as expected ok 3 vmsplice is blocked as expected not ok 4 vmsplice is blocked as expected ok 5 process_vm_read is blocked as expected ok 6 ptrace is blocked as expected At this point, I think we should remove the LRU test for secretmem. I'll adjust patch #1 and extend this test to cover that case as well. > >> Properly cleaning up in test_remote_access(), which is not >> part of this change, won't change that behavior. Therefore, run the >> vmsplice() test for now first -- something is a bit off once we involve >> fork(). >> >> [1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- >> 1 file changed, 42 insertions(+), 2 deletions(-) >> >> diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c >> index 9b298f6a04b3..0acbdcf8230e 100644 >> --- a/tools/testing/selftests/mm/memfd_secret.c >> +++ b/tools/testing/selftests/mm/memfd_secret.c >> @@ -20,6 +20,7 @@ >> #include <unistd.h> >> #include <errno.h> >> #include <stdio.h> >> +#include <fcntl.h> >> >> #include "../kselftest.h" >> >> @@ -83,6 +84,43 @@ static void test_mlock_limit(int fd) >> pass("mlock limit is respected\n"); >> } >> >> +static void test_vmsplice(int fd) >> +{ >> + ssize_t transferred; >> + struct iovec iov; >> + int pipefd[2]; >> + char *mem; >> + >> + if (pipe(pipefd)) { >> + fail("pipe failed: %s\n", strerror(errno)); >> + return; >> + } >> + >> + mem = mmap(NULL, page_size, prot, mode, fd, 0); >> + if (mem == MAP_FAILED) { >> + fail("Unable to mmap secret memory\n"); >> + goto close_pipe; >> + } >> + >> + /* >> + * vmsplice() may use GUP-fast, which must also fail. Prefault the >> + * page table, so GUP-fast could find it. >> + */ >> + memset(mem, PATTERN, page_size); >> + >> + iov.iov_base = mem; >> + iov.iov_len = page_size; >> + transferred = vmsplice(pipefd[1], &iov, 1, 0); >> + >> + ksft_test_result(transferred < 0 && errno == EFAULT, >> + "vmsplice is blocked as expected\n"); > > The same message will be printed on success and on failure. > > I think > > if (transferred < 0 && errno == EFAULT) > pass("vmsplice is blocked as expected"); > else > fail("vmsplice: unexpected memory acccess"); > > is clearer than feeding different strings to ksft_test_result(). > Can do, thanks!
On 26.03.24 13:32, David Hildenbrand wrote: > On 26.03.24 07:17, Mike Rapoport wrote: >> Hi David, >> >> On Mon, Mar 25, 2024 at 02:41:13PM +0100, David Hildenbrand wrote: >>> Let's add a simple reproducer for a scneario where GUP-fast could succeed >>> on secretmem folios, making vmsplice() succeed instead of failing. The >>> reproducer is based on a reproducer [1] by Miklos Szeredi. >>> >>> Perform the ftruncate() only once, and check the return value. >>> >>> For some reason, vmsplice() reliably fails (making the test succeed) when >>> we move the test_vmsplice() call after test_process_vm_read() / >>> test_ptrace(). >> >> That's because ftruncate() call was in test_remote_access() and you need it >> to mmap secretmem. > > I don't think that's the reason. I reshuffled the code a couple of times > without luck. > > And in fact, even executing the vmsplice() test twice results in the > second iteration succeeding on an old kernel (6.7.4-200.fc39.x86_64). > > ok 1 mlock limit is respected > ok 2 file IO is blocked as expected > not ok 3 vmsplice is blocked as expected > ok 4 vmsplice is blocked as expected > ok 5 process_vm_read is blocked as expected > ok 6 ptrace is blocked as expected > > Note that the mmap()+memset() succeeded. So the secretmem pages should be in the page table. > > > Even weirder, if I simply mmap()+memset()+munmap() secretmem *once*, the test passes > > diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c > index 0acbdcf8230e..7a973ec6ac8f 100644 > --- a/tools/testing/selftests/mm/memfd_secret.c > +++ b/tools/testing/selftests/mm/memfd_secret.c > @@ -96,6 +96,14 @@ static void test_vmsplice(int fd) > return; > } > > + mem = mmap(NULL, page_size, prot, mode, fd, 0); > + if (mem == MAP_FAILED) { > + fail("Unable to mmap secret memory\n"); > + goto close_pipe; > + } > + memset(mem, PATTERN, page_size); > + munmap(mem, page_size); > + > mem = mmap(NULL, page_size, prot, mode, fd, 0); > if (mem == MAP_FAILED) { > fail("Unable to mmap secret memory\n"); > > ok 1 mlock limit is respected > ok 2 file IO is blocked as expected > ok 3 vmsplice is blocked as expected > ok 4 process_vm_read is blocked as expected > ok 5 ptrace is blocked as expected > > > ... could it be that munmap()+mmap() will end up turning these pages into LRU pages? Okay, now I am completely confused. secretmem_fault() calls filemap_add_folio(), which should turn this into an LRU page. So secretmem pages should always be LRU pages. .. unless we're batching in the LRU cache and haven't done the lru_add_drain() ... And likely, the munmap() will drain the lru cache and turn the page into an LRU page. Okay, I'll go make sure if that's the case. If so, relying on the page being LRU vs. not LRU in GUP-fast is unreliable and shall be dropped.
diff --git a/tools/testing/selftests/mm/memfd_secret.c b/tools/testing/selftests/mm/memfd_secret.c index 9b298f6a04b3..0acbdcf8230e 100644 --- a/tools/testing/selftests/mm/memfd_secret.c +++ b/tools/testing/selftests/mm/memfd_secret.c @@ -20,6 +20,7 @@ #include <unistd.h> #include <errno.h> #include <stdio.h> +#include <fcntl.h> #include "../kselftest.h" @@ -83,6 +84,43 @@ static void test_mlock_limit(int fd) pass("mlock limit is respected\n"); } +static void test_vmsplice(int fd) +{ + ssize_t transferred; + struct iovec iov; + int pipefd[2]; + char *mem; + + if (pipe(pipefd)) { + fail("pipe failed: %s\n", strerror(errno)); + return; + } + + mem = mmap(NULL, page_size, prot, mode, fd, 0); + if (mem == MAP_FAILED) { + fail("Unable to mmap secret memory\n"); + goto close_pipe; + } + + /* + * vmsplice() may use GUP-fast, which must also fail. Prefault the + * page table, so GUP-fast could find it. + */ + memset(mem, PATTERN, page_size); + + iov.iov_base = mem; + iov.iov_len = page_size; + transferred = vmsplice(pipefd[1], &iov, 1, 0); + + ksft_test_result(transferred < 0 && errno == EFAULT, + "vmsplice is blocked as expected\n"); + + munmap(mem, page_size); +close_pipe: + close(pipefd[0]); + close(pipefd[1]); +} + static void try_process_vm_read(int fd, int pipefd[2]) { struct iovec liov, riov; @@ -187,7 +225,6 @@ static void test_remote_access(int fd, const char *name, return; } - ftruncate(fd, page_size); memset(mem, PATTERN, page_size); if (write(pipefd[1], &mem, sizeof(mem)) < 0) { @@ -258,7 +295,7 @@ static void prepare(void) strerror(errno)); } -#define NUM_TESTS 4 +#define NUM_TESTS 5 int main(int argc, char *argv[]) { @@ -277,9 +314,12 @@ int main(int argc, char *argv[]) ksft_exit_fail_msg("memfd_secret failed: %s\n", strerror(errno)); } + if (ftruncate(fd, page_size)) + ksft_exit_fail_msg("ftruncate failed: %s\n", strerror(errno)); test_mlock_limit(fd); test_file_apis(fd); + test_vmsplice(fd); test_process_vm_read(fd); test_ptrace(fd);
Let's add a simple reproducer for a scneario where GUP-fast could succeed on secretmem folios, making vmsplice() succeed instead of failing. The reproducer is based on a reproducer [1] by Miklos Szeredi. Perform the ftruncate() only once, and check the return value. For some reason, vmsplice() reliably fails (making the test succeed) when we move the test_vmsplice() call after test_process_vm_read() / test_ptrace(). Properly cleaning up in test_remote_access(), which is not part of this change, won't change that behavior. Therefore, run the vmsplice() test for now first -- something is a bit off once we involve fork(). [1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com Signed-off-by: David Hildenbrand <david@redhat.com> --- tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)