linux-kernel - Re: [PATCH 1/3] selftests: vm: add process

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]

Date:   Tue, 10 May 2022 10:35:57 -0600
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Suren Baghdasaryan <surenb@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>,
        David Rientjes <rientjes@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Roman Gushchin <guro@...com>, Minchan Kim <minchan@...nel.org>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Christian Brauner <brauner@...nel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Oleg Nesterov <oleg@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Jann Horn <jannh@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Peter Xu <peterx@...hat.com>,
        John Hubbard <jhubbard@...dia.com>, shuah@...nel.org,
        LKML <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>, linux-kselftest@...r.kernel.org,
        kernel-team <kernel-team@...roid.com>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 1/3] selftests: vm: add process_mrelease tests

On 5/10/22 10:29 AM, Suren Baghdasaryan wrote:
> On Tue, May 10, 2022 at 8:43 AM Shuah Khan <skhan@...uxfoundation.org> wrote:
>>
>> On 5/9/22 9:00 PM, Suren Baghdasaryan wrote:
>>> Introduce process_mrelease syscall sanity tests. They include tests of
>>> invalid pidfd and flags inputs, attempting to call process_mrelease
>>> with a live process and a valid usage of process_mrelease. Because
>>> process_mrelease has to be used against a process with a pending SIGKILL,
>>> it's possible that the process exits before process_mrelease gets called.
>>> In such cases we retry the test with a victim that allocates twice more
>>> memory up to 1GB. This would require the victim process to spend more
>>> time during exit and process_mrelease has a better chance of catching
>>> the process before it exits.
>>>
>>
>> +1 on Mike's comments on improving the change log. List what is getting
>> tested as opposed to describing the test code.
> 
> I'll try to improve the description but IMHO it does describe what
> it's testing - the process_mrelease syscall with valid and invalid
> inputs. I could omit the implementation details if that helps.
> 
>>
>>> Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
>>> ---
>>>    tools/testing/selftests/vm/Makefile        |   1 +
>>>    tools/testing/selftests/vm/mrelease_test.c | 176 +++++++++++++++++++++
>>>    tools/testing/selftests/vm/run_vmtests.sh  |  16 ++
>>>    3 files changed, 193 insertions(+)
>>>    create mode 100644 tools/testing/selftests/vm/mrelease_test.c
>>
>> Please update .gitignore with the new executable.
> 
> Ack.
> 
>>
>>>
>>> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
>>> index 04a49e876a46..733fccbff0ef 100644
>>> --- a/tools/testing/selftests/vm/Makefile
>>> +++ b/tools/testing/selftests/vm/Makefile
>>> @@ -43,6 +43,7 @@ TEST_GEN_FILES += map_populate
>>>    TEST_GEN_FILES += memfd_secret
>>>    TEST_GEN_FILES += mlock-random-test
>>>    TEST_GEN_FILES += mlock2-tests
>>> +TEST_GEN_FILES += mrelease_test
>>>    TEST_GEN_FILES += mremap_dontunmap
>>>    TEST_GEN_FILES += mremap_test
>>>    TEST_GEN_FILES += on-fault-limit
>>> diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c
>>> new file mode 100644
>>> index 000000000000..a61061bf8433
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/vm/mrelease_test.c
>>> @@ -0,0 +1,176 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 2022 Google LLC
>>> + */
>>> +#define _GNU_SOURCE
>>> +#include <errno.h>
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <sys/wait.h>
>>> +#include <unistd.h>
>>> +
>>> +#include "util.h"
>>> +
>>> +static inline int pidfd_open(pid_t pid, unsigned int flags)
>>> +{
>>> +#ifdef __NR_pidfd_open
>>> +     return syscall(__NR_pidfd_open, pid, flags);
>>> +#else
>>> +     errno = ENOSYS;
>>
>> This isn't an error - this would be skip because this syscall
>> isn't supported.
> 
> Ack.
> 
>>
>>> +     return -1;
>>> +#endif
>>
>> Key off of syscall return instead of these ifdefs - same comment
>> on all of the ifdefs
> 
> Ack. I was using some other test as an example but I guess that was
> not a good model.
> 
>>
>>> +}
>>> +
>>
>> I am not seeing any reason for breaking this code up have a separate
>> routine for pidfd_open().
> 
> I'm a bit unclear what you mean. Do you mean that userspace headers
> should already define pidfd_open() and I don't need to define it?
> 

Do you need pidfd_open() or can this be part of main? Without the ifdefs,
it is really a one line code.

thanks,
-- Shuah