Re: [PATCH v1 2/3] selftests/memfd_secret: add vmsplice() test

From: David Hildenbrand <david@redhat.com>
To: Mike Rapoport <rppt@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Miklos Szeredi <mszeredi@redhat.com>,
	Lorenzo Stoakes <lstoakes@gmail.com>,
	xingwei lee <xrivendell7@gmail.com>,
	yue sun <samsun1006219@gmail.com>
Subject: Re: [PATCH v1 2/3] selftests/memfd_secret: add vmsplice() test
Date: Tue, 26 Mar 2024 14:11:56 +0100	[thread overview]
Message-ID: <15117d1a-a03b-4ad5-9900-4c968212137f@redhat.com> (raw)
In-Reply-To: <baafcfbb-f767-41fb-b2de-1367991d073a@redhat.com>

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.

-- 
Cheers,

David / dhildenb