linux-kernel - Re: [PATCH v3 08/10] selftests/mm: Skip gup

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] [day] [month] [year] [list]

Message-ID: <16023193-6cb4-46d1-91c4-43342e7e6d30@redhat.com>
Date: Thu, 6 Mar 2025 15:40:28 +0100
From: David Hildenbrand <david@...hat.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>,
 Dev Jain <dev.jain@....com>, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird
 filesystems

On 06.03.25 13:42, Brendan Jackman wrote:
> On Thu, Mar 06, 2025 at 10:28:09AM +0100, David Hildenbrand wrote:
>> On 28.02.25 17:54, Brendan Jackman wrote:
>>> Some filesystems don't support funtract()ing unlinked files. They return
>>> ENOENT. In that case, skip the test.
>>>
>>
>> That's not documented in the man page, so is this a bug of these
>> filesystems?
> 
Note that I meant that ftruncate doesn't mention this in the man page.
The only occurrence is

"ENOENT The named file does not exist.", and that only applies to
truncate, not ftruncate.

> Um...
> 
> unlink(2) does say:
> 
>    If the name was the last link to a file but any processes still have
>    the file open, the file will remain in existence until the last file
>    descriptor referring to it is closed.
> 
> And POSIX says
> 
>    If one or more processes have the file open when the last link is
>    removed, the link shall be removed before unlink() returns, but the
>    removal of the file contents shall be postponed until all references
>    to the file are closed

Right, it's supposed to just stay around, it simply cannot be looked up anymore.

> I didn't call it a bug in the commit message because my impression was
> always that filesystem semantics are broadly determined by vibes. But
> looking at the above I do feel more confident that the "unlink isn't
> delete" thing is actually a pretty solid expectation.

I have a faint recollection that 9pfs is problematic with unlink ...
and indeed:

https://gitlab.com/qemu-project/qemu/-/issues/103

I'm not sure at this point what's expected to work and what not with
9pfs at this point.

> 
>> What are examples for these weird filesystems?
> 
> My experience of the issue is with 9pfs. broonie reported on #mm that
> NFS can display similar issues but I haven't hit it myself.
> >> As we have the fstype available, we could instead simply reject more
>> filesystems earlier. See fs_is_unknown().
> 
> Oh. I didn't know this was so easy, I thought that checking the
> filesystem type would require some awful walk to find the mountpoint
> and join it against the mount list. (Now I think about it, I should
> have recorded this rationale in the commit message, so you could
> easily see my bogus reasoning).
> 
> If there's a syscall to just say "what FS is this file on please?"
> we should just do that and explicitly denylist the systems that are
> known to have issues. I will just do 9pfs for now. Maybe we can log
> warning if the error shows up on systems that aren't listed, then if
> someone does run into it on NFS they should get a strong clue about
> what the problem is.

Yes, just skip 9pfs early, and mention in the commit message that 9pfs
has a history of being probematic with "use-after-unlink", maybe
mentioning the discussion I linked above.

Maybe something like this would work?

diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
index 9423ad439a614..349e40d3979f2 100644
--- a/tools/testing/selftests/mm/gup_longterm.c
+++ b/tools/testing/selftests/mm/gup_longterm.c
@@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd)
         return ret ? 0 : fs.f_type;
  }
  
+static bool fs_is_problematic(__fsword_t fs_type)
+{
+       switch (fs_type) {
+       case V9FS_MAGIC:
+               return false;
+       default:
+               return true;
+       }
+}
+
  static bool fs_is_unknown(__fsword_t fs_type)
  {
         /*
@@ -95,6 +105,11 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
         char *mem;
         int ret;
  
+       if (fs_is_problematic(fs_type)) {
+               ksft_test_result_skip("problematic fs\n");
+               return;
+       }
+
         if (ftruncate(fd, size)) {
                 ksft_test_result_fail("ftruncate() failed\n");
                 return;


I am not 100% sure if V9FS_MAGIC is what we should be using? "man fstatfs" lists
most magic values.

-- 
Cheers,

David / dhildenb