mm/secretmem: one fix and one refactoring
Subsystems (6)
cover mm/secretmem: one fix and one refactoring
Patch #1 fixes a GUP-fast issue, whereby we might succeed in pinning secretmem folios. Patch #2 extends the memfd_secret selftest to cover that case. Patch #3 removes folio_is_secretmem() and instead lets folio_fast_pin_allowed() cover that case as well.
With this series, the reproducer (+selftests) works as expected. To test patch #3, the gup_longterm test does exactly what we need, and keeps on working as expected.
David Hildenbrand (3):
mm/secretmem: fix GUP-fast succeeding on secretmem folios selftests/memfd_secret: add vmsplice() test
mm: merge folio_is_secretmem() into folio_fast_pin_allowed()
include/linux/secretmem.h | 21 ++--------- mm/gup.c | 33 ++++++++++------- tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- 3 files changed, 65 insertions(+), 33 deletions(-)
Signature · 1 line
2.43.2
PATCH 1/3 mm/secretmem: fix GUP-fast succeeding on secretmem folios
folio_is_secretmem() states that secretmem folios cannot be LRU folios:
so we may only exit early if we find an LRU folio. Yet, we exit early if we find a folio that is not a secretmem folio.
Consequently, folio_is_secretmem() fails to detect secretmem folios and, therefore, we can succeed in grabbing a secretmem folio during GUP-fast, crashing the kernel when we later try reading/writing to the folio, because the folio has been unmapped from the directmap.
--- include/linux/secretmem.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/secretmem.h +1 -1
@@ -16,7 +16,7 @@ static inline bool folio_is_secretmem(struct folio *folio)* We know that secretmem pages are not compound and LRU so we can * save a couple of cycles here. */ -if (folio_test_large (folio) || !folio_test_lru (folio)) +if (folio_test_large (folio) ||folio_test_lru (folio))return false; mapping = (struct address_space *)
Signature · 1 line
2.43.2
Andrew Morton
maintainerRe: [PATCH v1 1/3] mm/secretmem: fix GUP-fast succeeding on secretmem folios
Quoted 7 lines · On Mon, 25 Mar 2024 14:41:12 +0100 David Hildenbrand <[email protected]> wrote:
folio_is_secretmem()states that secretmem folios cannot be LRU folios:
so we may only exit early if we find an LRU folio. Yet, we exit early if we find a folio that is not a secretmem folio....
Cc <[email protected]>
Thanks, I split up this series. Because this patch goes mm-hotfixes-unstable -> mm-hotfixes-stable -> mainline whereas the other two go mm-unstable -> mm-stable -> mainline at a later time.
Re: [PATCH v1 1/3] mm/secretmem: fix GUP-fast succeeding on secretmem folios
Quoted 6 lines · Andrew Morton
Quoted 4 lines · On Mon, 25 Mar 2024 14:41:12 +0100 David Hildenbrand <[email protected]> wrote:
folio_is_secretmem()states that secretmem folios cannot be LRU folios:
so we may only exit early if we find an LRU folio. Yet, we exit early if we find a folio that is not a secretmem folio.
Quoted 7 lines
Quoted 3 lines
...
Cc <[email protected]>Thanks, I split up this series. Because this patch goes mm-hotfixes-unstable -> mm-hotfixes-stable -> mainline whereas the other two go mm-unstable -> mm-stable -> mainline at a later time.
Makes sense. I'll resend a v2 later, because there are some major changes (the fix was wrong/incomplete, we have to remove the LRU test completely).
Signature · 3 lines
Cheers, David / dhildenb
PATCH 2/3 selftests/memfd_secret: add vmsplice() test
In reply to David HildenbrandLet'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
--- tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)
tools/testing/selftests/mm/memfd_secret.c +42 -2
@@ -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 5int 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);
Signature · 1 line
2.43.2
Re: [PATCH v1 2/3] selftests/memfd_secret: add vmsplice() test
Hi David,
Quoted 9 lines · David Hildenbrand
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 thetest_vmsplice()call aftertest_process_vm_read()/test_ptrace().
That's because ftruncate() call was in test_remote_access() and you need it to mmap secretmem.
Quoted 48 lines
Properly cleaning up in
test_remote_access(), which is not part of this change, won't change that behavior. Therefore, run thevmsplice()test for now first -- something is a bit off once we involvefork().[1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com
Signed-off-by David Hildenbrand <[email protected]>--- tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)tools/testing/selftests/mm/memfd_secret.c +21
-0@@ -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
tools/testing/selftests/mm/memfd_secret.c static void test_mlock_limit(int fd) · line 116
Quoted 10 lines
tools/testing/selftests/mm/memfd_secret.c +10
-0+* 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
Quoted 6 lines
tools/testing/selftests/mm/memfd_secret.c +6
-0+ +munmap (mem, page_size); +close_pipe: +close (pipefd[0 ]); +close (pipefd[1 ]); +}
Signature · 2 lines
Sincerely yours, Mike.
Re: [PATCH v1 2/3] selftests/memfd_secret: add vmsplice() test
Quoted 7 lines · Mike Rapoport
Hi David,
Quoted 4 lines · David Hildenbrand
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.
Quoted 8 lines
Quoted 5 lines
Perform the
ftruncate()only once, and check the return value.For some reason,
vmsplice()reliably fails (making the test succeed) when we move thetest_vmsplice()call aftertest_process_vm_read()/test_ptrace().That's because
ftruncate()call was intest_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
tools/testing/selftests/mm/memfd_secret.c +8 -0
@@ -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.
Quoted 62 lines
Quoted 49 lines
Properly cleaning up in
test_remote_access(), which is not part of this change, won't change that behavior. Therefore, run thevmsplice()test for now first -- something is a bit off once we involvefork().[1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com
Signed-off-by David Hildenbrand <[email protected]>--- tools/testing/selftests/mm/memfd_secret.c | 44 +++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)tools/testing/selftests/mm/memfd_secret.c +21
-0@@ -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 tools/testing/selftests/mm/memfd_secret.c static void test_mlock_limit(int fd) · line 116
Quoted 10 lines
tools/testing/selftests/mm/memfd_secret.c +10
-0+* 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.
Quoted 9 lines
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!
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH v1 2/3] selftests/memfd_secret: add vmsplice() test
Quoted 55 lines · David Hildenbrand
Quoted 7 lines · Mike Rapoport
Hi David,
Quoted 4 lines · David Hildenbrand
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.Quoted 8 lines
Quoted 5 lines
Perform the
ftruncate()only once, and check the return value.For some reason,
vmsplice()reliably fails (making the test succeed) when we move thetest_vmsplice()call aftertest_process_vm_read()/test_ptrace().That's because
ftruncate()call was intest_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 5process_vm_readis blocked as expected ok 6 ptrace is blocked as expectedNote 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
tools/testing/selftests/mm/memfd_secret.c +8
-0@@ -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" );
Quoted 8 lines
ok 1 mlock limit is respected
ok 2 file IO is blocked as expected ok 3 vmsplice is blocked as expected ok 4process_vm_readis 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.
Signature · 3 lines
Cheers, David / dhildenb
PATCH 3/3 mm: merge folio_is_secretmem() into folio_fast_pin_allowed()
In reply to David Hildenbrandfolio_is_secretmem() is currently only used during GUP-fast, and using it in wrong context where concurrent truncation might happen, could be problematic.
Nowadays, folio_fast_pin_allowed() performs similar checks during GUP-fast and contains a lot of careful handling -- READ_ONCE( -- ), sanity checks -- lockdep_assert_irqs_disabled() -- and helpful comments on how this handling is safe and correct.
So let's merge folio_is_secretmem() into folio_fast_pin_allowed(), still avoiding checking the actual mapping only if really required.
--- include/linux/secretmem.h | 21 ++------------------- mm/gup.c | 33 +++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 31 deletions(-)
include/linux/secretmem.h +2 -19
@@ -6,25 +6,8 @@extern const struct address_space_operations secretmem_aops; -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) { -struct address_space *mapping; - -/* -* Using folio_mapping() is quite slow because of the actual call -* instruction. -* We know that secretmem pages are not compound and LRU so we can -* save a couple of cycles here. -*/ -if (folio_test_large (folio) ||folio_test_lru (folio)) -return false; - - mapping = (struct address_space *) - ((unsigned long )folio->mapping & ~PAGE_MAPPING_FLAGS ); - -if (!mapping || mapping != folio->mapping) -return false; -return mapping->a_ops == &secretmem_aops; }
@@ -38,7 +21,7 @@ static inline bool vma_is_secretmem(struct vm_area_struct *vma)return false; } -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) {return false; }
mm/gup.c +21 -12
@@ -2472,6 +2472,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked);* This call assumes the caller has pinned the folio, that the lowest page table * level still points to this folio, and that interrupts have been disabled. * +* GUP-fast must reject all secretmem folios. +* * Writing to pinned file-backed dirty tracked folios is inherently problematic * (see comment describing the writable_file_mapping_allowed() function). We * therefore try to avoid the most egregious case of a long-term mapping doing
@@ -2484,22 +2486,32 @@ EXPORT_SYMBOL(get_user_pages_unlocked);static bool folio_fast_pin_allowed (struct folio *folio,unsigned int flags) {struct address_space *mapping; +bool check_secretmem = false; +bool reject_file_backed = false;unsigned long mapping_flags;/* * If we aren't pinning then no problematic write can occur. A long term * pin is the most egregious case so this is the one we disallow. */ -if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) != +if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) == (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) -return true; + reject_file_backed = true; + +/* We hold a folio reference, so we can safely access folio fields. */ -/* The folio is pinned, so we can safely access folio fields. */ +/* secretmem folios are only order-0 folios and never LRU folios. */ +if (IS_ENABLED (CONFIG_SECRETMEM ) && !folio_test_large (folio) && + !folio_test_lru (folio)) + check_secretmem = true; + +if (!reject_file_backed && !check_secretmem) +return true;if (WARN_ON_ONCE (folio_test_slab (folio)))return false; -/* hugetlb mappings do not require dirty-tracking. */ +/* hugetlb neither requires dirty-tracking nor can be secretmem. */ if (folio_test_hugetlb (folio))return true;
@@ -2535,10 +2547,12 @@ static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags)/* * At this point, we know the mapping is non-null and points to an -* address_space object. The only remaining whitelisted file system is -* shmem. +* address_space object. */ -return shmem_mapping (mapping); +if (check_secretmem &&secretmem_mapping (mapping)) +return false; +/* The only remaining allowed file system is shmem. */ +return !reject_file_backed ||shmem_mapping (mapping); } static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start,
@@ -2624,11 +2638,6 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,if (!folio) goto pte_unmap; -if (unlikely (folio_is_secretmem (folio))) { -gup_put_folio (folio,1 , flags); - goto pte_unmap; - } -if (unlikely (pmd_val (pmd) !=pmd_val (*pmdp)) ||unlikely (pte_val (pte) !=pte_val (ptep_get (ptep)))) {gup_put_folio (folio,1 , flags);
Signature · 1 line
2.43.2
Re: [PATCH v1 3/3] mm: merge folio_is_secretmem() into folio_fast_pin_allowed()
Quoted 9 lines · David Hildenbrand
folio_is_secretmem()is currently only used during GUP-fast, and using it in wrong context where concurrent truncation might happen, could be problematic.Nowadays,
folio_fast_pin_allowed()performs similar checks during GUP-fast and contains a lot of careful handling --READ_ONCE( -- ), sanity checks --lockdep_assert_irqs_disabled()-- and helpful comments on how this handling is safe and correct.
Quoted 4 lines
So let's merge
folio_is_secretmem()intofolio_fast_pin_allowed(), still avoiding checking the actual mapping only if really required.Signed-off-by David Hildenbrand <[email protected]>
A few comments below, no strong feelings about them.
Quoted 59 lines
--- include/linux/secretmem.h | 21 ++------------------- mm/gup.c | 33 +++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 31 deletions(-)include/linux/secretmem.h +2
-19@@ -6,25 +6,8 @@extern const struct address_space_operations secretmem_aops; -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) { -struct address_space *mapping; - -/* -* Using folio_mapping() is quite slow because of the actual call -* instruction. -* We know that secretmem pages are not compound and LRU so we can -* save a couple of cycles here. -*/ -if (folio_test_large (folio) ||folio_test_lru (folio)) -return false; - - mapping = (struct address_space *) - ((unsigned long )folio->mapping & ~PAGE_MAPPING_FLAGS ); - -if (!mapping || mapping != folio->mapping) -return false; -return mapping->a_ops == &secretmem_aops; }@@ -38,7 +21,7 @@ static inline bool vma_is_secretmem(struct vm_area_struct *vma)return false; } -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) {return false; }mm/gup.c +2
-0@@ -2472,6 +2472,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked);* This call assumes the caller has pinned the folio, that the lowest page table * level still points to this folio, and that interrupts have been disabled. * +* GUP-fast must reject all secretmem folios. +* * Writing to pinned file-backed dirty tracked folios is inherently problematic * (see comment describing the writable_file_mapping_allowed() function). We * therefore try to avoid the most egregious case of a long-term mapping doing
mm/gup.c EXPORT_SYMBOL(get_user_pages_unlocked); · line 2486
Quoted 1 line
mm/gup.c +0
-0@@ -2484,22 +2486,32 @@ EXPORT_SYMBOL(get_user_pages_unlocked);static bool folio_fast_pin_allowed (struct folio *folio,unsigned int flags)
Now when this function checks for gup in general, maybe it's worth to rename it to, say, folio_fast_gup_allowed.
Quoted 10 lines
mm/gup.c +2
-0{struct address_space *mapping; +bool check_secretmem = false; +bool reject_file_backed = false;unsigned long mapping_flags;/* * If we aren't pinning then no problematic write can occur. A long term * pin is the most egregious case so this is the one we disallow. */
mm/gup.c EXPORT_SYMBOL(get_user_pages_unlocked); · line 2503
Quoted 10 lines
mm/gup.c +5
-3-if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) != +if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) == (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) -return true; + reject_file_backed = true; + +/* We hold a folio reference, so we can safely access folio fields. */ -/* The folio is pinned, so we can safely access folio fields. */ +/* secretmem folios are only order-0 folios and never LRU folios. */
Nit: ^ always
Quoted 46 lines
mm/gup.c +12
-9+if (IS_ENABLED (CONFIG_SECRETMEM ) && !folio_test_large (folio) && + !folio_test_lru (folio)) + check_secretmem = true; + +if (!reject_file_backed && !check_secretmem) +return true;if (WARN_ON_ONCE (folio_test_slab (folio)))return false; -/* hugetlb mappings do not require dirty-tracking. */ +/* hugetlb neither requires dirty-tracking nor can be secretmem. */ if (folio_test_hugetlb (folio))return true;@@ -2535,10 +2547,12 @@ static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags)/* * At this point, we know the mapping is non-null and points to an -* address_space object. The only remaining whitelisted file system is -* shmem. +* address_space object. */ -return shmem_mapping (mapping); +if (check_secretmem &&secretmem_mapping (mapping)) +return false; +/* The only remaining allowed file system is shmem. */ +return !reject_file_backed ||shmem_mapping (mapping); } static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start,@@ -2624,11 +2638,6 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,if (!folio) goto pte_unmap; -if (unlikely (folio_is_secretmem (folio))) { -gup_put_folio (folio,1 , flags); - goto pte_unmap; - } -if (unlikely (pmd_val (pmd) !=pmd_val (*pmdp)) ||unlikely (pte_val (pte) !=pte_val (ptep_get (ptep)))) {gup_put_folio (folio,1 , flags);Signature · 1 line
2.43.2
Signature · 2 lines
Sincerely yours, Mike.
Re: [PATCH v1 3/3] mm: merge folio_is_secretmem() into folio_fast_pin_allowed()
Quoted 7 lines · Mike Rapoport
Quoted 6 lines · David Hildenbrand
folio_is_secretmem()is currently only used during GUP-fast, and using it in wrong context where concurrent truncation might happen, could be problematic.Nowadays,
folio_fast_pin_allowed()performs similar checks during GUP-fast and contains a lot of careful handling --READ_ONCE( -- ), sanity
Re-reading my stuff ...
s/( -- )/() --/
Quoted 5 lines
Quoted 5 lines
checks --
lockdep_assert_irqs_disabled()-- and helpful comments on how this handling is safe and correct.So let's merge
folio_is_secretmem()intofolio_fast_pin_allowed(), still avoiding checking the actual mapping only if really required.
s/avoiding//
Quoted 5 lines
Quoted 2 lines
Signed-off-by David Hildenbrand <[email protected]>Reviewed-by Mike Rapoport (IBM) <[email protected]>
Thanks!
Quoted 57 lines
A few comments below, no strong feelings about them.
Quoted 55 lines
--- include/linux/secretmem.h | 21 ++------------------- mm/gup.c | 33 +++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 31 deletions(-)include/linux/secretmem.h +2
-19@@ -6,25 +6,8 @@extern const struct address_space_operations secretmem_aops; -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) { -struct address_space *mapping; - -/* -* Using folio_mapping() is quite slow because of the actual call -* instruction. -* We know that secretmem pages are not compound and LRU so we can -* save a couple of cycles here. -*/ -if (folio_test_large (folio) ||folio_test_lru (folio)) -return false; - - mapping = (struct address_space *) - ((unsigned long )folio->mapping & ~PAGE_MAPPING_FLAGS ); - -if (!mapping || mapping != folio->mapping) -return false; -return mapping->a_ops == &secretmem_aops; }@@ -38,7 +21,7 @@ static inline bool vma_is_secretmem(struct vm_area_struct *vma)return false; } -static inline bool folio_is_secretmem (struct folio *folio) +static inline bool secretmem_mapping (struct address_space *mapping) {return false; }mm/gup.c +2
-0@@ -2472,6 +2472,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked);* This call assumes the caller has pinned the folio, that the lowest page table * level still points to this folio, and that interrupts have been disabled. * +* GUP-fast must reject all secretmem folios. +*
Quoted 8 lines
mm/gup.c EXPORT_SYMBOL(get_user_pages_unlocked); · line 2486
Quoted 5 lines
mm/gup.c +0
-0* Writing to pinned file-backed dirty tracked folios is inherently problematic * (see comment describing the writable_file_mapping_allowed() function). We * therefore try to avoid the most egregious case of a long-term mapping doing @@ -2484,22 +2486,32 @@ EXPORT_SYMBOL(get_user_pages_unlocked);static bool folio_fast_pin_allowed (struct folio *folio,unsigned int flags)Now when this function checks for gup in general, maybe it's worth to rename it to, say,
folio_fast_gup_allowed.
Had the exact the same thought, so I'll do it!
Not sure about "fast gup" vs. "gup fast" vs. "lockless gup", it's all inconsistent and we should likely clean that up.
Likely, we should just prefix all relevant functions with "gup_fast". I'll call this "gup_fast_folio_allowed" for now.
The first description of the function becomes: "Used in the GUP-fast path to determine whether GUP is permitted to work on a specific folio."
Quoted 19 lines
Quoted 19 lines
mm/gup.c +6
-2{struct address_space *mapping; +bool check_secretmem = false; +bool reject_file_backed = false;unsigned long mapping_flags;/* * If we aren't pinning then no problematic write can occur. A long term * pin is the most egregious case so this is the one we disallow. */ -if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) != +if ((flags & (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) == (FOLL_PIN |FOLL_LONGTERM |FOLL_WRITE )) -return true; + reject_file_backed = true; + +/* We hold a folio reference, so we can safely access folio fields. */
Quoted 4 lines
mm/gup.c EXPORT_SYMBOL(get_user_pages_unlocked); · line 2503
Quoted 2 lines
mm/gup.c +1
-1-/* The folio is pinned, so we can safely access folio fields. */ +/* secretmem folios are only order-0 folios and never LRU folios. */ Nit: ^ always
ack
Thanks for the review!
Signature · 3 lines
Cheers, David / dhildenb