mm/mprotect: Fix soft-dirty checks
Subsystems (5)
v3 cover mm/mprotect: Fix soft-dirty checks
v2: https://lore.kernel.org/lkml/[email protected]/
Patch 1 is the previous patch and real fix. Two more test patches added to add mprotect test to soft-dirty.c, meanwhile add soft-dirty test into the vm test loop.
Please review, thanks.
Peter Xu (3):
mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
selftests: soft-dirty: Add test for mprotect
selftests: Add soft-dirty into run_vmtests.sh
mm/internal.h | 18 ++++++ mm/mmap.c | 2 +- mm/mprotect.c | 2 +- tools/testing/selftests/vm/run_vmtests.sh | 2 + tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++- 5 files changed, 90 insertions(+), 3 deletions(-)
Signature · 1 line
2.32.0
PATCH v3 1/3 mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
The check wanted to make sure when soft-dirty tracking is enabled we won't grant write bit by accident, as a page fault is needed for dirty tracking. The intention is correct but we didn't check it right because VM_SOFTDIRTY set actually means soft-dirty tracking disabled. Fix it.
There's another thing tricky about soft-dirty is that, we can't check the vma flag !(vma_flags & VM_SOFTDIRTY) directly but only check it after we checked CONFIG_MEM_SOFT_DIRTY because otherwise VM_SOFTDIRTY will be defined as zero, and !(vma_flags & VM_SOFTDIRTY) will constantly return true. To avoid misuse, introduce a helper for checking whether vma has soft-dirty tracking enabled.
We can easily verify this with any exclusive anonymous page, like program below:
=======8<======
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <assert.h>
#include <inttypes.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdbool.h>
#define BIT_ULL(nr) (1ULL << (nr)) #define PM_SOFT_DIRTY BIT_ULL(55)
unsigned int psize;
char *page;
uint64_t pagemap_read_vaddr(int fd, void *vaddr)
{
uint64_t value;
int ret; ret = pread(fd, &value, sizeof(uint64_t),
((uint64_t)vaddr >> 12) * sizeof(uint64_t));
assert(ret == sizeof(uint64_t));
return value;}
void clear_refs_write(void)
{
int fd = open("/proc/self/clear_refs", O_RDWR); assert(fd >= 0);
write(fd, "4", 2);
close(fd);}
#define check_soft_dirty(str, expect) do { \
bool dirty = pagemap_read_vaddr(fd, page) & PM_SOFT_DIRTY; \
if (dirty != expect) { \
printf("ERROR: %s, soft-dirty=%d (expect: %d)\n", str, dirty, expect); \
exit(-1); \
} \} while (0)
int main(void)
{
int fd = open("/proc/self/pagemap", O_RDONLY); assert(fd >= 0);
psize = getpagesize();
page = mmap(NULL, psize, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
assert(page != MAP_FAILED);
*page = 1;
check_soft_dirty("Just faulted in page", 1);
clear_refs_write();
check_soft_dirty("Clear_refs written", 0);
mprotect(page, psize, PROT_READ);
check_soft_dirty("Marked RO", 0);
mprotect(page, psize, PROT_READ|PROT_WRITE);
check_soft_dirty("Marked RW", 0);
*page = 2;
check_soft_dirty("Wrote page again", 1);
munmap(page, psize);
close(fd);
printf("Test passed.\n");
return 0; }
=======8<======
Here we attach a Fixes to commit 64fe24a3e05e only for easy tracking, as this patch won't apply to a tree before that point. However the commit wasn't the source of problem, it's just that then anonymous memory will also suffer from this problem with mprotect().
--- mm/internal.h | 18 ++++++++++++++++++ mm/mmap.c | 2 +- mm/mprotect.c | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-)
mm/internal.h +18 -0
@@ -860,4 +860,22 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); DECLARE_PER_CPU(struct per_cpu_nodestat, boot_nodestats); +static inline bool vma_soft_dirty_enabled (struct vm_area_struct *vma) +{ +/* +* NOTE: we must check this before VM_SOFTDIRTY on soft-dirty +* enablements, because when without soft-dirty being compiled in, +* VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) +* will be constantly true. +*/ +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) +return false; + +/* +* Soft-dirty is kind of special: its tracking is enabled when the +* vma flags not set. +*/ +return !(vma->vm_flags &VM_SOFTDIRTY ); +} +#endif /* __MM_INTERNAL_H */
mm/mmap.c +1 -1
@@ -1518,7 +1518,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)return 0 ;/* Do we need to track softdirty? */ -if (IS_ENABLED (CONFIG_MEM_SOFT_DIRTY ) && !(vm_flags &VM_SOFTDIRTY )) +if (vma_soft_dirty_enabled (vma))return 1 ;/* Specialty mapping? */
mm/mprotect.c +1 -1
@@ -49,7 +49,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,return false;/* Do we need write faults for softdirty tracking? */ -if ((vma->vm_flags &VM_SOFTDIRTY ) && !pte_soft_dirty (pte)) +if (vma_soft_dirty_enabled (vma) && !pte_soft_dirty (pte))return false;/* Do we need write faults for uffd-wp tracking? */
Signature · 1 line
2.32.0
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
Quoted 12 lines · Peter Xu
The check wanted to make sure when soft-dirty tracking is enabled we won't grant write bit by accident, as a page fault is needed for dirty tracking. The intention is correct but we didn't check it right because
VM_SOFTDIRTYset actually means soft-dirty tracking disabled. Fix it.There's another thing tricky about soft-dirty is that, we can't check the vma flag !(vma_flags &
VM_SOFTDIRTY) directly but only check it after we checkedCONFIG_MEM_SOFT_DIRTYbecause otherwiseVM_SOFTDIRTYwill be defined as zero, and !(vma_flags &VM_SOFTDIRTY) will constantly return true. To avoid misuse, introduce a helper for checking whether vma has soft-dirty tracking enabled.
[...]
Quoted 5 lines
Here we attach a Fixes to commit
64fe24a3e05eonly for easy tracking, as this patch won't apply to a tree before that point. However the commit wasn't the source of problem, it's just that then anonymous memory will also suffer from this problem withmprotect().
I'd remove that paragraph and also add
That introduced this wrong check for pagecache pages AFAIKS.
We don't care if the patch applies before 64fe24a3e05e, if someone wants to backport the fix, they can just adjust it accordingly.
Quoted 24 lines
Fixes 64fe24a3e05e ("mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection")Signed-off-by Peter Xu <[email protected]>--- mm/internal.h | 18 ++++++++++++++++++ mm/mmap.c | 2 +- mm/mprotect.c | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-)mm/internal.h +7
-0@@ -860,4 +860,22 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); DECLARE_PER_CPU(struct per_cpu_nodestat, boot_nodestats); +static inline bool vma_soft_dirty_enabled (struct vm_area_struct *vma) +{ +/* +* NOTE: we must check this before VM_SOFTDIRTY on soft-dirty +* enablements, because when without soft-dirty being compiled in, +* VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) +* will be constantly true.
mm/internal.h struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); · line 879
Quoted 10 lines
mm/internal.h +10
-0+*/ +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) +return false; + +/* +* Soft-dirty is kind of special: its tracking is enabled when the +* vma flags not set. +*/ +return !(vma->vm_flags &VM_SOFTDIRTY ); +}
That will come in handy in other patches I'm cooking.
Quoted 20 lines
mm/internal.h +1
-0+#endif /* __MM_INTERNAL_H */ mm/mmap.c +1
-1@@ -1518,7 +1518,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)return 0 ;/* Do we need to track softdirty? */ -if (IS_ENABLED (CONFIG_MEM_SOFT_DIRTY ) && !(vm_flags &VM_SOFTDIRTY )) +if (vma_soft_dirty_enabled (vma))return 1 ;/* Specialty mapping? */ mm/mprotect.c +0
-0
mm/mprotect.c static inline bool can_change_pte_writable(struct vm_area_struct *vma, · line 55
Quoted 8 lines
mm/mprotect.c +1
-1@@ -49,7 +49,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,return false;/* Do we need write faults for softdirty tracking? */ -if ((vma->vm_flags &VM_SOFTDIRTY ) && !pte_soft_dirty (pte)) +if (vma_soft_dirty_enabled (vma) && !pte_soft_dirty (pte))return false;/* Do we need write faults for uffd-wp tracking? */
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
Quoted 23 lines · David Hildenbrand
Quoted 12 lines · Peter Xu
The check wanted to make sure when soft-dirty tracking is enabled we won't grant write bit by accident, as a page fault is needed for dirty tracking. The intention is correct but we didn't check it right because
VM_SOFTDIRTYset actually means soft-dirty tracking disabled. Fix it.There's another thing tricky about soft-dirty is that, we can't check the vma flag !(vma_flags &
VM_SOFTDIRTY) directly but only check it after we checkedCONFIG_MEM_SOFT_DIRTYbecause otherwiseVM_SOFTDIRTYwill be defined as zero, and !(vma_flags &VM_SOFTDIRTY) will constantly return true. To avoid misuse, introduce a helper for checking whether vma has soft-dirty tracking enabled.[...]
Quoted 5 lines
Here we attach a Fixes to commit
64fe24a3e05eonly for easy tracking, as this patch won't apply to a tree before that point. However the commit wasn't the source of problem, it's just that then anonymous memory will also suffer from this problem withmprotect().
Quoted 8 lines
I'd remove that paragraph and also add
Fixes 64e455079e1b ("mm: softdirty: enable write notifications on VMAs after VM_SOFTDIRTY cleared")That introduced this wrong check for pagecache pages AFAIKS.
We don't care if the patch applies before
64fe24a3e05e, if someone wants to backport the fix, they can just adjust it accordingly.
IMO besides marking the culprit commit, Fixes can also provide input to stable trees to see whether we should try pick some patch up. What I wanted to express here is we don't need to try pick this patch up before kernel that doesn't have 64fe24a3e05e because it won't apply.
I can attach both Fixes with the hope that it'll help in both cases if you're fine with it, with slight explanations.
Quoted 65 lines
Quoted 25 lines
Fixes 64fe24a3e05e ("mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection")Signed-off-by Peter Xu <[email protected]>--- mm/internal.h | 18 ++++++++++++++++++ mm/mmap.c | 2 +- mm/mprotect.c | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-)mm/internal.h +7
-0@@ -860,4 +860,22 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); DECLARE_PER_CPU(struct per_cpu_nodestat, boot_nodestats); +static inline bool vma_soft_dirty_enabled (struct vm_area_struct *vma) +{ +/* +* NOTE: we must check this before VM_SOFTDIRTY on soft-dirty +* enablements, because when without soft-dirty being compiled in, +* VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) +* will be constantly true. mm/internal.h struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); · line 879
Quoted 10 lines
mm/internal.h +10
-0+*/ +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) +return false; + +/* +* Soft-dirty is kind of special: its tracking is enabled when the +* vma flags not set. +*/ +return !(vma->vm_flags &VM_SOFTDIRTY ); +}That will come in handy in other patches I'm cooking.
Quoted 27 lines
mm/internal.h +1
-0+#endif /* __MM_INTERNAL_H */ mm/mmap.c +1
-1@@ -1518,7 +1518,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)return 0 ;/* Do we need to track softdirty? */ -if (IS_ENABLED (CONFIG_MEM_SOFT_DIRTY ) && !(vm_flags &VM_SOFTDIRTY )) +if (vma_soft_dirty_enabled (vma))return 1 ;/* Specialty mapping? */ mm/mprotect.c +1
-1@@ -49,7 +49,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,return false;/* Do we need write faults for softdirty tracking? */ -if ((vma->vm_flags &VM_SOFTDIRTY ) && !pte_soft_dirty (pte)) +if (vma_soft_dirty_enabled (vma) && !pte_soft_dirty (pte))return false;
Quoted 4 lines
mm/mprotect.c static inline bool can_change_pte_writable(struct vm_area_struct *vma, · line 55
Quoted 1 line
mm/mprotect.c +0
-0/* Do we need write faults for uffd-wp tracking? */ Reviewed-by David Hildenbrand <[email protected]>
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
Quoted 33 lines · Peter Xu
Quoted 23 lines · David Hildenbrand
Quoted 12 lines · Peter Xu
The check wanted to make sure when soft-dirty tracking is enabled we won't grant write bit by accident, as a page fault is needed for dirty tracking. The intention is correct but we didn't check it right because
VM_SOFTDIRTYset actually means soft-dirty tracking disabled. Fix it.There's another thing tricky about soft-dirty is that, we can't check the vma flag !(vma_flags &
VM_SOFTDIRTY) directly but only check it after we checkedCONFIG_MEM_SOFT_DIRTYbecause otherwiseVM_SOFTDIRTYwill be defined as zero, and !(vma_flags &VM_SOFTDIRTY) will constantly return true. To avoid misuse, introduce a helper for checking whether vma has soft-dirty tracking enabled.[...]
Quoted 5 lines
Here we attach a Fixes to commit
64fe24a3e05eonly for easy tracking, as this patch won't apply to a tree before that point. However the commit wasn't the source of problem, it's just that then anonymous memory will also suffer from this problem withmprotect().Quoted 8 lines
I'd remove that paragraph and also add
Fixes 64e455079e1b ("mm: softdirty: enable write notifications on VMAs after VM_SOFTDIRTY cleared")That introduced this wrong check for pagecache pages AFAIKS.
We don't care if the patch applies before
64fe24a3e05e, if someone wants to backport the fix, they can just adjust it accordingly.
Quoted 7 lines
IMO besides marking the culprit commit, Fixes can also provide input to stable trees to see whether we should try pick some patch up. What I wanted to express here is we don't need to try pick this patch up before kernel that doesn't have
64fe24a3e05ebecause it won't apply.I can attach both Fixes with the hope that it'll help in both cases if you're fine with it, with slight explanations.
Makes sense. Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
In reply to David HildenbrandQuoted 11 lines · David Hildenbrand
Quoted 11 lines
mm/internal.h +11
-0+static inline bool vma_soft_dirty_enabled (struct vm_area_struct *vma) +{ +/* +* NOTE: we must check this before VM_SOFTDIRTY on soft-dirty +* enablements, because when without soft-dirty being compiled in, +* VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) +* will be constantly true. +*/ +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) +return false; +
Quoted 8 lines
mm/internal.h struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); · line 879
Quoted 6 lines
mm/internal.h +6
-0+/* +* Soft-dirty is kind of special: its tracking is enabled when the +* vma flags not set. +*/ +return !(vma->vm_flags &VM_SOFTDIRTY ); +}That will come in handy in other patches I'm cooking.
clear_refs_write() also comes to mind as well (for consistency; I see no correctness or performance issue).
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
Quoted 22 lines · Nadav Amit
Quoted 11 lines · David Hildenbrand
Quoted 11 lines
(quoted diff) +11
-0+static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) +{ + /* + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty + * enablements, because when without soft-dirty being compiled in, + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) + * will be constantly true. + */ + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) + return false; +Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* + * Soft-dirty is kind of special: its tracking is enabled when the + * vma flags not set. + */ + return !(vma->vm_flags & VM_SOFTDIRTY); +}That will come in handy in other patches I'm cooking.
Quoted 2 lines
clear_refs_write()also comes to mind as well (for consistency; I see no correctness or performance issue).
I explicitly didn't touch that because current code is better..
mas_for_each(&mas, vma, ULONG_MAX) {
if (!(vma->vm_flags & VM_SOFTDIRTY))
continue;
vma->vm_flags &= ~VM_SOFTDIRTY;
vma_set_page_prot(vma);
}It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all vma will be jumped.
If replaced with vma_soft_dirty_enabled() it'll be instead constantly false returned. We'll redo vma_set_page_prot() even if unnecessary.
Here if we want to add the "CONFIG_MEM_SOFT_DIRTY" into equation it can be:
fs/proc/task_mmu.c +2 -0
@@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf, }if (type ==CLEAR_REFS_SOFT_DIRTY ) { +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) + goto out_unlock;mas_for_each (&mas, vma,ULONG_MAX ) {if (!(vma->vm_flags &VM_SOFTDIRTY ))continue ;
Or even at the entrance to not take the mm sem. But it's not anything important IMHO, so if no one asking for that I'll just leave it be.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
Quoted 57 lines · Peter Xu
Quoted 22 lines · Nadav Amit
Quoted 11 lines · David Hildenbrand
Quoted 11 lines
(quoted diff) +11
-0+static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) +{ + /* + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty + * enablements, because when without soft-dirty being compiled in, + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) + * will be constantly true. + */ + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) + return false; +Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* + * Soft-dirty is kind of special: its tracking is enabled when the + * vma flags not set. + */ + return !(vma->vm_flags & VM_SOFTDIRTY); +}That will come in handy in other patches I'm cooking.
Quoted 2 lines
clear_refs_write()also comes to mind as well (for consistency; I see no correctness or performance issue).I explicitly didn't touch that because current code is better..
mas_for_each(&mas, vma, ULONG_MAX) { if (!(vma->vm_flags & VM_SOFTDIRTY)) continue; vma->vm_flags &= ~VM_SOFTDIRTY; vma_set_page_prot(vma); }It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all vma will be jumped.
If replaced with
vma_soft_dirty_enabled()it'll be instead constantly false returned. We'll redovma_set_page_prot()even if unnecessary.Here if we want to add the "
CONFIG_MEM_SOFT_DIRTY" into equation it can be:fs/proc/task_mmu.c +2
-0@@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf, }if (type ==CLEAR_REFS_SOFT_DIRTY ) { +if (!IS_ENABLED (CONFIG_MEM_SOFT_DIRTY )) + goto out_unlock;mas_for_each (&mas, vma,ULONG_MAX ) {if (!(vma->vm_flags &VM_SOFTDIRTY ))continue ;
Quoted 2 lines
Or even at the entrance to not take the mm sem. But it's not anything important IMHO, so if no one asking for that I'll just leave it be.
Yeah, I don't think we particularly care about that.
Signature · 3 lines
Thanks, David / dhildenb
PATCH v3 2/3 selftests: soft-dirty: Add test for mprotect
In reply to Peter XuAdd two soft-diryt test cases for mprotect() on both anon or file.
--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)
tools/testing/selftests/vm/soft-dirty.c +68 -1
@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map; + +if (anon) { + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 ); +if (!map) +ksft_exit_fail_msg ("anon mmap failed\n" ); + }else { +unlink (fname); + test_fd =open (fname,O_RDWR |O_CREAT ); +if (test_fd <0 ) { +ksft_test_result_skip ("Test %s huge page allocation\n" , __func__); +return ; + } +ftruncate (test_fd, pagesize); + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_SHARED , test_fd,0 ); +if (!map) +ksft_exit_fail_msg ("file mmap failed\n" ); + } + + *map =1 ; +ksft_test_result (pagemap_is_softdirty (pagemap_fd, map) ==1 , +"Test %s-%s dirty bit of new written page\n" , + __func__, type[anon]); +clear_softdirty (); +ksft_test_result (pagemap_is_softdirty (pagemap_fd, map) ==0 , +"Test %s-%s soft-dirty clear after clear_refs\n" , + __func__, type[anon]); +mprotect (map, pagesize,PROT_READ ); +ksft_test_result (pagemap_is_softdirty (pagemap_fd, map) ==0 , +"Test %s-%s soft-dirty clear after marking RO\n" , + __func__, type[anon]); +mprotect (map, pagesize,PROT_READ |PROT_WRITE ); +ksft_test_result (pagemap_is_softdirty (pagemap_fd, map) ==0 , +"Test %s-%s soft-dirty clear after marking RW\n" , + __func__, type[anon]); + *map =2 ; +ksft_test_result (pagemap_is_softdirty (pagemap_fd, map) ==1 , +"Test %s-%s soft-dirty after rewritten\n" , + __func__, type[anon]); + +munmap (map, pagesize); + +if (!anon) { +close (test_fd); +unlink (fname); + } +} + +static void test_mprotect_anon (int pagemap_fd,int pagesize) +{ +test_mprotect (pagemap_fd, pagesize, true); +} + +static void test_mprotect_file (int pagemap_fd,int pagesize) +{ +test_mprotect (pagemap_fd, pagesize, false); +} +int main (int argc,char **argv) {int pagemap_fd;int pagesize;ksft_print_header (); -ksft_set_plan (5 ); +ksft_set_plan (15 ); pagemap_fd =open (PAGEMAP_FILE_PATH ,O_RDONLY );if (pagemap_fd <0 )
@@ -138,6 +203,8 @@ int main(int argc, char **argv)test_simple (pagemap_fd, pagesize);test_vma_reuse (pagemap_fd, pagesize);test_hugepage (pagemap_fd, pagesize); +test_mprotect_anon (pagemap_fd, pagesize); +test_mprotect_file (pagemap_fd, pagesize);close (pagemap_fd);
Signature · 1 line
2.32.0
Re: [PATCH v3 2/3] selftests: soft-dirty: Add test for mprotect
Quoted 1 line · Peter Xu
Add two soft-diryt test cases for
mprotect()on both anon or file.
s/soft-diryt/soft-dirty/
Quoted 11 lines
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)tools/testing/selftests/vm/soft-dirty.c +0
-0
tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 129
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +6
-0@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map;
Instead of fname, unlink, open, close, unlink you can use a tmpfile
FILE *file;
file = tmpfile();
if (!file) {
ksft_test_result_fail("tmpfile() failed\n");
return;
}test_fd = fileno(file);
tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 140
Quoted 11 lines
tools/testing/selftests/vm/soft-dirty.c +11
-0+ +if (anon) { + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 ); +if (!map) +ksft_exit_fail_msg ("anon mmap failed\n" ); + }else { +unlink (fname); + test_fd =open (fname,O_RDWR |O_CREAT ); +if (test_fd <0 ) { +ksft_test_result_skip ("Test %s huge page allocation\n" , __func__);
Wrong copy-paste I assume :)
tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 149
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +9
-0+return ; + } +ftruncate (test_fd, pagesize); + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_SHARED , test_fd,0 ); +if (!map) +ksft_exit_fail_msg ("file mmap failed\n" ); + } +
Apart from that LGTM.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v3 2/3] selftests: soft-dirty: Add test for mprotect
Quoted 4 lines · David Hildenbrand
Quoted 1 line · Peter Xu
Add two soft-diryt test cases for
mprotect()on both anon or file.s/soft-diryt/soft-dirty/
Fixed.
Quoted 24 lines
Quoted 12 lines
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)tools/testing/selftests/vm/soft-dirty.c +0
-0tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 129
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +6
-0@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map;Instead of fname, unlink, open, close, unlink you can use a tmpfile
Quoted 8 lines
FILE *file;
file =
tmpfile();if (!file) { ksft_test_result_fail("tmpfile() failed\n"); return; }
test_fd= fileno(file);
Note that tmpfile() should by default fetch from /tmp which is very possibly a tmpfs afaict. It's tricky in this special test case since I don't think tmpfs can trigger this bug (shmem doesn't define page_mkwrite).
I wanted to create under this dir to make the best possible bet to trigger the bug. E.g. major fs will works like xfs, btrfs, extN. It'll stop work if someone clones the Linux repo under tmpfs but it's rare.
Quoted 14 lines
tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 140
Quoted 12 lines
tools/testing/selftests/vm/soft-dirty.c +11
-0+ +if (anon) { + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 ); +if (!map) +ksft_exit_fail_msg ("anon mmap failed\n" ); + }else { +unlink (fname); + test_fd =open (fname,O_RDWR |O_CREAT ); +if (test_fd <0 ) { +ksft_test_result_skip ("Test %s huge page allocation\n" , __func__);Wrong copy-paste I assume :)
Yeh :) I'll fix it.
Quoted 12 lines
tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 149
Quoted 10 lines
tools/testing/selftests/vm/soft-dirty.c +9
-0+return ; + } +ftruncate (test_fd, pagesize); + map =mmap (NULL , pagesize,PROT_READ |PROT_WRITE , +MAP_SHARED , test_fd,0 ); +if (!map) +ksft_exit_fail_msg ("file mmap failed\n" ); + } +Apart from that LGTM.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH v3 2/3] selftests: soft-dirty: Add test for mprotect
Quoted 41 lines · Peter Xu
Quoted 4 lines · David Hildenbrand
Quoted 1 line · Peter Xu
Add two soft-diryt test cases for
mprotect()on both anon or file.s/soft-diryt/soft-dirty/
Fixed.
Quoted 24 lines
Quoted 12 lines
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)tools/testing/selftests/vm/soft-dirty.c +0
-0tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 129
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +6
-0@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map;Instead of fname, unlink, open, close, unlink you can use a tmpfile
Quoted 8 lines
FILE *file;
file =
tmpfile();if (!file) { ksft_test_result_fail("tmpfile() failed\n"); return; }
test_fd= fileno(file);
Quoted 4 lines
Note that
tmpfile()should by default fetch from /tmp which is very possibly a tmpfs afaict. It's tricky in this special test case since I don't think tmpfs can trigger this bug (shmem doesn't definepage_mkwrite).
I don't think we need that? SOFTDIRTY tracking enabled should be sufficient, or what am I missing?
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v3 2/3] selftests: soft-dirty: Add test for mprotect
Quoted 42 lines · David Hildenbrand
Quoted 41 lines · Peter Xu
Quoted 4 lines · David Hildenbrand
Quoted 1 line · Peter Xu
Add two soft-diryt test cases for
mprotect()on both anon or file.s/soft-diryt/soft-dirty/
Fixed.
Quoted 32 lines
Quoted 12 lines
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)tools/testing/selftests/vm/soft-dirty.c +0
-0tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 129
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +6
-0@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map;Instead of fname, unlink, open, close, unlink you can use a tmpfile
FILE *file;
file =
tmpfile();if (!file) { ksft_test_result_fail("tmpfile() failed\n"); return; }
test_fd= fileno(file);
Quoted 8 lines
Quoted 4 lines
Note that
tmpfile()should by default fetch from /tmp which is very possibly a tmpfs afaict. It's tricky in this special test case since I don't think tmpfs can trigger this bug (shmem doesn't definepage_mkwrite).I don't think we need that? SOFTDIRTY tracking enabled should be sufficient, or what am I missing?
I think you're right that it doesn't work with tmpfile. I do wonder why, because I'd have thought that it's sufficient for vma_wants_writenotify() to return "1" due to the vma_soft_dirty_enabled() check.
Hm ....
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH v3 2/3] selftests: soft-dirty: Add test for mprotect
Quoted 48 lines · David Hildenbrand
Quoted 47 lines · David Hildenbrand
Quoted 45 lines · Peter Xu
Quoted 4 lines · David Hildenbrand
Quoted 1 line · Peter Xu
Add two soft-diryt test cases for
mprotect()on both anon or file.s/soft-diryt/soft-dirty/
Fixed.
Quoted 24 lines
Quoted 12 lines
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)tools/testing/selftests/vm/soft-dirty.c +0
-0tools/testing/selftests/vm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 129
Quoted 9 lines
tools/testing/selftests/vm/soft-dirty.c +6
-0@@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)free (map); } +static void test_mprotect (int pagemap_fd,int pagesize,bool anon) +{ +const char *type[] = {"file" ,"anon" }; +const char *fname ="./soft-dirty-test-file" ; +int test_fd; +char *map;Instead of fname, unlink, open, close, unlink you can use a tmpfile
Quoted 8 lines
FILE *file;
file =
tmpfile();if (!file) { ksft_test_result_fail("tmpfile() failed\n"); return; }
test_fd= fileno(file);Note that
tmpfile()should by default fetch from /tmp which is very possibly a tmpfs afaict. It's tricky in this special test case since I don't think tmpfs can trigger this bug (shmem doesn't definepage_mkwrite).
Quoted 8 lines
Quoted 3 lines
I don't think we need that? SOFTDIRTY tracking enabled should be sufficient, or what am I missing?
I think you're right that it doesn't work with tmpfile. I do wonder why, because I'd have thought that it's sufficient for
vma_wants_writenotify()to return "1" due to thevma_soft_dirty_enabled()check.
I can't say I know the whole rational of this, but I think it's below that will start to return 0 already before the soft-dirty check:
if (pgprot_val(vm_page_prot) != pgprot_val(vm_pgprot_modify(vm_page_prot, vm_flags))) return 0;
in vma_wants_writenotify().
Signature · 1 line
Peter Xu
PATCH v3 3/3 selftests: Add soft-dirty into run_vmtests.sh
In reply to Peter Xu--- tools/testing/selftests/vm/run_vmtests.sh | 2 ++ 1 file changed, 2 insertions(+)
tools/testing/selftests/vm/run_vmtests.sh +2 -0
@@ -190,4 +190,6 @@ then run_test ./protection_keys_64 fi +run_test ./soft-dirty + exit $exitcode
Signature · 1 line
2.32.0
Re: [PATCH v3 3/3] selftests: Add soft-dirty into run_vmtests.sh
Quoted 10 lines · Peter Xu
Signed-off-by Peter Xu <[email protected]>--- tools/testing/selftests/vm/run_vmtests.sh | 2 ++ 1 file changed, 2 insertions(+)tools/testing/selftests/vm/run_vmtests.sh +0
-0
tools/testing/selftests/vm/run_vmtests.sh then · line 195
Quoted 6 lines
tools/testing/selftests/vm/run_vmtests.sh +2
-0@@ -190,4 +190,6 @@ then run_test ./protection_keys_64 fi +run_test ./soft-dirty + exit $exitcode
Signature · 3 lines
Thanks, David / dhildenb