linux-kernel - Re: [PATCH 14/29] selftests/mm: uffd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZC3INzNZqZ1JNER8@x1n>
Date:   Wed, 5 Apr 2023 15:12:55 -0400
From:   Peter Xu <peterx@...hat.com>
To:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Cc:     Andrea Arcangeli <aarcange@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nadav Amit <nadav.amit@...il.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Leonardo Bras Soares Passos <lsoaresp@...hat.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>
Subject: Re: [PATCH 14/29] selftests/mm: uffd_[un]register()

On Thu, Mar 30, 2023 at 12:07:47PM -0400, Peter Xu wrote:
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index c57757c2a36f..17f2bb82c3db 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -330,33 +330,6 @@ void uffd_test_ctx_init(uint64_t features)
>  			err("pipe");
>  }
>  
> -uint64_t get_expected_ioctls(uint64_t mode)
> -{
> -	uint64_t ioctls = UFFD_API_RANGE_IOCTLS;
> -
> -	if (test_type == TEST_HUGETLB)
> -		ioctls &= ~(1 << _UFFDIO_ZEROPAGE);
> -
> -	if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp))
> -		ioctls &= ~(1 << _UFFDIO_WRITEPROTECT);
> -
> -	if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor))
> -		ioctls &= ~(1 << _UFFDIO_CONTINUE);
> -
> -	return ioctls;
> -}
> -
> -void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls)
> -{
> -	uint64_t expected = get_expected_ioctls(mode);
> -	uint64_t actual = ioctls & expected;
> -
> -	if (actual != expected) {
> -		err("missing ioctl(s): expected %"PRIx64" actual: %"PRIx64,
> -		    expected, actual);
> -	}
> -}

Here I dropped the other reference of get_expected_ioctls(), so I also
dropped this test which I think is kind of flawed IMHO - as I replied in
the other thread, we should probably not reference UFFD_API_RANGE_IOCTLS.

But I can feel (from the comments in the other patch that removed the other
reference of get_expected_ioctls()) that a lot of us would still care about
this test.

So I added a new patch / test on top of the series (so it'll have one more
patch in the next version at last), just to test all possible combinations
of UFFDIO_REGISTER alongside with its returned uffdio_register.ioctls.

This is IMHO better than get_expected_ioctls() because:

  - It's much cleaner to have a separate test on this rather than testing
    it randomly in the code with random values passed in.

  - It tests all combinations. It not only includes shmem-private that this
    series introduced while wasn't there before, but also all combinations
    of (miss, wp, minor) tuples.

  - It doesn't rely on UFFD_API_RANGE_IOCTLS anymore.

It'll be something like this:

===8<===
/*
 * Test the returned uffdio_register.ioctls with different register modes.
 * Note that _UFFDIO_ZEROPAGE is tested separately in the zeropage test.
 */
static void
do_register_ioctls_test(uffd_test_args_t *args, bool miss, bool wp, bool minor)
{
	uint64_t ioctls = 0, expected = BIT_ULL(_UFFDIO_WAKE);
	mem_type_t *mem_type = args->mem_type;
	int ret;

	ret = uffd_register_with_ioctls(uffd, area_dst, page_size,
					miss, wp, minor, &ioctls);

	/*
	 * Handle special cases of UFFDIO_REGISTER here where it should
	 * just fail with -EINVAL first..
	 *
	 * Case 1: register MINOR on anon
	 * Case 2: register with no mode selected
	 */
	if ((minor && (mem_type->mem_flag == MEM_ANON)) ||
	    (!miss && !wp && !minor)) {
		if (ret != -EINVAL)
			err("register (miss=%d, wp=%d, minor=%d) failed "
			    "with wrong errno=%d", miss, wp, minor, ret);
		return;
	}

	/* UFFDIO_REGISTER should succeed, then check ioctls returned */
	if (miss)
		expected |= BIT_ULL(_UFFDIO_COPY);
	if (wp)
		expected |= BIT_ULL(_UFFDIO_WRITEPROTECT);
	if (minor)
		expected |= BIT_ULL(_UFFDIO_CONTINUE);

	if ((ioctls & expected) != expected)
		err("unexpected uffdio_register.ioctls "
		    "(miss=%d, wp=%d, minor=%d): expected=0x%"PRIx64", "
		    "returned=0x%"PRIx64, miss, wp, minor, expected, ioctls);

	uffd_unregister(uffd, area_dst, page_size);
}

static void uffd_register_ioctls_test(uffd_test_args_t *args)
{
	int miss, wp, minor;

	for (miss = 0; miss <= 1; miss++)
		for (wp = 0; wp <= 1; wp++)
			for (minor = 0; minor <= 1; minor++)
				do_register_ioctls_test(args, miss, wp, minor);

	uffd_test_pass();
}
===8<===

Side note: the _UFFDIO_ZEROPAGE test will be left in the specific zeropage
test.

I considered moving get_expected_ioctls() rather than dropping in the same
patch, but that's just over-complicated when without the unit test
frameworks being ready.  I hope this addresses the concern here, otherwise
please shoot.

I've also attached the two patches that will test uffdio_register.ioctls as
a whole, just in case helpful for discussion before I post v2.

Thanks,

-- 
Peter Xu

View attachment "0001-selftests-mm-Move-zeropage-test-into-uffd-unit-tests.patch" of type "text/plain" (8902 bytes)

View attachment "0001-selftests-mm-Add-uffdio-register-ioctls-test.patch" of type "text/plain" (6857 bytes)