Re: [PATCH] tools/testing/selftests/vm/mlock2-tests: fix mlock2 false-negative errors
From: Rafael Aquini
Date: Tue Mar 24 2020 - 11:49:31 EST
On Tue, Mar 24, 2020 at 04:42:18PM +0100, Michal Hocko wrote:
> [Cc Eric - the email thread starts http://lkml.kernel.org/r/20200322013525.1095493-1-aquini@xxxxxxxxxx]
>
> On Mon 23-03-20 11:54:49, Rafael Aquini wrote:
> [...]
> > I'm OK with it, if you want to go ahead and do the kill.
>
> See the patch below
>
> From 07c08f387d036c70239d4060ffd30534cf77a0a5 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@xxxxxxxx>
> Date: Mon, 23 Mar 2020 17:33:46 +0100
> Subject: [PATCH] selftests: vm: drop dependencies on page flags from mlock2
> tests
>
> It was noticed that mlock2 tests are failing after 9c4e6b1a7027f
> ("mm, mlock, vmscan: no more skipping pagevecs") because the patch has
> changed the timing on when the page is added to the unevictable LRU list
> and thus gains the unevictable page flag.
>
> The test was just too dependent on the implementation details which were
> true at the time when it was introduced. Page flags and the timing when
> they are set is something no userspace should ever depend on. The test
> should be testing only for the user observable contract of the tested
> syscalls. Those are defined pretty well for the mlock and there are
> other means for testing them. In fact this is already done and testing
> for page flags can be safely dropped to achieve the aimed purpose.
> Present bits can be checked by /proc/<pid>/smaps RSS field and the
> locking state by VmFlags although I would argue that Locked: field would
> be more appropriate.
>
> Drop all the page flag machinery and considerably simplify the test.
> This should be more robust for future kernel changes while checking the
> promised contract is still valid.
>
> Reported-by: Rafael Aquini <aquini@xxxxxxxxxx>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> tools/testing/selftests/vm/mlock2-tests.c | 233 ++++------------------
> 1 file changed, 37 insertions(+), 196 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/mlock2-tests.c b/tools/testing/selftests/vm/mlock2-tests.c
> index 637b6d0ac0d0..11b2301f3aa3 100644
> --- a/tools/testing/selftests/vm/mlock2-tests.c
> +++ b/tools/testing/selftests/vm/mlock2-tests.c
> @@ -67,59 +67,6 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
> return ret;
> }
>
> -static uint64_t get_pageflags(unsigned long addr)
> -{
> - FILE *file;
> - uint64_t pfn;
> - unsigned long offset;
> -
> - file = fopen("/proc/self/pagemap", "r");
> - if (!file) {
> - perror("fopen pagemap");
> - _exit(1);
> - }
> -
> - offset = addr / getpagesize() * sizeof(pfn);
> -
> - if (fseek(file, offset, SEEK_SET)) {
> - perror("fseek pagemap");
> - _exit(1);
> - }
> -
> - if (fread(&pfn, sizeof(pfn), 1, file) != 1) {
> - perror("fread pagemap");
> - _exit(1);
> - }
> -
> - fclose(file);
> - return pfn;
> -}
> -
> -static uint64_t get_kpageflags(unsigned long pfn)
> -{
> - uint64_t flags;
> - FILE *file;
> -
> - file = fopen("/proc/kpageflags", "r");
> - if (!file) {
> - perror("fopen kpageflags");
> - _exit(1);
> - }
> -
> - if (fseek(file, pfn * sizeof(flags), SEEK_SET)) {
> - perror("fseek kpageflags");
> - _exit(1);
> - }
> -
> - if (fread(&flags, sizeof(flags), 1, file) != 1) {
> - perror("fread kpageflags");
> - _exit(1);
> - }
> -
> - fclose(file);
> - return flags;
> -}
> -
> #define VMFLAGS "VmFlags:"
>
> static bool is_vmflag_set(unsigned long addr, const char *vmflag)
> @@ -159,19 +106,13 @@ static bool is_vmflag_set(unsigned long addr, const char *vmflag)
> #define RSS "Rss:"
> #define LOCKED "lo"
>
> -static bool is_vma_lock_on_fault(unsigned long addr)
> +static unsigned long get_value_for_name(unsigned long addr, const char *name)
> {
> - bool ret = false;
> - bool locked;
> - FILE *smaps = NULL;
> - unsigned long vma_size, vma_rss;
> char *line = NULL;
> - char *value;
> size_t size = 0;
> -
> - locked = is_vmflag_set(addr, LOCKED);
> - if (!locked)
> - goto out;
> + char *value_ptr;
> + FILE *smaps = NULL;
> + unsigned long value = -1UL;
>
> smaps = seek_to_smaps_entry(addr);
> if (!smaps) {
> @@ -180,112 +121,70 @@ static bool is_vma_lock_on_fault(unsigned long addr)
> }
>
> while (getline(&line, &size, smaps) > 0) {
> - if (!strstr(line, SIZE)) {
> + if (!strstr(line, name)) {
> free(line);
> line = NULL;
> size = 0;
> continue;
> }
>
> - value = line + strlen(SIZE);
> - if (sscanf(value, "%lu kB", &vma_size) < 1) {
> + value_ptr = line + strlen(name);
> + if (sscanf(value_ptr, "%lu kB", &value) < 1) {
> printf("Unable to parse smaps entry for Size\n");
> goto out;
> }
> break;
> }
>
> - while (getline(&line, &size, smaps) > 0) {
> - if (!strstr(line, RSS)) {
> - free(line);
> - line = NULL;
> - size = 0;
> - continue;
> - }
> -
> - value = line + strlen(RSS);
> - if (sscanf(value, "%lu kB", &vma_rss) < 1) {
> - printf("Unable to parse smaps entry for Rss\n");
> - goto out;
> - }
> - break;
> - }
> -
> - ret = locked && (vma_rss < vma_size);
> out:
> - free(line);
> if (smaps)
> fclose(smaps);
> - return ret;
> + free(line);
> + return value;
> }
>
> -#define PRESENT_BIT 0x8000000000000000ULL
> -#define PFN_MASK 0x007FFFFFFFFFFFFFULL
> -#define UNEVICTABLE_BIT (1UL << 18)
> -
> -static int lock_check(char *map)
> +static bool is_vma_lock_on_fault(unsigned long addr)
> {
> - unsigned long page_size = getpagesize();
> - uint64_t page1_flags, page2_flags;
> + bool locked;
> + unsigned long vma_size, vma_rss;
>
> - page1_flags = get_pageflags((unsigned long)map);
> - page2_flags = get_pageflags((unsigned long)map + page_size);
> + locked = is_vmflag_set(addr, LOCKED);
> + if (!locked)
> + return false;
>
> - /* Both pages should be present */
> - if (((page1_flags & PRESENT_BIT) == 0) ||
> - ((page2_flags & PRESENT_BIT) == 0)) {
> - printf("Failed to make both pages present\n");
> - return 1;
> - }
> + vma_size = get_value_for_name(addr, SIZE);
> + vma_rss = get_value_for_name(addr, RSS);
>
> - page1_flags = get_kpageflags(page1_flags & PFN_MASK);
> - page2_flags = get_kpageflags(page2_flags & PFN_MASK);
> + /* only one page is faulted in */
> + return (vma_rss < vma_size);
> +}
>
> - /* Both pages should be unevictable */
> - if (((page1_flags & UNEVICTABLE_BIT) == 0) ||
> - ((page2_flags & UNEVICTABLE_BIT) == 0)) {
> - printf("Failed to make both pages unevictable\n");
> - return 1;
> - }
> +#define PRESENT_BIT 0x8000000000000000ULL
> +#define PFN_MASK 0x007FFFFFFFFFFFFFULL
> +#define UNEVICTABLE_BIT (1UL << 18)
>
> - if (!is_vmflag_set((unsigned long)map, LOCKED)) {
> - printf("VMA flag %s is missing on page 1\n", LOCKED);
> - return 1;
> - }
> +static int lock_check(unsigned long addr)
> +{
> + bool locked;
> + unsigned long vma_size, vma_rss;
>
> - if (!is_vmflag_set((unsigned long)map + page_size, LOCKED)) {
> - printf("VMA flag %s is missing on page 2\n", LOCKED);
> - return 1;
> - }
> + locked = is_vmflag_set(addr, LOCKED);
> + if (!locked)
> + return false;
>
> - return 0;
> + vma_size = get_value_for_name(addr, SIZE);
> + vma_rss = get_value_for_name(addr, RSS);
> +
> + return (vma_rss == vma_size);
> }
>
> static int unlock_lock_check(char *map)
> {
> - unsigned long page_size = getpagesize();
> - uint64_t page1_flags, page2_flags;
> -
> - page1_flags = get_pageflags((unsigned long)map);
> - page2_flags = get_pageflags((unsigned long)map + page_size);
> - page1_flags = get_kpageflags(page1_flags & PFN_MASK);
> - page2_flags = get_kpageflags(page2_flags & PFN_MASK);
> -
> - if ((page1_flags & UNEVICTABLE_BIT) || (page2_flags & UNEVICTABLE_BIT)) {
> - printf("A page is still marked unevictable after unlock\n");
> - return 1;
> - }
> -
> if (is_vmflag_set((unsigned long)map, LOCKED)) {
> printf("VMA flag %s is present on page 1 after unlock\n", LOCKED);
> return 1;
> }
>
> - if (is_vmflag_set((unsigned long)map + page_size, LOCKED)) {
> - printf("VMA flag %s is present on page 2 after unlock\n", LOCKED);
> - return 1;
> - }
> -
> return 0;
> }
>
> @@ -311,7 +210,7 @@ static int test_mlock_lock()
> goto unmap;
> }
>
> - if (lock_check(map))
> + if (!lock_check((unsigned long)map))
> goto unmap;
>
> /* Now unlock and recheck attributes */
> @@ -330,64 +229,18 @@ static int test_mlock_lock()
>
> static int onfault_check(char *map)
> {
> - unsigned long page_size = getpagesize();
> - uint64_t page1_flags, page2_flags;
> -
> - page1_flags = get_pageflags((unsigned long)map);
> - page2_flags = get_pageflags((unsigned long)map + page_size);
> -
> - /* Neither page should be present */
> - if ((page1_flags & PRESENT_BIT) || (page2_flags & PRESENT_BIT)) {
> - printf("Pages were made present by MLOCK_ONFAULT\n");
> - return 1;
> - }
> -
> *map = 'a';
> - page1_flags = get_pageflags((unsigned long)map);
> - page2_flags = get_pageflags((unsigned long)map + page_size);
> -
> - /* Only page 1 should be present */
> - if ((page1_flags & PRESENT_BIT) == 0) {
> - printf("Page 1 is not present after fault\n");
> - return 1;
> - } else if (page2_flags & PRESENT_BIT) {
> - printf("Page 2 was made present\n");
> - return 1;
> - }
> -
> - page1_flags = get_kpageflags(page1_flags & PFN_MASK);
> -
> - /* Page 1 should be unevictable */
> - if ((page1_flags & UNEVICTABLE_BIT) == 0) {
> - printf("Failed to make faulted page unevictable\n");
> - return 1;
> - }
> -
> if (!is_vma_lock_on_fault((unsigned long)map)) {
> printf("VMA is not marked for lock on fault\n");
> return 1;
> }
>
> - if (!is_vma_lock_on_fault((unsigned long)map + page_size)) {
> - printf("VMA is not marked for lock on fault\n");
> - return 1;
> - }
> -
> return 0;
> }
>
> static int unlock_onfault_check(char *map)
> {
> unsigned long page_size = getpagesize();
> - uint64_t page1_flags;
> -
> - page1_flags = get_pageflags((unsigned long)map);
> - page1_flags = get_kpageflags(page1_flags & PFN_MASK);
> -
> - if (page1_flags & UNEVICTABLE_BIT) {
> - printf("Page 1 is still marked unevictable after unlock\n");
> - return 1;
> - }
>
> if (is_vma_lock_on_fault((unsigned long)map) ||
> is_vma_lock_on_fault((unsigned long)map + page_size)) {
> @@ -445,7 +298,6 @@ static int test_lock_onfault_of_present()
> char *map;
> int ret = 1;
> unsigned long page_size = getpagesize();
> - uint64_t page1_flags, page2_flags;
>
> map = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> @@ -465,17 +317,6 @@ static int test_lock_onfault_of_present()
> goto unmap;
> }
>
> - page1_flags = get_pageflags((unsigned long)map);
> - page2_flags = get_pageflags((unsigned long)map + page_size);
> - page1_flags = get_kpageflags(page1_flags & PFN_MASK);
> - page2_flags = get_kpageflags(page2_flags & PFN_MASK);
> -
> - /* Page 1 should be unevictable */
> - if ((page1_flags & UNEVICTABLE_BIT) == 0) {
> - printf("Failed to make present page unevictable\n");
> - goto unmap;
> - }
> -
> if (!is_vma_lock_on_fault((unsigned long)map) ||
> !is_vma_lock_on_fault((unsigned long)map + page_size)) {
> printf("VMA with present pages is not marked lock on fault\n");
> @@ -507,7 +348,7 @@ static int test_munlockall()
> goto out;
> }
>
> - if (lock_check(map))
> + if (!lock_check((unsigned long)map))
> goto unmap;
>
> if (munlockall()) {
> @@ -549,7 +390,7 @@ static int test_munlockall()
> goto out;
> }
>
> - if (lock_check(map))
> + if (!lock_check((unsigned long)map))
> goto unmap;
>
> if (munlockall()) {
> --
> 2.25.1
>
> --
> Michal Hocko
> SUSE Labs
>
Thanks Michal!
Acked-by: Rafael Aquini <aquini@xxxxxxxxxx>